Uploaded image for project: 'SX Spring Security Extension'
  1. SX Spring Security Extension
  2. SES-38

WebSSOProfileConsumerImpl fails if there is more than one AudienceRestriction

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Invalid
    • Affects Version/s: saml-1.0.0
    • Fix Version/s: saml-1.0.0.RC1
    • Component/s: saml
    • Labels:
      None

      Description

      In WebSSOProfileConsumerImpl#verifyAssertionConditions, if the local entity is not in the first AudienceRestriction, an exception is thrown. The relevant code is:

      340 audience:
      341 for (AudienceRestriction rest : conditions.getAudienceRestrictions()) {
      342 if (rest.getAudiences().size() == 0)

      { 343 log.debug("No audit audience specified for the assertion"); 344 throw new SAMLException("SAML response is invalid"); 345 }

      346 for (Audience aud : rest.getAudiences()) {
      347 if (context.getLocalEntityId().equals(aud.getAudienceURI()))

      { 348 continue audience; ==> this should return rather than continue. 349 }

      350 }

      ==> If it's not in the first AR, we get here and throw an exception

      351 log.debug("Our entity is not the intended audience of the assertion");
      352 throw new SAMLException("SAML response is not intended for this entity");
      353 }

        Activity

        Hide
        philvarner Phil Varner added a comment -

        Patch:

        Index: saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java
        ===================================================================
        — saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java (revision 74)
        +++ saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java (working copy)
        @@ -337,27 +337,24 @@
        throw new SAMLException("SAML response is not valid");
        }

        • audience:
          +
          for (AudienceRestriction rest : conditions.getAudienceRestrictions()) {
          +
          if (rest.getAudiences().size() == 0) { log.debug("No audit audience specified for the assertion"); throw new SAMLException("SAML response is invalid"); }

          +
          for (Audience aud : rest.getAudiences())

          Unknown macro: { if (context.getLocalEntityId().equals(aud.getAudienceURI())) { - continue audience; + return; // found! } }
        • log.debug("Our entity is not the intended audience of the assertion");
        • throw new SAMLException("SAML response is not intended for this entity");
          }
        • /** ? BUG
        • if (conditions.getConditions().size() > 0) { - log.debug("Assertion contain not understood conditions"); - throw new SAMLException("SAML response is not valid"); - }
        • */
          + log.debug("Our entity " + context.getLocalEntityId() + " is not the intended audience of the assertion");
          + throw new SAMLException("SAML response is not intended for this entity");
          +
          }

        /**

        Show
        philvarner Phil Varner added a comment - Patch: Index: saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java =================================================================== — saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java (revision 74) +++ saml2-core/src/main/java/org/springframework/security/saml/websso/WebSSOProfileConsumerImpl.java (working copy) @@ -337,27 +337,24 @@ throw new SAMLException("SAML response is not valid"); } audience: + for (AudienceRestriction rest : conditions.getAudienceRestrictions()) { + if (rest.getAudiences().size() == 0) { log.debug("No audit audience specified for the assertion"); throw new SAMLException("SAML response is invalid"); } + for (Audience aud : rest.getAudiences()) Unknown macro: { if (context.getLocalEntityId().equals(aud.getAudienceURI())) { - continue audience; + return; // found! } } log.debug("Our entity is not the intended audience of the assertion"); throw new SAMLException("SAML response is not intended for this entity"); } /** ? BUG if (conditions.getConditions().size() > 0) { - log.debug("Assertion contain not understood conditions"); - throw new SAMLException("SAML response is not valid"); - } */ + log.debug("Our entity " + context.getLocalEntityId() + " is not the intended audience of the assertion"); + throw new SAMLException("SAML response is not intended for this entity"); + } /**
        Hide
        vsch Vladimir Schäfer added a comment -

        Hi, I'm just checking this bug and the original implementation seems to be correct. In SAML2-core, 922:

        "Note that multiple <AudienceRestriction> elements MAY be included in a single assertion, and each
        MUST be evaluated independently. The effect of this requirement and the preceding definition is that
        within a given condition, the audiences form a disjunction (an "OR") while multiple conditions form a
        conjunction (an "AND")."

        The original code failed when any of the AudienceRestrictions didn't contain the current SP, as defined.

        Is it possible that your assertions is not formatted in accordance with the specification? If this is the case I'd recommend to override the WebSSOProfileConsumerImpl#verifyAssertionConditions method in your custom implementation to solve the problem.

        Show
        vsch Vladimir Schäfer added a comment - Hi, I'm just checking this bug and the original implementation seems to be correct. In SAML2-core, 922: "Note that multiple <AudienceRestriction> elements MAY be included in a single assertion, and each MUST be evaluated independently. The effect of this requirement and the preceding definition is that within a given condition, the audiences form a disjunction (an "OR") while multiple conditions form a conjunction (an "AND")." The original code failed when any of the AudienceRestrictions didn't contain the current SP, as defined. Is it possible that your assertions is not formatted in accordance with the specification? If this is the case I'd recommend to override the WebSSOProfileConsumerImpl#verifyAssertionConditions method in your custom implementation to solve the problem.
        Hide
        philvarner Phil Varner added a comment -

        Thanks Vladimir. When you say "the original code failed", you mean that it correctly returned an error?

        Show
        philvarner Phil Varner added a comment - Thanks Vladimir. When you say "the original code failed", you mean that it correctly returned an error?
        Hide
        vsch Vladimir Schäfer added a comment -

        Yes, that's what I meant.

        Show
        vsch Vladimir Schäfer added a comment - Yes, that's what I meant.
        Hide
        philvarner Phil Varner added a comment -

        It looks like Siteminder might not implement the spec correctly. I'm working with a client using Siteminder, and the Assertions contain two separate ARs with one correctly containing the entityID and the other with the Siteminder name for the IdP-SP configuration.

        This document on WebEx / Siteminder integration has an example AuthnRequest in the appendix:
        http://developer.webex.com/c/document_library/get_file?folderId=22041&name=DLFE-902.pdf
        which contains two identical ARs, which I assume is because one was generated based on the SP entityID and the other was the Siteminder config name. In the case that these aren't the same, it seems to me it's always a spec violation, since the Siteminder name is never going to have any meaning to the SP.

        This zip has a doc:http://developer.webex.com/c/document_library/get_file?folderId=22041&name=DLFE-1803.zip
        describing the config of Siteminder. It looks like the on page 18 in the "SSO" tab, the "Audience" field is always included as an AR, even if it's the same as the entityID in the SP metadata.

        The IDPM I'm working with told me this is how he always configures it, and that Google Apps and SFDC work with it.

        Maybe there should be a "relaxed" mode by which the semantics of matches at least one AR passes?

        Show
        philvarner Phil Varner added a comment - It looks like Siteminder might not implement the spec correctly. I'm working with a client using Siteminder, and the Assertions contain two separate ARs with one correctly containing the entityID and the other with the Siteminder name for the IdP-SP configuration. This document on WebEx / Siteminder integration has an example AuthnRequest in the appendix: http://developer.webex.com/c/document_library/get_file?folderId=22041&name=DLFE-902.pdf which contains two identical ARs, which I assume is because one was generated based on the SP entityID and the other was the Siteminder config name. In the case that these aren't the same, it seems to me it's always a spec violation, since the Siteminder name is never going to have any meaning to the SP. This zip has a doc: http://developer.webex.com/c/document_library/get_file?folderId=22041&name=DLFE-1803.zip describing the config of Siteminder. It looks like the on page 18 in the "SSO" tab, the "Audience" field is always included as an AR, even if it's the same as the entityID in the SP metadata. The IDPM I'm working with told me this is how he always configures it, and that Google Apps and SFDC work with it. Maybe there should be a "relaxed" mode by which the semantics of matches at least one AR passes?

          People

          • Assignee:
            vsch Vladimir Schäfer
            Reporter:
            philvarner Phil Varner
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development