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/co...)

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#getMethodD...)

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) { ... }

Thanks, Endi!! pushed to master: commit ccf2eb507471a9f19a1768befadeff404c96635e Christina On 05/12/2015 01:42 PM, Endi Sukma Dewata wrote:

On 5/12/2015 1:01 PM, Christina Fu wrote:

...

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

There is one more mSignedAuditLogger in PKIRealm. Other than that it's ACKed.