Loosen the validate_subject_confirmation check?

[sthanson]

Currently your validate_subject_confirmation check in response.rb requires:

1. At least one Subject > SubjectConfirmation element to have a Method of 'urn:oasis:names:tc:SAML:2.0:cm:bearer'
2. At least one SubjectConfirmationData element to exist
3. At least one of these three attributes (InResponseTo, NotOnOrAfter, NotBefore) to be present.
4. All of these three attributes (InResponseTo, NotOnOrAfter, NotBefore) to be valid if present.

I was wondering what people thought of loosening this test a little bit.

The first option is to actually lower the bar the for this test. It seems like the meat of the test involves checking the InResponseTo, NotOnOrAfter, NotBefore attributes (#3 and #4 in my list above) and that #1 and #2 exist to prevent us from checking those attributes on elements where they don't exist. I would propose only failing the test if one of these three InResponseTo, NotOnOrAfter, NotBefore fails their check.... or we expected them to be there based on the structure, but they weren't actually included. But if we don't expect those attributes to be there (such as a different SubjectConfirmation Method or no SubjectConfirmationData element), then the test is simply skipped instead of failed.

I prefer this option. Aside from some important checks like the x509 certificate, I like the idea of a saml parser validating that what has been provided is valid instead of dictating every element and attribute that must be provided (because brittle structures make it harder to on board new saml partners).

The second option for loosening the test would be to add a flag to skip this test. Much like the skip_conditions option skips the validate_conditions test, a skip_subject_confirmation option would skip this test.

You can see an example of my first option in this commit:
sthanson@5bd9230

If you like this approach I'm happy to add tests and turn it into a PR.

[pitbulk]

In the php-saml and the python-saml we have a "strict" parameter that relaxes the SAML validation (useful for testing environments).

I'm ok with the idea of adding a "skip_subject_confirmation" that will skip the whole process of search a valid subjectconfirmation element, adding at the top of the validate_subject_confirmation method:

return true if options[:skip_subject_confirmation]

Can you send a PR with this change and implement tests?

[sthanson]

I created a new pull request with the skip_subject_confirmation flag.

#260

[pitbulk]

Can you add documentation at the SAMLResponse constructor?
https://github.com/sthanson/ruby-saml/blob/skip_validate_subject_confirmation/lib/onelogin/ruby-saml/response.rb#L36

It mentions :skip_conditions and should mention :skip_subject_confirmation

Also maybe is a good moment to add those parameters to the documentation, editing README.md and adding at L143 at the "The Initialization Phase" section, explaining that some validations could be skipped during the SAMLResponse validation, and provide an example.

[sthanson]

Hi @pitbulk

I added documentation to the two files you suggested.

I added the new README.md documentation at L169 instead of L143 because it seemed like that "def saml_settings" code block related to the previous paragraphs and it seemed out of place after my new paragraph. Hope that is ok. If not, I'm happy to move it to L143 like you originally suggested.

I also noticed I had put the wrong units in a previous comment I committed to the app. "allowed_clock_drift" is in seconds, not minutes. So I updated the comments to reflect that. I can re-make my pull request without this change if you prefer.

Thanks!

[sthanson]

I made the text change and I'm rebuilding the PR to get rid of the merge commit.

[sthanson]

Rebuilt PR to get rid of my merge commit:

#261