Conditions and AudienceRestriction validation

[mauromol]

An in-depth discussion is at #323.
This is somewhat related to #322, but it's more targeted at SAML specification compliancy.

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 within <Conditions>, although the <Conditions> element is optional in the schema, it should be present BECAUSE it should contain AT LEAST one AudienceRestriction matching the SP entity id.

What java-saml is doing right now is:

• in com.onelogin.saml2.authn.SamlResponse.checkOneCondition() it checks that there is exactly one <Conditions> element; the javadoc says "checks that the samlp:Response/saml:Assertion/saml:Conditions element exists and is unique", but the latter part is useless because the schema already enforces that at most one such element is present; so, since that check is made after performing schema validation (at least if schema validation is enabled...), the check is partially useless
• com.onelogin.saml2.authn.SamlResponse.validateAudiences() checks that, if any <AudienceRestriction> element exists, at least one of them is equal to the SP entity id

This said, considering what the specification requires, I think that the above two methods could be changed like this:

• in com.onelogin.saml2.authn.SamlResponse.validateAudiences() make validation fail if validAudiences is empty
• possibly remove com.onelogin.saml2.authn.SamlResponse.checkOneCondition(), because it becomes redundant by the above check, especially if it's moved at the same position of the checkOneCondition() call in com.onelogin.saml2.authn.SamlResponse.isValid(String)

What do you think?

[pitbulk]

We could agree on making the AudienceRestriction element required, but that gonna be a behavior change, so maybe we need to control it with a security setting

checkOneCondition is covered by the schema validator, but I don't see such extra check something bad.

[pitbulk]

@mauromol Do you have a PR for this one?

[mauromol]

No, because I still have some doubts. First of all, is a dedicated security setting for this really worth? Considering that:

• the current behaviour does not adhere to specification; if we maintain it as the default one, it means that backward compatibility is preferred even when the specification is violated
• the current checks on conditions and audiences are already performed only if "isStrict" is set to true in settings: do we really need to protect this behind a second level of strictness?

So, I would be inclined to implement this without any further "flag" to enable it. Or, on the contrary, to add a flag to disable it to explicitly preserve backward compatibility (or to go even further, as requested in #322: what do you think about it?), however I was wondering whether it's worth the effort.

With regards to checkOneCondition: yes, it does not hurt, but then I'm a bit lost on the policy: in #334 (comment) you substantially say that schema validation is enough, but here you'd prefer to maintain the redundant check: so what should be the way? :-)

[mauromol]

@pitbulk what do you then think about this? I can add the additional check to AudienceRestriction element, but I need to know if you wish it to be an opt-in or an opt-out (when "isStrict" is set in both cases).
WRT checkOneCondition, again I'm open to both decisions: remove the (redundant and useless) check or leave it there (for whatever reason).