mock-saml icon indicating copy to clipboard operation
mock-saml copied to clipboard

"Error: Missing signature" result seems to not conform to saml-bindings-2.0

Open itzg opened this issue 2 years ago • 7 comments

When my SP makes this authn redirect request (line breaks added for clarity):

https://mocksaml.com/api/saml/sso?
SAMLRequest=nJJBj9MwEIX%2FiuV7YieUJbE2kcpWiEoLVNvCgdvEmWwtYjt4JsDy61GbRSqXCu3R9nwz73neLYEfJ7Oe%2BRge8PuMxOKXHwOZ00Mj5xRMBHJkAngkw9bs1x%2FuTZlrA0SY2MUgL5DpOjOlyNHGUYrtppGuz2pti27o6je1fm2Hqi46WNXVCiscVjd9NUDRwQAFSvEFE7kYGlnmWoot0YzbQAyBG1nq8lWmy6woDsWNKVem1Hmhy69SbJDYBeAzeWSeyCjlo%2F12Epvb6BVMTp0OiihKsf5r6i4Gmj2mPaYfzuLnh%2FuFN0qN0cJ4jMS549%2BPuUdT6UovTcCSFLtnl29d6F14vP4l3VJE5v3hsMt2n%2FYH2Z7XYs4ek3gXkwe%2B3uR04%2FpsOJcaDOz4Sbb%2FodcjQw8Mt%2BpiZPsci4%2FgcbvZxdHZpxfI4ASBHAaWYj2O8eddQmBsJKcZpWqXkf%2BGr%2F0TAAD%2F%2Fw%3D%3D

&SigAlg=http%3A%2F%2Fwww.w3.org%2F2000%2F09%2Fxmldsig%23rsa-sha1

&Signature=auEkgD83piMUaxm%2BetHGmxDHpQV9b3t9CLxriYGdmklSH5ZH8aWku4zeFz8sEtZoAA6JiXLkEAIKbEQfee%2BsX70g%2FhVjPC9w%2BVTGjBbbJd98CEgtSvDWMB9AsfEtPw59kO5mux%2BcSuAXyfRberO96vcjF4X5WF27wA7A7qDT6RwkzK7V%2BQ0%2FesVDu1AGJkXNUJZv9EjZOtEnOymlPgLufpAlD5dPnR99Ktf3G1bJT7KDWi9V1TTizq5xr6rA5%2BocVnHEZN7ZPiCcGZfgtbjCJ0ZIkMpGG6ciZoPW00w00fXPRcdB%2BGIJuQT%2BXbiHYhzMHl3y7UMAZ7FVgkaRqmzJ%2BQ%3D%3D

Then the response page only shows ""Error: Missing signature".

FWIW I identified this section of code as the origin of that response

https://github.com/boxyhq/mock-saml/blob/b6f2e89ff6e9663e6b289e11c69722be2cbf37d7/utils/request.ts#L34-L40

From the saml-bindings-2.0 specification, section 3.4.4, it states

A query string parameter named SAMLEncoding is reserved to identify the encoding mechanism used. If this parameter is omitted, then the value is assumed to be urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE

As such, DEFLATE is the mechanism in play, which then is discussed in section 3.4.4.1. Item 1 of XML serialization states:

Any signature on the SAML protocol message, including the ds:Signature XML element itself, MUST be removed

which is what the SAML authentication library that I am uses does. That seems to be a mismatch with the expectation of the code referenced above, but I might be missing some broader context of the code.

Further in 3.4.4.1 the block that starts:

If the underlying SAML protocol message is signed with an XML signature [XMLSig], the URL-encoded form of the message MUST be signed as follows:

You'll note that the URL I attempted, shown above, includes SigAlg and Signature, but they don't seem to be considered by the request processing.

itzg avatar Feb 11 '23 17:02 itzg

@itzg Thank you for reporting the error. Will take a look into this asap.

niwsa avatar Feb 11 '23 18:02 niwsa

Thanks! But really, no rush at all. I was just investigating a scenario reported at my https://github.com/itzg/saml-auth-proxy.

itzg avatar Feb 11 '23 18:02 itzg

@itzg Thanks for reporting this, we hadn't looked at the implementation of this part of the RFC. We'll report back as soon as we have had a chance to support this spec.

deepakprabhakara avatar Feb 16 '23 13:02 deepakprabhakara

@itzg Interestingly there is no public certificate anywhere in the request so it looks like that has to be exchanged in some form earlier to sending the request. Trying to check what the RFC says about this but if you have an idea of how that is supposed to work then please let me know. For now I could temporarily let the request bypass the validation check.

deepakprabhakara avatar Mar 24 '23 18:03 deepakprabhakara

As far I know, it is standard practice to upload the SP's metadata to the IdP prior to authorizations, such as

https://samltest.id/start-sp-test/

If the public key were transferred with every request it seems like that would defeat the chain of trust.

I haven't looked that up in the spec yet. Don't worry about making any special changes to the verification. I'm not actively needing to test SAML at this time.

itzg avatar Mar 24 '23 18:03 itzg

@itzg That makes sense, we wanted to be as stateless as possible. Public key transfer with every request is not a problem with the chain of trust in itself since it is contained inside the SAML request in a POST request. We'll have a think about tackling this case.

deepakprabhakara avatar Mar 24 '23 18:03 deepakprabhakara

Pushed a workaround for now - https://github.com/boxyhq/mock-saml/pull/159

deepakprabhakara avatar Mar 24 '23 23:03 deepakprabhakara