6 Replies Latest reply on May 22, 2013 5:31 PM by anil.saldhana

    Supporting List Attributes through SAML 2.0 in PicketLink

    tim.kutz

      We have been developing several applications which are all authenticated using PicketLink and SAML2.0.  In the simplest case, authentication works, and attributes are published.  However, when a user has a list of values in an attribute, only one value of the list is published.  After a great deal of effort stepping through and debugging the sequence, I believe I have tracked down each of the places that need to be changed in order to publish these attributes correctly.  If the changes were confined to extension points alone, I would implement them myself, and move ahead.  Unfortunately, they are not.  Two of the places that need changed can be replaced as part of configuration, but the third involves the createAttributeStatement() method of the StatementUtil class.

       

      Here is what I am seeing.

       

      First, in org.jboss.security.mapping.providers.attribute.LdapAttributeMappingProvider, where the attribute values are retrieved from our LDAP server, the attributes are assumed to be single valued.  The code in question, lines 238-256 of the version I'm currently looking at, is as follows:

       

       

                     while (results.hasMore())
                     {
                        sr = (SearchResult) results.next(); 
                        Attributes attributes = sr.getAttributes();
                        NamingEnumeration<? extends javax.naming.directory.Attribute> ne = attributes.getAll();
      
                        while(ne != null && ne.hasMoreElements())
                        {
                           javax.naming.directory.Attribute ldapAtt = ne.next();
                           if("mail".equalsIgnoreCase(ldapAtt.getID()))
                           {
                              attributeList.add(AttributeFactory.createEmailAddress((String) ldapAtt.get()));   
                           }
                           else
                              attributeList.add(AttributeFactory.createAttribute(ldapAtt.getID(), 
                                    (String)ldapAtt.get())); 
                        } 
                     }       
      
      

       

      The line highlighted in red, as you can see, created an attribute with a single value (ldapAtt.get()).  For a multivalued attribute, this should be iterating through multiple values.  I'm unclear as to whether it should be creating multiple attributes, one for each value, all with the same name, or if it should be producing one attribute at this point, with multiple values.  Either could work, but would need to be handled correctly in the AttributeManager implementation.  As the Attributes being created here are parameterized types, I'm unsure if an Attribute<List<String>> is appropriate or not.  More on this below, as it affects the implementation of AttributeManager.

       

      In the AttributeManager, the list of Attributes generated above is converted into a map.  If multiple values are treated as separate attributes in the list, then all but one will be lost, as they are added to the list, since they have the same key.  Althought his would appear at first to point towards using Attribute<List<String>> to support these, there is an added twist.  Multiple MappingProviders can be configured, and when this is the case, they each add their attributes to the same list.  Thus, if there are attribute name collisions across the sources, values may be lost.  It seems more appropriate that the AttributeManager merges the multiple entries as it creates the Map of values.  I'm still not entirely sure of this, though, as it may have impact on other downstream code that I'm not familiar with. 

       

      Regardless of the two fixes above, multiple value still will not be published into the SAML.  The final change is in the createAttributeStatement(Map<String,Object>) method of org.picketlink.identity.federation.core.saml.v2.util.StatementUtil.  This method iterates through the map of attributes generated by the AttributeManager, and produces the Document objects for the Attribute section.  It assumes that the attribute values are single valued.  Interestingly, there is a special section of the code which handles Roles, but instead of adding multiple values of the attribute to the Attribute, it adds an additional Attribute for each role.  This actually produces incorrect SAML output.  Where it should produce:

       

       

      <saml:AttributeStatement>
          <saml:Attribute Name="Role">
              <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" 
                                              xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
                                              xsi:type="xs:string">CPDEV002</saml:AttributeValue>
               <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" 
                                               xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
                                               xsi:type="xs:string">User</saml:AttributeValue>
          </saml:Attribute>
      </saml:AttributeStatement>
      
      

       

      We instead get:

       

      <saml:AttributeStatement>
          <saml:Attribute Name="Role">
              <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" 
                                              xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
                                              xsi:type="xs:string">CPDEV002</saml:AttributeValue>
          </saml:Attribute>
          <saml:Attribute Name="Role">
               <saml:AttributeValue xmlns:xs="http://www.w3.org/2001/XMLSchema" 
                                               xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
                                               xsi:type="xs:string">User</saml:AttributeValue>
          </saml:Attribute>
      </saml:AttributeStatement>
      

       

      I've added the following Enhancement request in the issue tracker for this, as well.

       

      https://issues.jboss.org/browse/PLFED-386

       

      Although I'm happy to work on making these changes, I do need to make sure I take the correct approach, especially with regards to the Attribute<List<String>> vs multiple Attribute<String> question, and when/how to merge the multiple attributes of the same name.

        • 1. Re: Supporting List Attributes through SAML 2.0 in PicketLink
          tim.kutz

          As of this morning, I have what appears to be a working solution to this.  We are still testing, but initial results are positive.

           

          I made changes in three key places, and was able to plug them all in through configuration hooks, rather than updating a private branch of PicketLink itself. 

           

          The first change, as described in my previous post, is in the MappingProvider.  I extended the LdapAttributeMappingProvider as LdapExtAttributeMappingProvider, and modified the code section highlighted above to capture multiple values of the same attribute as separate instances.  That is, if an attribute named "children" has 3 values, "Mike", "Nancy", and "Joe", then three Attributes are added to the ArrayList being built, one with {name="children", value="Mike"}, one with {name="children", value="Nancy"}, and one with {name="children", value="Joe"}.  This was fairly straightforward, using the Naming api.  This change can be made without making the other two.  It will not fix the issue, but it will not break any downstream code.

           

          The second change was to create an AttributeManager.  This MultiValueAttributeManager was based directly on the JBossAppServerAttributeManager.  The change in this class was to make it aware of possible name collisions within the list of attributes returned from the MappingContext.  Note that, under the current implementation, multiple MappingProviders can be configured, and the Attribute lists returned are simply concatenated.  Merging values with name collisions not only handles multiple valued attributes, it should successfully merge name collisions that occur across separate MappingProviders.  In the process of doing this, it converts all Attribute<String> instances to Attribute<List<String>> instances.  When a name collision is found, it adds the additional value to the list, rather than creating a new Attribute.  This change, once made, will cause all authentications to fail, without the third change being made as well.

           

          The third change is actually made within the StatementUtil helper class.  In order to make this pluggable, I copied and modified the only SAML handler that uses it, SAML2AuthenticationHandler, and renamed it to ExtSAML2AuthenticationHandler.  The only actual change to the Handler itself, is to replace StatementUtil with ExtStatemementUtil for the createAttributeStatement() static method calls.  This method was changed, to expect a list of values, and to put all members of the list into the value list of the actual SAMLAttribute object for marshalling.  Note that, in the process, I also changed the generation of the Roles attribute to correctly put multiple values into a single attribute.

           

          I am attaching our code to the Jira ticket above, so it can be merged into the mainline.  If I can be of any help in moving that process forward, I'd be happy to do so.  We'd much prefer to be using core code, rather than our own patches.

          • 2. Re: Supporting List Attributes through SAML 2.0 in PicketLink
            anil.saldhana

            Tim, I should be able to figure out the fixes with your problem statement.

            • 3. Re: Supporting List Attributes through SAML 2.0 in PicketLink
              tim.kutz

              If you need any further information, please don't hesitate to let me know.

               

              It would be our preference to use an updated version of PicketLink, rather than our updates, but I'm not certain our timelines will allow this.  If there's any way you could provide some indication of how long it would take to integrate these fixes into your mainline, and issue a tested release, so we can make a decision and plan accordingly, it would be greatly appreciated.

              • 4. Re: Supporting List Attributes through SAML 2.0 in PicketLink
                anil.saldhana

                On further investigation, Tim,  I am unsure if there is any major issue here.  An attribute can have 0 or more attribute values.  We have been using one role value for an attribute called role. 

                 

                Look at AssertionUtil class in PL which can determine roles in an assertion.  I suggest you use that class to find out the roles in the assertion. 

                 

                I understand that you want to have one attribute called "role" and multiple attribute values.  But our SAML generation does not take this into consideration. I am kind of reluctant to make the change. Best is for the role processing logic to go through all the attributes and get values for all attributes called "role".  This is what the https://docs.jboss.org/picketlink/2/apidocs/org/picketlink/identity/federation/core/saml/v2/util/AssertionUtil.html getRoles() method does.

                • 5. Re: Supporting List Attributes through SAML 2.0 in PicketLink
                  tim.kutz

                  From the SAML side, yes, an attribute can have multiple values.  However, from the PicketLink side, any attribute other than Role which has multiple values only has one value of the list published.  As an example, create a multi-valued attribute in an LDAP repository, and place 2 or more values into it, within LDAP.  You will see that only one of the values makes it into the SAML, without the fixes above.  There is special code to handle this case for Roles, but it ignores all other attributes.  I need it for the other attributes, as well.

                   

                  With regards to multiple elements with the same attribute name, while your role handling code works, a non-PicketLink SP would have to do extra work to identify the roles, as the common generated routine to parse attributes from the XML is to place them into a Map/Dictionary type structure, where subsequent attributes would remove previous entries of the same name.  This is exactly what was happening with one of our non-Java SP's, without this fix.  It's not just what I want, it's the way the schema is defined for SAML.

                   

                  The important point, here, though, is that this fix applies to more than just roles.  It applies to all multi-valued attributes that are intended to be published as SAML Attributes.

                  • 6. Re: Supporting List Attributes through SAML 2.0 in PicketLink
                    anil.saldhana

                    Tim - here is what I am planning.

                     

                    We are close to doing a 2.1.7 release (within 7 days). This particular issue is far reaching which touches many areas, implying greater chances of regression for the developers.

                     

                    So I am breaking your use case into the following tasks:

                    a) Changes to SAML2AuthenticationHandler (and StatementUtil) to create a Role attribute with multiple values.  (https://issues.jboss.org/browse/PLINK2-83).  This is done for 2.1.7  [Changes: https://github.com/anilsaldhana/federation/commit/4e82a5c7dc40fefb487b2c53d6ae0dff646c4116)

                    b) Changes to PicketBox LdapAttributeMappingProvider.  This needs to be considered separately by PicketBox. 

                    c) Changes to JBossAppServerAttributeManager.  I have not done this for 2.1.7

                     

                    I would suggest that you continue to use your version of mapping provider and the attribute manager.

                     

                    We have been quite busy with PicketLink 2.5.  Hence we could not get to your JIRA issue in time.