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

275 - Validates documents with signed message and assertion #277

Merged
merged 2 commits into from Nov 10, 2015

Conversation

levelboy
Copy link
Contributor

Status

READY

Migrations

NO

Description

see #275
The modification is that when we check the signature of the response, we only check the first Reference object for that signature. According to the SAML standard, a signature node can only have one Reference node within.

Related PRs

Problem seems to be introduced here: 275512a

Todos

  • Refactor xml_security to use only one XML library. At the moment it uses both nokogiri and REXML
  • This only checks the signature on the document, as it was doing previously. It would be best and correct according to the SAML standard to verify each signatures in the document.

Deploy Notes

No deploy notes.

Steps to Test or Reproduce

First commit adds relevant response document and failing test. Run rake test to see it failing.

…nature. As per the SAML specification, a signature can only have one reference node. (5.4.2 References)
@levelboy
Copy link
Contributor Author

levelboy commented Nov 5, 2015

@pitbulk Do you have any notes on this?
Thank you.

@pitbulk
Copy link
Collaborator

pitbulk commented Nov 5, 2015

@levelboy Sorry, I had no time to review it yet.

@pitbulk
Copy link
Collaborator

pitbulk commented Nov 10, 2015

Hi @levelboy,

Thanks you very much for the PR, it seems that fixs the bug introduced at 1.1.0

I read again the SAML standard and you are right, SAML expects only 1 reference

5.4.2 References
....
Signatures MUST contain a single <ds:Reference> containing a same-document reference to the ID
attribute value of the root element of the assertion or protocol message being signed. For example, if the ID attribute value is "foo", then the URI attribute in the <ds:Reference> element MUST be "#foo".

So we will apply your patch.

P.S We followed xmlsec lib steps that loop over all the references, but is true we don't need to do that on SAML:
https://github.com/robrichards/xmlseclibs/blob/1aaa0c20c9967d29a8cccdf81ad3994244c10d8d/src/XMLSecurityDSig.php#L572

@pitbulk pitbulk merged commit 9bb26bd into SAML-Toolkits:master Nov 10, 2015
@levelboy
Copy link
Contributor Author

Great, thanks @pitbulk

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.

None yet

3 participants