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

Azure ADFS doesn't sign on top-level but on Assertion level #405

Closed
vandernorth opened this issue Nov 5, 2019 · 4 comments
Closed

Azure ADFS doesn't sign on top-level but on Assertion level #405

vandernorth opened this issue Nov 5, 2019 · 4 comments

Comments

@vandernorth
Copy link
Contributor

Hi,

After a few 'normal' windows server ADFS integrations with passport-saml I tried to implement Azure ADFS today, but something strange seems to happen.

On the callback URL I retrieve an XML in the following form:

<samlp:Response ID="_BBBBB">...</samlp:Status>
	<Assertion ID="_AAAAA">
		<Signature ><SignedInfo><Reference URI="#_AAAAA">...</Reference></SignedInfo></Signature>
	</Assertion>
</samlp:Response>

In SAML.js in the validateSignatureForCert function I see the comment

We expect each signature to contain exactly one reference to the top level of the xml we are validating, so if we see anything else, reject.

Which I understand but doesn't seem to be valid for this SAML response. Should we allow just the assertion to be signed and checked?

I'd like to know if this is expected behavior or that it is something that needs to be fixed (I'm happy to supply a PR if that's the case).

@markstos
Copy link
Contributor

Should we allow just the assertion to be signed and checked?

The authority is the SAML spec. You can find the related documents here: http://saml.xml.org/saml-specifications Find the related section and quote it here. If we can be more spec-compliant, we are interested in a patch.

@vandernorth
Copy link
Contributor Author

Sound good! I've tried to look it up, and I think I've found it but I'm curious if you agree with me.

From https://www.oasis-open.org/committees/download.php/56776/sstc-saml-core-errata-2.0-wd-07.pdf

5.4.2 References
SAML assertions and protocol messages MUST supply a value for the ID attribute on the root element of
the assertion or protocol message being signed. The assertion’s or protocol message's root element may
or may not be the root element of the actual XML document containing the signed assertion or protocol
message (e.g., it might be contained within a SOAP envelope).
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".

Which seems to state that it's fine either way, so you can sign the document-root (Response in my example) or the top-level Assertion (the Assertion with ID="_AAAAA" in my example)

I'm willing to create a patch if you agree with this. If you have any directions on how you would patch it yourself I could bear that in mind.

@vandernorth
Copy link
Contributor Author

Just found out that this library does support the mentioned SAML spec. First you check the top-level signature and if that's not found you do check if the assertion is signed and if that signature is correct.

Guess we're dealing with a configuration or certificate issue of some sort. Yet to figure that out, I'll comment here if I ever resolve this.

I will close this issue.

@vandernorth
Copy link
Contributor Author

Azure seems to have a new certificate they use to sign, but this certificate is not in the federation metadata. Haven't found a solution for that yet.

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

No branches or pull requests

2 participants