Add explicit IdP signing key feedback

[pablothedude]

An explicit IdP signing key check is added for easier debugging.

The ease of debugging should outweigh the drawbacks of moving the responsibility of the SAML2 library also to EB and the performance penalties for doing this twice.
#1328

[thijskh]

This always checks only the first certificate. Is it certain that there never will be > 1 certificate?

[pablothedude]

Hi Thijs, that was something I was wondering too. Because it seemed to me that a response would only be signed by one key. And therefore I wanted to ask @baszoetekouw if and when this would happen. But you beat me to it.

[pmeulen]

The XML signature standard does not limit the number of KeyInfo or X509Data elements in a Signature. So there could be any number, including none according to XML Signature.

The purpose of a KeyInfo is to specify what key was used to sign, an thus what key must be used to verify the signature: "KeyInfo indicates the key to be used to validate the signature." In my opinion it does not make sense to have keyInfo pointing to multiple keys.

This is how the Stepup SAML Bundle handles this: https://github.com/OpenConext/Stepup-saml-bundle/blob/main/src/SigningSignatureVerifier.php#L40

I think it is unlikely that an IdP or SP would be putting more than one KeyInfo with an X509Data element, or more than X509Data elements in a KeyInfo in a Signature. I've never seen this happen, but I cannot rule out that someone out there is doing this.

[baszoetekouw]

@pmeulen your link is broken, unfortunately? Were you referring to https://github.com/OpenConext/Stepup-saml-bundle/blob/main/src/Signing/SignatureVerifier.php ? The flow there is simpler than what we propose here. Afaics, Stepup simply tries to verify the signature with all trusted keys; it doesn't seem to differentiate between the wrong key being specified in the assertion (i.e., unknown key in <KeyInfo>) or any other type of signature validation failing.

Anyway, I have been trying to track down in the SAML2 code where the signing certificates are added, but I'm having trouble finding it. It's not clear for example which <ds:Signature>s exactly are considered.
The SAML spec at least says that multiple elements in the Response can be signed, and that the signature then applies to all descendent element, so that suggests that it considers (for example) both the Response signature and the Assertion signature.

So it's probably best to iterate over all certificates returned by $sspResponse->getCertificates(). The question remains of course, of whether to raise an error if all of them are unknown, or if any of them is unknown. The latter sounds safer (and might defend against wrapping attacks even), but might cause failures for SAML Reponses from obscure IdPs.
@thijskh @pmeulen what do you think?

[baszoetekouw]

I think we should err of the safe side, and check all of the signatures. If even one of them is unknown, reject the message.
Any normal message should pass this test; an IdP that does something strange like signing a message with two different keys should be investigated anyway.

[pmeulen]

Yes I meant https://github.com/OpenConext/Stepup-saml-bundle/blob/main/src/Signing/SignatureVerifier.php#L40

[pmeulen]

I'm a fan of being strict.

• Only allow Signatures element in normal locations (on Assertion, on SAMLRequest)
• Only one SignatureValue per Signature
• Only one KeyInfo of the X509Data per SignatureValue

If we're think this breaks things, we make these checks log a warning at first, and run this in production for a wile. Then we investigate any warnings that we see, and decide what to do.

But I don't expect this to be an issue. See e.g. the checks that the (new) SSP library does:

https://github.com/simplesamlphp/xml-security/blob/feb95e14e9c9941681ac6d2fc5880691b61afff7/src/XML/ds/Signature.php#L114-L148

Also earlier drafts of the XML-DSIG spec had these restrictions, this was later changed to unrestricted.