Repository navigation
Potentially unsafe default impl for getKeyInfo() #403
Description
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 spec12: 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 application2
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 asSignedXml.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
Activity
cjbarth commentedon Oct 10, 2023
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 commentedon Oct 11, 2023
@cjbarth - if we don't plan to act on this for now, I'll move this to discussions