[BUG]: keyInfo usage

[IlyaRazuvaev]

Is your feature request related to a problem? Please describe...

I have just updated to 4 version and confused by validateSignatureValue function.
I have duplicated <KeyInfo> inside SamlRequest and SamlMetadata. Similar to Okta example http://saml.oktadev.com/.
That's mean that loadSignature functions will initialize this.keyInfo by request key, and validateSignatureValue will use it preferable over metadata certificate without any option to choose another behavior.

       // loadSignature 
        const keyInfo = xpath.select1(".//*[local-name(.)='KeyInfo']", signatureNode);
        // TODO: should this just be a single return instead of an array that we always take the first entry of?
        if (xpath.isNodeLike(keyInfo)) {
            this.keyInfo = keyInfo;
        }
       
        // validateSignatureValue 
        const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey;

Describe teh solution you'd like...

Another order of keys.

Describe the alternatives you've considered...

Configurable keyInfo

[LoneRifle]

Thanks for flagging this. This was also picked up by another user and discussed in #399. The user pointed out that there is a viable workaround in this comment.

tldr: consider doing the following to declare getCertFromKeyInfo() a no-op:

new SignedXml(
  {
    // ...
    getCertFromKeyInfo: () => undefined
  }
);

4.x introduced a breaking change where we now adhere to section 3.2.2 of W3C's XML Signature Syntax and Processing (Second Edition)¹, ie, if a certificate is present in the KeyInfo element of an XML document, we will use that for validation.

Footnotes

1. https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-CoreValidation ↩

[cjbarth]

getKeyInfoContent is now a noop by default. This will show in the next semver-major release.

[shunkica]

Thanks for flagging this. This was also picked up by another user and discussed in #399. The user pointed out that there is a viable workaround in this comment.

tldr: consider doing the following to declare getCertFromKeyInfo() a no-op:

new SignedXml(
  {
    // ...
    getCertFromKeyInfo: () => undefined
  }
);

4.x introduced a breaking change where we now adhere to section 3.2.2 of W3C's XML Signature Syntax and Processing (Second Edition)1, ie, if a certificate is present in the KeyInfo element of an XML document, we will use that for validation.

Footnotes

1. https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-CoreValidation [↩](#user-content-fnref-1-6bd00c995b16e7b73ddf894a6d7c52fd)

By that logic you should also resolve the SecurityTokenReference when in wssecurity mode.

https://docs.oasis-open.org/wss/v1.1/wss-v1.1-spec-pr-x509TokenProfile-01.htm

[cjbarth]

By that logic you should also resolve the SecurityTokenReference when in wssecurity mode.

https://docs.oasis-open.org/wss/v1.1/wss-v1.1-spec-pr-x509TokenProfile-01.htm

Feel free to submit a PR.

[srd90]

This issue's description describes problems caused by 4.0.0's policy of trusting by default and without any additional validation of trustworthiness of certificate shipped in keyinfo (due to how default getCertFromKeyInfo function was implemented) even if SignedXML was configured (via publicCert) explicitly to use certificate received via some trusted side-channel (due to precedence order being to prefer output of configured getCertFromKeyInfo function - and default implementation was "trust all" - over publicCert).

Issue of trusting by default any certificate for signature validation from keyinfo was addressed at 6.0.0

Related security advisory:

• GHSA-2xp3-57p7-qf4v

FWIW, my humble opinion is still that precedence order should be: explicitly "pinned" publicCert and if such cert doesn't exist output of user provided getCertFromKeyInfo implementation and to be perfectly clear: It is up to user - who implements function for getCertFromKeyInfo - to make sure that whatever cert it outputs can be trusted for signature validation purposes...just in case someone lands here via search engine and doesn't bother to investigate too deeply: getCertFromKeyInfo function which blindly returns cert from signed document's keyinfo is not proper approach...why? See GHSA-2xp3-57p7-qf4v

Last but not least maybe this issue could be closed because starting from 6.0.0 publicCert is respected (i.e. from issue descriptions point of view: cert from metadata - configured via publicCert - is respected) unless user explicitly configure some user provided function to getCertFromKeyInfo.