Potentially unsafe default impl for getKeyInfo()

[LoneRifle]

As discussed in #399, and with thanks to @srd90

4.x releases of this package introduced a breaking change, where getKeyInfo() would choose to consider the certificate <KeyInfo /> accompanying a <SignatureValue /> to validate the signature, over SignedXml.publicCert, a certificate identifying the requesting client obtained out-of-band, ahead of time, and known to the application at runtime.

The current behaviour adheres to spec¹²: notably,

questions of trust of such key information (e.g., its authenticity or strength) are out of scope of this specification [and presumably, compliant implementations] and left to the application²

That said, this default behaviour is potentially unsafe: a user of this package may neglect to test <KeyInfo /> for trustworthiness. It is also a change from 3.x behaviour, which ignored all <KeyInfo /> elements.

To date, we have held back on providing a safer default, since this package is undergoing significant change and has to remain largely stable to support the node-saml package, particularly given the time constraints the team faces, consisting purely of volunteers.

This issue hence proposes to change the default behaviour for getKeyInfo() to:

• move the current impl to an opt-in, and;
• provide an impl that either ignores <KeyInfo />, or checks it against a known whitelist such as SignedXml.publicCert.

This issue also incorporates @cjbarth's proposal for a convenience method to check <KeyInfo/> against SignedXml.publicCert or other whitelist.

Given that this is a breaking change, this should be addressed in a major release

Footnotes

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

2. https://www.w3.org/TR/2008/REC-xmldsig-core-20080610/#sec-KeyInfo ↩ ↩²

[cjbarth]

I've added a comment in #401 to clear up the README in this case. I'm very much in favor of keeping to the spec, so having a clear README about how to handle the case where you'd like to deviate from the spec I think is the more appropriate action. This course also means that we won't have to take on a breaking change. Even adding a convenience method wouldn't be a breaking change as that would just extend the API surface.

I was even thinking about this convenience method and there really isn't a need for it since getCertFromKeyInfo() is publicly available, they can just call it themselves and perform whatever checks, including certificate revocation checks, they want. We can document that better if you think needed. Otherwise, I'm happy the way things are.

[LoneRifle]

@cjbarth - if we don't plan to act on this for now, I'll move this to discussions

[srd90]

Title of #403 and #404 is incorrect. #399 was about default implementation of getCertFromKeyInfo. Not about default implementation of getKeyInfo.

---

Acute concern of #399 was addressed at

• Set getCertFromKeyInfo to noop #445

At the end of the day problem was also addressed with security advisory: GHSA-2xp3-57p7-qf4v