spring-attic / spring-security-saml

SAML extension for the Spring Security project
Other
419 stars 479 forks source link

SES-38: WebSSOProfileConsumerImpl fails if there is more than one AudienceRestriction #44

Closed spring-projects-issues closed 14 years ago

spring-projects-issues commented 14 years ago

Phil Varner (Migrated from SES-38) said:

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 }

spring-projects-issues commented 14 years ago

Phil Varner said:

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"); }

spring-projects-issues commented 14 years ago

Vladimir Schäfer said:

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

"Note that multiple 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.

spring-projects-issues commented 14 years ago

Phil Varner said:

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

spring-projects-issues commented 14 years ago

Vladimir Schäfer said:

Yes, that's what I meant.

spring-projects-issues commented 14 years ago

Phil Varner said:

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?