[jboss-jira] [JBoss JIRA] (SECURITY-659) WebJASPIAuthenticator ignores GroupPrincipalCallback but requires PasswordValidationCallback

WebJASPIAuthenticator ignores GroupPrincipalCallback but requires PasswordValidationCallback -------------------------------------------------------------------------------------------- Key: SECURITY-659 URL: https://issues.jboss.org/browse/SECURITY-659 Project: PicketBox (JBoss Security and Identity Management) Issue Type: Bug Security Level: Public(Everyone can see) Components: PicketBox Affects Versions: PicketBox_v4_0_7 Reporter: arjan tijms Assignee: Anil Saldhana Labels: authentication, jaspi, jaspic In JBoss AS 7.1.1, if a user provided {{ServerAuthModule}} provides a {{GroupPrincipalCallback}}, then this is ignored by {{WebJASPIAuthenticator}}. The provided handler copies the {{GroupPrincipalCallback}}, but the authenticator then does nothing with it. Simulteanously, if the {{ServerAuthModule}} does not provide a {{PasswordValidationCallback}} to the handler, then this will result in a null pointer exception in the authenticator. Regarding the ignored {{GroupPrincipalCallback}}, the problem seems to be in the following code: {code:title=WebJASPIAuthenticator#authenticate} // ... if (result) { PasswordValidationCallback pvc = cbh.getPasswordValidationCallback(); CallerPrincipalCallback cpc = cbh.getCallerPrincipalCallback(); // get the client principal from the callback. Principal clientPrincipal = cpc.getPrincipal(); if (clientPrincipal == null) { clientPrincipal = new SimplePrincipal(cpc.getName()); } // if the client principal is not a jboss generic principal, we need to build one before registering. if (!(clientPrincipal instanceof JBossGenericPrincipal)) clientPrincipal = this.buildJBossPrincipal(clientSubject, clientPrincipal); {code} {{buildJBossPrincipal()}} looks at the "Roles" group in the Subject, but this hasn't been set by either the handler or other code based on what the GroupPrincipalCallback contains. I wonder if changing this into the following would be more correct: {code:title=WebJASPIAuthenticator#authenticate} // ... if (result) { PasswordValidationCallback pvc = cbh.getPasswordValidationCallback(); CallerPrincipalCallback cpc = cbh.getCallerPrincipalCallback(); GroupPrincipalCallback gpc = cbh.getGroupPrincipalCallback(); // ADDED // get the client principal from the callback. Principal clientPrincipal = cpc.getPrincipal(); if (clientPrincipal == null) { clientPrincipal = new SimplePrincipal(cpc.getName()); } // if the client principal is not a jboss generic principal, we need to build one before registering. if (!(clientPrincipal instanceof JBossGenericPrincipal)) clientPrincipal = this.buildJBossPrincipal(clientSubject, clientPrincipal, gpc); // ADDED gpc PARAMETER {code} With {{buildJBossPrincipal()}} implemented as: {code:title=WebJASPIAuthenticator#buildJBossPrincipal} protected Principal buildJBossPrincipal(Subject subject, Principal principal, GroupPrincipalCallback groupPrincipalCallback) { List<String> roles = new ArrayList<String>(); // look for roles in the subject first. for (Principal p : subject.getPrincipals()) { if (p instanceof Group && p.getName().equals("Roles")) { Enumeration<? extends Principal> members = ((Group) p).members(); while (members.hasMoreElements()) roles.add(members.nextElement().getName()); } } // START ADDED if (groupPrincipalCallback != null && groupPrincipalCallback.getGroups() != null) { for (String group : groupPrincipalCallback.getGroups()) { roles.add(group); } } // END ADDED // if the subject didn't contain any roles, look for the roles declared in the deployment descriptor. JBossWebRealm realm = (JBossWebRealm) this.getContainer().getRealm(); Set<String> descriptorRoles = realm.getPrincipalVersusRolesMap().get(principal.getName()); if (roles.isEmpty() && descriptorRoles != null) roles.addAll(descriptorRoles); // build and return the JBossGenericPrincipal. return new JBossGenericPrincipal(realm, principal.getName(), null, roles, principal, null, null, null, subject); } {code} As for the PasswordValidationCallback, WebJASPIAuthenticator now contains the following code in {{authenticate()}}: {code:title=WebJASPIAuthenticator#authenticate} this.register(request, response, clientPrincipal, authMethod, pvc.getUsername(), new String(pvc.getPassword())); {code} The {{register()}} method considers both username and password as optional, but because there's no null check on {{pvc}}, the above line will throw a NPE in case no PasswordValidationCallback is provided. This could perhaps be changed into something like the following: {code:title=WebJASPIAuthenticator#authenticate} this.register(request, response, clientPrincipal, authMethod, pvc != null ? pvc.getUsername() : null, pvc != null ? new String(pvc.getPassword()) : null); {code}