Update README.md to reflect getCertFromKeyInfo changes

[dozgunyal]

Changes in #445 was not reflected in the documentation.
This PR attempts to fix it.

Summary by CodeRabbit

• Documentation
  • Clarified and updated defaults for two public options that control key-info content inclusion and certificate extraction—these defaults were swapped. Notes explain the new default behaviors, their impact on signature output and certificate handling, and how to override them for alternate interoperability or output needs.

[cjbarth]

@dozgunyal , would you be open to changing your PR to remove the SignedXml.getCertFromKeyInfo stuff and adjust the default property to point to our implementation of noop instead as per the discussion?

[cjbarth]

@dozgunyal , are you willing to make the adjustments?

[coderabbitai]

Walkthrough

Swapped defaults for two SignedXml options in documentation: getKeyInfoContent now defaults to SignedXml.getKeyInfoContent; getCertFromKeyInfo now defaults to noop. No code behavior or API signatures were changed.

Changes

Cohort / File(s)	Change Summary
Documentation update README.md	Updated README to swap default option values: getKeyInfoContent default set to SignedXml.getKeyInfoContent; getCertFromKeyInfo default set to noop.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

• Files to spot-check: README.md (verify wording and defaults)

Poem

🐇 I swapped two defaults with a twitch and a grin,
One hums its name, the other stays thin.
In README's garden the change is complete,
A small tidy hop, concise and neat.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "Update README.md to reflect getKeyInfoContent changes" is related to the changeset, as the modifications indeed involve updating documentation to reflect changes to getKeyInfoContent. However, the actual changeset involves swapping defaults for TWO public options: getKeyInfoContent (changed from noop to SignedXml.getKeyInfoContent) and getCertFromKeyInfo (changed from SignedXml.getCertFromKeyInfo to noop). The title captures only one of these two equally important changes, making it a partial representation of the full scope of the PR's modifications.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19623c3 and 796509f.

📒 Files selected for processing (1)

• README.md (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• README.md

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

README.md (2)

264-264: Clarify the meaning of "noop" default value.

The term "noop" is not immediately clear to all users. Based on the example at line 153 (getCertFromKeyInfo: () => null), "noop" appears to mean a function that returns null or does nothing. Consider clarifying this term or using more explicit language like "function that returns null" or "disabled by default".

Apply this diff to improve clarity:

-- `getCertFromKeyInfo` - function - default `noop` - a function that returns the certificate from the `<KeyInfo />` node
+- `getCertFromKeyInfo` - function - default `noop` (disabled; must provide your own implementation) - a function that returns the certificate from the `<KeyInfo />` node

---

264-264: Add explicit security warning about CVE-2024-32962.

The past review discussion (PR #445 context) identified a critical security vulnerability (CVE-2024-32962 / GHSA-2xp3-57p7-qf4v) when SignedXml.getCertFromKeyInfo is used. While the default is now correctly set to noop, users could still manually configure the vulnerable implementation. The documentation should include an explicit security warning to prevent misuse.

Consider adding a security note near the getCertFromKeyInfo option or in a dedicated security section. For example:

- `getCertFromKeyInfo` - function - default `noop` - a function that returns the certificate from the `<KeyInfo />` node
+- `getCertFromKeyInfo` - function - default `noop` - a function that returns the certificate from the `<KeyInfo />` node
+  **Security Warning:** Do **not** use `SignedXml.getCertFromKeyInfo` for this option, as it allows XML signatures to be validated with arbitrary certificates, bypassing the chain of trust. This would trigger CVE-2024-32962 / GHSA-2xp3-57p7-qf4v. Provide your own safe implementation if needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e692cf and 96222e0.

📒 Files selected for processing (1)

• README.md (1 hunks)

[dozgunyal]

@dozgunyal , are you willing to make the adjustments?

Hi, sorry, I will try to send it this weekend.

[dozgunyal]

Hi @cjbarth,
I have not been using this library for a while now. I've lost my access to the system that uses this library to verify my concerns too.
I've read your comments, the vulnerability document that you created and the standard.

Unfortunately, I do not agree with all of your concerns and I think this function is still a good helper.

Before I start the my explanation, I would like to tell you that I am going to close this PR and will not contribute more because I do not think that I have the relevant competence and I lack the vision of you contributors.

I am sorry if I wasted your time and energy but I am still glad that I bring this one to your attention.

Why I think removing this function is not necessary:

As per the standard:

• "Obtain the keying information from KeyInfo or from an external source."
• "However, questions of trust of such key information (e.g., its authenticity or strength) are out of scope of this specification and left to the application."

Firstly, I think, these bring legitimacy to obtaining key from the KeyInfo, if only the certificate is strong enough and validated properly. The project that I was working at the time was also depending on this implementation.
Also, I like how this function takes the need from developers to parse the document twice themselves. Because both the key info and the certificate are extracted by the tools at disposal of this library, namely isDomNode, utils.derToPem, xpath.

My suggestion would be extending the library documentation to highlight the importance of the validation of cert but keep this function. But again, I respectfully refuse to take this responsibility.

getCertFromKeyInfo: (keyInfo) => {
   const cert = SignedXml.getCertFromKeyInfo(keyInfo)
   // do whatever to check if the cert is valid
   return cert.
}

Kindest regards to all.

[cjbarth]

@dozgunyal , you are right, it is within spec to have the library return the KeyInfo, but we want the default to be noop so that the user pays attention to the security decisions they are making. With recent changes (#506), it is possible to have signed KeyInfo, so that allows for more secure usage in some cases.

[srd90]

@dozgunyal you wrote:

Also, I like how this function takes the need from developers to parse the document twice themselves.

Have you checked implementation of getCertFromKeyInfo?

xml-crypto/src/signed-xml.ts

Lines 230 to 239 in ef8ba25

	static getCertFromKeyInfo(keyInfo?: Node | null): string | null {
	if (keyInfo != null) {
	const cert = xpath.select1(".//*[local-name(.)='X509Certificate']", keyInfo);
	if (isDomNode.isNodeLike(cert)) {
	return utils.derToPem(cert.textContent ?? "", "CERTIFICATE");
	}
	}
	return null;
	}

it is not showstopper to (re)implement by person who is at the same time capable to understand security concerns related to using cert that is controllable by attacker. I.e. person who is capable to construct some proper validation (*) for cert is most likely capable to extract it from provided KeyInfo node.

(*) e.g. by checking that extracted cert is signed by some very specific corporate internal CA cert (I try to say that one should not trust all public CAs because attacker could order his/her own cert from the market and use associated key and signed cert to resign xml document) or that cert is listed at some corporate directory server to be trusted for signing (this would be equal to checking that extracted cert is at some list of trusted certs)

@cjbarth I did not spot from #506 change that would require specifically aforementioned footgun implementation (at public production API). Someone shall configure his/her stack to use it and is going to be very happy that now all (and I mean all) signed documents with attached certs are considered properly signed because it is less common to test incorrectly signed cases (test case that attacker resigned doc). It is more common to test only so called "happy cases". Situation was similar with passport-saml when one was able to disable cert checking completely by not providing cert at all / setting it to falsy, here is one starred gist of such approach: https://gist.github.com/fuxingloh/4d6e1caa24237c5870809fe24c47726f ( gist was fixed Aug 22, 2022 but it provided very unsecure config between years 2017 - 2022 https://gist.github.com/fuxingloh/4d6e1caa24237c5870809fe24c47726f/revisions ). There was also other similar cases.

I thought that footgun getCertFromKeyInfo was on its way out:

https://github.com/node-saml/xml-crypto/pull/470/files#r1591640940

https://github.com/node-saml/xml-crypto/pull/470/files#r2219669880

[ahacker1-securesaml]

Yes we should remove the footgun getCertFromKeyInfo. The current implementation defeats the whole point of signature verification, which trusts an arbitrary attacker supplied certificate.

I understand that sometimes, users do need to override the implementation of getCertFromKeyInfo. However, none of these cases, should or would involve using the untrusted certificate from the key info node verbatim.

We should provide sample known functions for: getCertFromKeyInfo (and remove default implementation). For example:

getCertFromTrustedCert(cert) {
return cert // already trusted, no need to parse KeyInfo
}

getCertFromTrustedFingerprint(fingerprint) {
// extract x509 certificate from keyinfo
// compare sha-256 of x509 certificate canonical der format with fingerprint
// if true then canonical der bytes are trusteed (authenticated by our fingerprint)
// re-parse canonical der bytes into new certificate and return it (if true)
}

getCertFromTrustedCA(ca) {
// extract x509 certificate from KeyInfo
// use ca chain of trust to verify x509 certificate, then return it
}

These should cover all the legitimate use cases. Default should be getCertFromTrustedCert (using known trusted certificate).

I'm not sure what use case @dozgunyal had. I'm pretty sure it falls into the 3 above.

I agree with @srd90 analysis about footgun. I would like to point out xmlsec1 had a similar recurring problem, where they trusted the certificate inside the attacker supplied keyinfo node by default. It has lead to several authentication bypasses, affecting big applications.

Regarding @dozgunyal statement on the spec:

"However, questions of trust of such key information (e.g., its authenticity or strength) are out of scope of this specification and left to the application."

We should be following secure by default standards. This specification is poorly written (no wonder the number of bugs in XML signature), we shouldn't follow this advice.

The spec shouldn't mean that we should trust attacker supplied certificates by default. It means we have to provide a mechanism for clients to verify with a trusted certificate, and that the specification doesn't detail it (it should have given examples like above).

Regarding @cjbarth statement on the signed keyinfo:

With recent changes (#506), it is possible to have signed KeyInfo, so that allows for more secure usage in some cases.

Signing keyinfo doesn't do anything here. Here, the problem is concerning using an untrusted certificate (i.e. from the keyinfo node), to verify the XML signature. Even if the keyinfo node is signed, we would be verifying it with the same untrusted certificate (from getCertFromKeyInfo) inside the keyInfo node.

[cjbarth]

Thank you to both @ahacker1-securesaml and @srd90 for your comments. I've gone back and forth on this many times, reading the spec, and then listening to your expertise. I can easily see a scenario by which that KeyInfo could have been added and signed with a cert that wasn't valid. The only way to legitimately defend against that would be to check the certificate chain.

So, for right now, we certainly want to get the documentation updated to reflect what is really going on. In the next breaking release I can certainly see removing it because we have no intention of dealing with certificate chains and revocation.

Still, I can see internal applications that would use their own certificate to sign something and send it. In that case, the signature would prove only that the payload hasn't changed since it was signed with the provided key. In that case, when coupled with certificate inspection, a thusly signed XML document could be shown to be approved by the signer, no matter who generated the original XML.

(I will process all the XML documents generated by a desktop in our domain, and each one will sign it with its own cert and include that in KeyInfo. I just have to check to make sure that our domain cert is in the chain. I don't want to be bothered keeping track of each individual cert.)

So, maybe I'm missing something, but the problem isn't really that the cert comes from the XML, it is that, without inspecting the cert, we shouldn't trust it as I understand @srd90 also says. So, by removing this stub code, we are forcing them to write code that both pulls the KeyInfo and validates the extracted key. Since that should all be in one step, I can entirely see why the existing default implementation is worthless and should be removed. We'll be sure to update the README with the examples that @ahacker1-securesaml provided when that happens (hopefully soon).