Please review. This patch address the missing REST API auth/authz auditing part of the ticket https://fedorahosted.org/pki/ticket/1160 The kra for getKeyInfo will come as a separate patch after this. here are sample signed audit log messages resulted from my test cases: pki -d . -c netscape -h kraHost -p 28443 -P https -n "PKI Administrator for kraHost" key-find --maxResults -5 == case when running the above request as a kraadmin with valid cert == 0.http-bio-28443-exec-1 - [07/May/2015:14:30:26 EDT] [14] [6] [AuditEvent=AUTH_SUCCESS][SubjectID=kraadmin][Outcome=Success][AuthMgr=certUserDBAuthMgr] authentication success 0.http-bio-28443-exec-1 - [07/May/2015:14:30:27 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=kraadmin][Outcome=Success][aclResource=certServer.kra.account][Op=login][Info=AccountResource.login] authorization success 0.http-bio-28443-exec-2 - [07/May/2015:14:30:27 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=kraadmin][Outcome=Success][aclResource=null][Op=null][Info=ACL mapping not found; OK:SystemCertResource.getTransportCert] authorization success 0.http-bio-28443-exec-3 - [07/May/2015:14:30:28 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=kraadmin][Outcome=Success][aclResource=certServer.kra.keys][Op=execute][Info=KeyResource.listKeys] authorization success 0.http-bio-28443-exec-4 - [07/May/2015:14:30:28 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=kraadmin][Outcome=Success][aclResource=certServer.kra.account][Op=logout][Info=AccountResource.logout] authorization success == case when running the above request as a caadmin with ca admin cert == 0.http-bio-28443-exec-6 - [07/May/2015:14:31:24 EDT] [14] [6] [AuditEvent=AUTH_FAIL][SubjectID=CN=PKI Administrator, EMAILADDRESS=caadmin(a)idm.lab.bos.redhat.com, O=idm.lab.bos.redhat.com Security Domain][Outcome=Failure][AuthMgr=certUserDBAuthMgr][AttemptedCred=$Unidentified$] authentication failure == case when creating a caadmin in the kra user db but not given any group privilege == 0.http-bio-28443-exec-18 - [07/May/2015:14:48:31 EDT] [14] [6] [AuditEvent=AUTH_SUCCESS][SubjectID=caadmin][Outcome=Success][AuthMgr=certUserDBAuthMgr] authentication success 0.http-bio-28443-exec-18 - [07/May/2015:14:48:31 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=caadmin][Outcome=Success][aclResource=certServer.kra.account][Op=login][Info=AccountResource.login] authorization success 0.http-bio-28443-exec-19 - [07/May/2015:14:48:31 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=caadmin][Outcome=Success][aclResource=null][Op=null][Info=ACL mapping not found; OK:SystemCertResource.getTransportCert] authorization success 0.http-bio-28443-exec-2 - [07/May/2015:14:48:32 EDT] [14] [6] [AuditEvent=AUTHZ_FAIL][SubjectID=caadmin][Outcome=Failure][aclResource=certServer.kra.keys][Op=execute][Info=Authorization Error] authorization failure 0.http-bio-28443-exec-3 - [07/May/2015:14:48:32 EDT] [14] [6] [AuditEvent=AUTHZ_SUCCESS][SubjectID=caadmin][Outcome=Success][aclResource=certServer.kra.account][Op=logout][Info=AccountResource.logout] authorization success thanks, Christina _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

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

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.