1:01 p.m.
Attached please find the update.
Two things to note:
1. for comment #2, as discussed over irc, I put the auth manager id in
the authToken instead. Turns out the session contaxt has the whole
authToken in it, so there is no need to put it in separately in the
session context.
2. for comment #3, the difference between the password based and cert
based auth is that by the time it gets here, cert based auth already
passed the ssl auth, so we know exactly who the subject is, and what
remains is just a matter of mapping it to the right user in the
internaldb. Unlike cert based auth, the password based auth could be
anyone attempted to be the uid provided in the auth, so the "attempted"
is more useful in capturing the attempt.
I changed it so that for cert based auth now has "attemptedUID" to be
the same as that of the subjectid, and I added comment to explain that.
The two auth methods are going to be different, and for a good reason.
I addressed the rest of the comments as requested.
thanks,
Christina
On 05/11/2015 04:26 PM, Endi Sukma Dewata wrote:
On 5/11/2015 1:39 PM, Christina Fu wrote:
> This updated patch address the issue that Endi found which would cause
> startup to fail for anonymous access.
>
> thanks,
> Christina
More comments:
1. Let's not use Hungarian notation in new code:
protected ILogger mSignedAuditLogger = CMS.getSignedAuditLogger();
2. The PKIRealm and the REST method may be executed by different
threads (see ticket #1054), so the following code may not work as
intended:
SessionContext ctx = SessionContext.getContext();
ctx.put(SessionContext.AUTH_MANAGER_ID, ...);
The code should be moved into SessionContextInterceptor and the
authManager should be obtained/inferred from the authToken:
PKIPrincipal pkiPrincipal = (PKIPrincipal) principal;
IAuthToken authToken = pkiPrincipal.getAuthToken();
String authManager = ...
SessionContext ctx = SessionContext.getContext();
ctx.put(SessionContext.AUTH_MANAGER_ID, authManager);
3. If the authentication fails for any reason, the subject ID and
attempted audit UID are not consistently logged. In
PKIRealm.authenticate(username, password) the variables are
initialized as follows:
auditSubjectID = ILogger.UNIDENTIFIED;
attemptedAuditUID = username;
In PKIRealm.authenticate(certs) the variables are initialized as follows:
auditSubjectID = getAuditUserfromCert(certs[0]);
attemptedAuditUID = ILogger.UNIDENTIFIED;
Probably the second one should have been like this:
auditSubjectID = ILogger.UNIDENTIFIED;
attemptedAuditUID = getAuditUserfromCert(certs[0]);
4. The PKIRealm.authenticate(certs) will not be called if there's no
client certificate, so the following check is unnecessary:
if (certs.length == 0) {
logDebug("missing client cert");
}
5. The getAuditUserfromCert() can be simplified as follows:
String certUID = clientCert.getSubjectDN().getName();
return StringUtils.stripToNull(certUID);
See
http://commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/apache...
6. Not sure how important this is, but the simple class name and
method name may not be enough to uniquely identify a method:
auditInfo = clazz.getSimpleName() + "." + method.getName()
To properly identify a method we need to use the full class name, the
method name, and the method signature. Unfortunately there's no
built-in Java method to generate the signature.
Possible solution:
http://www.java2s.com/Code/Java/Reflection/Methodsignature.htm
Another solution using objectweb library:
http://asm.ow2.org/asm33/javadoc/user/org/objectweb/asm/Type.html#getMeth...
Or just leave it as is for now.
7. Let's not use negative variable name:
boolean noAuthzRequired = false;
since it will be more difficult to follow the logic (double negatives
!noAuthzRequired). This would be better:
boolean authzRequired = true; // equivalent to aclMapping != null
8. It's not necessary to check if securityContext is null since it's a
context variable provided by Tomcat:
if (securityContext != null)
principal = securityContext.getUserPrincipal();
9. The catch clause for IOException can be moved up just to surround
the operation that might generate it:
try {
loadProperties();
} catch (IOException e) {
...
}