Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #322: allow SAML responses without Conditions #323

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThePetrov
Copy link
Contributor

The Conditions element is technically optional, so for maximum compatibility I don't think it should be required by the library. On the other hand I can't deny the security benefits brought by requiring that element to exist. To get the best of both worlds I've added a new configuration setting wantConditionsPresent and retained the default behavior of requiring the Conditions element for a response to be considered valid. If someone actually needs to validate responses without the Conditions element they can set that property to false.

- the conditions element is optional according to the spec
- require Conditions element to be present by default
- added new configuration option to allow lack of Conditions
@mauromol
Copy link
Contributor

mauromol commented Apr 2, 2021

Please note, however, that the SAML 2.0 specification says:

  • section 3.4.1.4 of the Core specification, describing the processing rules of the Authentication Request Protocol:

The resulting assertion(s) MUST contain a <saml:AudienceRestriction> element
referencing the requester as an acceptable relying party. Other audiences MAY be included as
deemed appropriate by the identity provider.

  • section 4.1.4.2 of the Profiles specification, describing the Response usage in the Web SSO Profile:

The assertion(s) containing a bearer subject confirmation MUST contain an
<AudienceRestriction> including the service provider's unique identifier as an <Audience>

So, since the <AudienceRestriction> element appears in the <Conditions> block and java-saml scope is the Web SSO Profile, IMHO enforcing that a <Conditions> element is present is correct.

What I would argue, instead, is why java-saml requires just one <Conditions> element at most to be present: the Profiles specification says in fact:

Other conditions (and other <Audience> elements) MAY be included as requested by the service
provider or at the discretion of the identity provider. (Of course, all such conditions MUST be
understood by and accepted by the service provider in order for the assertion to be considered valid.)

@ThePetrov
Copy link
Contributor Author

ThePetrov commented Apr 6, 2021

That's very insightful, thanks for the reply @mauromol ! You're correct, I wasn't aware that the Conditions were enforced in the profiles part of the spec.

You're correct about multiple Conditions being a possibility, besides the specification this is also allowed in the DTD. I wonder what's the maintainer's take on the subject.
EDIT: I was wrong actually, the DTD doesn't declare a maxOccurs attribute on the Conditions element (source) and the default value of maxOccurs is 1. Which means that there can be either no Conditions element in an Assertion or exactly once. Given that the specification mentions conditions in a lower case I assume it refers to conditions declared within a Conditions element rather than multiple Conditions elements, especially considering that contents of a Conditions element don't have an upper bound on the amount and that the semantics of multiple audience restrictions are discussed in section 2.5.1.4 of the core specification.

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

@ThePetrov
Copy link
Contributor Author

@pitbulk Do you think this is a change worth pursuing?

@mauromol
Copy link
Contributor

EDIT: I was wrong actually, the DTD doesn't declare a maxOccurs attribute on the Conditions element (source) and the default value of maxOccurs is 1. Which means that there can be either no Conditions element in an Assertion or exactly once. Given that the specification mentions conditions in a lower case I assume it refers to conditions declared within a Conditions element rather than multiple Conditions elements, especially considering that contents of a Conditions element don't have an upper bound on the amount and that the semantics of multiple audience restrictions are discussed in section 2.5.1.4 of the core specification.

You are right: it's not the <Conditions> element which might appear multiple times, but rather the existing one, if any, may contain multiple <Condition> and <AudienceRestriction> subelements.
However, the requirement imposed by the profile specification with regards to the need of an <AudienceRestriction> still stands.
So, indeed, the code in com.onelogin.saml2.authn.SamlResponse.checkOneCondition() is partially useless: the actual check that should be made is that at least one <AudienceRestriction> exists and that at least one matches the SP entity id. com.onelogin.saml2.authn.SamlResponse.validateAudiences() should be doing that but indeed fails because it accepts the lack of any <AudienceRestriction>.

I'm going to open an issue for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants