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

[Anil Saldhana (JIRA)]

     [ https://issues.jboss.org/browse/SECURITY-659?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Anil Saldhana reassigned SECURITY-659:
--------------------------------------

    Assignee: Stefan Guilhen  (was: Anil Saldhana)

    
> 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: Stefan Guilhen
>              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}

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.jboss.org/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira