[Pki-devel] [PATCH] pki-cfu-0058-Ticket-1160-audit-logging-needed-REST-API-auth-authz.patch

[Endi Sukma Dewata]

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/commons/lang/StringUtils.html#stripToNull(java.lang.String)

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#getMethodDescriptor(java.lang.reflect.Method)

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

-- 
Endi S. Dewata