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

[Christina Fu]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pki-cfu-0061-Ticket-1160-audit-logging-needed-REST-API-auth-authz.patch
Type: text/x-patch
Size: 24121 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20150512/56eef986/attachment.bin>