[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]