From a62ab91c57a51711e1a74b842c8b918053e318f5 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 10:00:06 -0500 Subject: [PATCH 1/7] docs: say that privateKey and publicCert are read for one key each The README offered "a string of multiple certs in PEM format" as a verification publicCert, but Node's crypto loads one key from a value and ignores the rest: the first of several certificates, and a public key before any certificate. Say so, and how to trust several keys. The test pins the half of this that matters: a publicCert holding two certificates does not verify a signature made with the second one's key, which in a chain is the issuer's. Co-Authored-By: Claude Opus 5 --- README.md | 18 +++++++++- test/signature-unit-tests.spec.ts | 57 ++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index e6650ef4..c921fb4c 100644 --- a/README.md +++ b/README.md @@ -226,7 +226,7 @@ To customize this see [customizing algorithms](#customizing-algorithms) for an e When verifying a xml document you can pass the following options to the `SignedXml` constructor to customize the verify process: -- `publicCert` - **[optional]** your certificate as a string, a string of multiple certs in PEM format, or a Buffer +- `publicCert` - **[optional]** the certificate or public key to verify with, as a PEM `String` or `Buffer`. Verification uses [one key](#one-key-per-value) from it. - `privateKey` - **[optional]** your private key as a string or a Buffer - used for verifying symmetrical signatures (HMAC) The certificate that will be used to check the signature will first be determined by calling `this.getCertFromKeyInfo()`, which function you can customize as you see fit. If that returns `null`, then `publicCert` is used. If that is `null`, then `privateKey` is used (for symmetrical signing applications). @@ -565,6 +565,22 @@ MIIBxDCCAW6gAwIBAgIQxUSX... -----END CERTIFICATE----- ``` +### One key per value + +Signing and verification hand `privateKey` and `publicCert` to Node's crypto, which loads one key +from a value and ignores the rest. + +- `privateKey` holds one private key. A file that also holds its certificate, or its chain, is + fine. Given several private keys, signing uses the first. +- Verification takes one key from `publicCert`. From several certificates it takes the first, which + is how a chain given leaf first works, so the chain's issuers are not trusted to sign. A public + key is taken before any certificate, wherever it is in the value. +- To trust several independent keys, verify with each in turn, as node-saml does for its `idpCert` + array. + +A `privateKey` holding more than one private key, or a `publicCert` holding a public key together +with any other key or certificate, prints a warning. Both will be errors in 7.0. + ### What the parser accepts `toPem()`, `pemToDer()` and `pemCertificates()` read diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 4367660b..f7ef9cd9 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1,6 +1,6 @@ import * as xpath from "xpath"; import * as xmldom from "@xmldom/xmldom"; -import { SignedXml, createOptionalCallbackFunction } from "../src/index"; +import { SignedXml, createOptionalCallbackFunction, pemCertificates, toPem } from "../src/index"; import * as fs from "fs"; import * as crypto from "crypto"; import { expect } from "chai"; @@ -1725,4 +1725,59 @@ describe("Signature unit tests", function () { "#unique-id", ); }); + + describe("verifies with one key from a publicCert holding several", function () { + const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); + const pairs = { + A: { + privateKey: fs.readFileSync("./test/static/client.pem", "latin1"), + certificate: fs.readFileSync("./test/static/client_public.pem", "latin1"), + }, + // The bundle holds this key beside its certificates, and signs with it. + B: { privateKey: bundle, certificate: toPem(pemCertificates(bundle)[0], "CERTIFICATE") }, + }; + + function sign(privateKey: string) { + const sig = new SignedXml({ + privateKey, + canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", + signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + }); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + transforms: [ + "http://www.w3.org/2000/09/xmldsig#enveloped-signature", + "http://www.w3.org/2001/10/xml-exc-c14n#", + ], + }); + sig.computeSignature(""); + + return sig.getSignedXml(); + } + + function checkSignature(xml: string, publicCert: string) { + const doc = new xmldom.DOMParser().parseFromString(xml); + const signature = xpath.select1("//*[local-name(.)='Signature']", doc); + isDomNode.assertIsNodeLike(signature); + const sig = new SignedXml({ publicCert }); + sig.loadSignature(signature); + + return sig.checkSignature(xml); + } + + for (const [first, second] of [ + ["A", "B"], + ["B", "A"], + ] as const) { + it(`uses the first of two certificates, and not the second, which in a chain is the issuer's (${first} first)`, function () { + const publicCert = `${pairs[first].certificate}${pairs[second].certificate}`; + + expect(checkSignature(sign(pairs[first].privateKey), publicCert)).to.be.true; + expect(() => checkSignature(sign(pairs[second].privateKey), publicCert)).to.throw( + "invalid signature", + ); + }); + } + }); }); From 3899798d949099148874c33526e17d9abf054156 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 10:00:06 -0500 Subject: [PATCH 2/7] fix: warn once when privateKey or publicCert holds keys Node ignores Emit a process warning, once per process for each case, when signing with a string or Buffer privateKey holding more than one private key, and when verifying with a string or Buffer publicCert holding a public key together with any other key or certificate. Several certificates with no public key are also what a chain looks like, so they stay silent. 7.0 makes both cases errors (#608). Closes #606 Co-Authored-By: Claude Opus 5 --- src/signed-xml.ts | 48 +++++++++++++++++++++++++++++++++++++++++++++++ src/utils.ts | 10 ++++++++++ 2 files changed, 58 insertions(+) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 66902e71..c679edea 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -50,6 +50,50 @@ const warnValidateElementAgainstReferences = deprecate( "XML_CRYPTO_VALIDATE_ELEMENT_AGAINST_REFERENCES", ); +// Node's crypto loads one key from a value and ignores the rest, so these are the values in which +// it ignores a key the caller may have meant it to use. #608 makes them errors in 7.0. +const emittedKeyWarnings = new Set(); + +function keyLabels(value: crypto.KeyLike): string[] { + if (typeof value !== "string" && !Buffer.isBuffer(value)) { + return []; + } + + const text = Buffer.isBuffer(value) ? value.toString("latin1") : value; + + return utils + .pemLabels(text) + .filter((label) => /(?:PRIVATE KEY|PUBLIC KEY|CERTIFICATE)$/.test(label)); +} + +function warnOnceForKey(code: string, message: string) { + if (!emittedKeyWarnings.has(code)) { + emittedKeyWarnings.add(code); + process.emitWarning(message, { code }); + } +} + +function warnIfSeveralPrivateKeys(privateKey: crypto.KeyLike) { + if (keyLabels(privateKey).filter((label) => label.endsWith("PRIVATE KEY")).length > 1) { + warnOnceForKey( + "XML_CRYPTO_SEVERAL_PRIVATE_KEYS", + "`privateKey` holds more than one private key, and only the first is used to sign. This will be an error in 7.0. Give `privateKey` one private key.", + ); + } +} + +// Several certificates are not reported: that is also what a chain given leaf first looks like. +function warnIfPublicKeyAmongOthers(publicCert: crypto.KeyLike) { + const labels = keyLabels(publicCert); + + if (labels.length > 1 && labels.some((label) => label.endsWith("PUBLIC KEY"))) { + warnOnceForKey( + "XML_CRYPTO_PUBLIC_KEY_AMONG_OTHERS", + "`publicCert` holds a public key together with other keys or certificates, and only its first public key is used to verify. This will be an error in 7.0. Give `publicCert` one key, and verify with each key in turn to trust several.", + ); + } +} + export class SignedXml { idMode?: "wssecurity"; idAttributes: string[]; @@ -368,6 +412,9 @@ export class SignedXml { if (key == null) { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } + if (key === this.publicCert) { + warnIfPublicKeyAmongOthers(key); + } // Check the signature verification to know whether to reset signature value or not. const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); @@ -465,6 +512,7 @@ export class SignedXml { if (this.privateKey == null) { throw new Error("Private key is required to compute signature"); } + warnIfSeveralPrivateKeys(this.privateKey); if (typeof callback === "function") { signer.getSignature(signedInfoCanon, this.privateKey, callback); } else { diff --git a/src/utils.ts b/src/utils.ts index 64412736..ee91c6c5 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -366,6 +366,16 @@ export function pemCertificates(pem: string): string[] { return certificates.map(canonicalBase64); } +// A value this parser cannot read yields no labels, because callers only report on what Node's +// crypto goes on to load, and must not fail a value it can load. +export function pemLabels(pem: string): string[] { + try { + return pemMessages(normalizePemInput(pem)).map((message) => message.label); + } catch { + return []; + } +} + /** * Returns the decoded bytes of the one PEM message a value holds, whatever its label. * From dcb3a4c92b1228bce4d2064e032a27bf4e647333 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 10:38:21 -0500 Subject: [PATCH 3/7] fix: leave to Node which key it picks from an ambiguous value The README and the warnings said which key Node's crypto takes from a value holding several. That is Node's behavior to document, and these are the values 7.0 rejects, so both now say only that one key is used. The label match stays broad on purpose, a cheap superset of the keys Node loads, and the comment says the 7.0 error has to ask Node instead. Co-Authored-By: Claude Opus 5 --- README.md | 5 ++--- src/signed-xml.ts | 9 +++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index c921fb4c..46b6b9f3 100644 --- a/README.md +++ b/README.md @@ -571,10 +571,9 @@ Signing and verification hand `privateKey` and `publicCert` to Node's crypto, wh from a value and ignores the rest. - `privateKey` holds one private key. A file that also holds its certificate, or its chain, is - fine. Given several private keys, signing uses the first. + fine. - Verification takes one key from `publicCert`. From several certificates it takes the first, which - is how a chain given leaf first works, so the chain's issuers are not trusted to sign. A public - key is taken before any certificate, wherever it is in the value. + is how a chain given leaf first works, so the chain's issuers are not trusted to sign. - To trust several independent keys, verify with each in turn, as node-saml does for its `idpCert` array. diff --git a/src/signed-xml.ts b/src/signed-xml.ts index c679edea..7a6df7bd 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -50,8 +50,9 @@ const warnValidateElementAgainstReferences = deprecate( "XML_CRYPTO_VALIDATE_ELEMENT_AGAINST_REFERENCES", ); -// Node's crypto loads one key from a value and ignores the rest, so these are the values in which -// it ignores a key the caller may have meant it to use. #608 makes them errors in 7.0. +// Node's crypto loads one key from a value and ignores the rest. These warn about values that might +// hold a key it ignores, judged by label alone and erring toward a warning: which messages are keys +// is Node's to say, and the 7.0 error in #608 has to ask it rather than tighten these labels. const emittedKeyWarnings = new Set(); function keyLabels(value: crypto.KeyLike): string[] { @@ -77,7 +78,7 @@ function warnIfSeveralPrivateKeys(privateKey: crypto.KeyLike) { if (keyLabels(privateKey).filter((label) => label.endsWith("PRIVATE KEY")).length > 1) { warnOnceForKey( "XML_CRYPTO_SEVERAL_PRIVATE_KEYS", - "`privateKey` holds more than one private key, and only the first is used to sign. This will be an error in 7.0. Give `privateKey` one private key.", + "`privateKey` holds more than one private key, and only one of them is used to sign. This will be an error in 7.0. Give `privateKey` one private key.", ); } } @@ -89,7 +90,7 @@ function warnIfPublicKeyAmongOthers(publicCert: crypto.KeyLike) { if (labels.length > 1 && labels.some((label) => label.endsWith("PUBLIC KEY"))) { warnOnceForKey( "XML_CRYPTO_PUBLIC_KEY_AMONG_OTHERS", - "`publicCert` holds a public key together with other keys or certificates, and only its first public key is used to verify. This will be an error in 7.0. Give `publicCert` one key, and verify with each key in turn to trust several.", + "`publicCert` holds a public key together with other keys or certificates, and only one of them is used to verify. This will be an error in 7.0. Give `publicCert` one key, and verify with each key in turn to trust several.", ); } } From 5a11fb23c366fe5f6aed2933eae72081fcd016cd Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 11:04:40 -0500 Subject: [PATCH 4/7] fix: drop the warnings for privateKey and publicCert holding several keys privateKey and publicCert are crypto.KeyLike, so what Node's crypto reads is valid input, and RFC 7468 section 2 allows several messages in one value. Node reads such a value and fails closed, so nothing here should throw, and a warning ahead of an error that should not come is code to maintain for no gain. It also parsed key material that Node parses. What remains is the README, which promised that several certificates would each verify, and the test pinning that a chain's issuer is not trusted. Co-Authored-By: Claude Opus 5 --- README.md | 3 --- src/signed-xml.ts | 49 ----------------------------------------------- src/utils.ts | 10 ---------- 3 files changed, 62 deletions(-) diff --git a/README.md b/README.md index 46b6b9f3..41a16ca4 100644 --- a/README.md +++ b/README.md @@ -577,9 +577,6 @@ from a value and ignores the rest. - To trust several independent keys, verify with each in turn, as node-saml does for its `idpCert` array. -A `privateKey` holding more than one private key, or a `publicCert` holding a public key together -with any other key or certificate, prints a warning. Both will be errors in 7.0. - ### What the parser accepts `toPem()`, `pemToDer()` and `pemCertificates()` read diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 7a6df7bd..66902e71 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -50,51 +50,6 @@ const warnValidateElementAgainstReferences = deprecate( "XML_CRYPTO_VALIDATE_ELEMENT_AGAINST_REFERENCES", ); -// Node's crypto loads one key from a value and ignores the rest. These warn about values that might -// hold a key it ignores, judged by label alone and erring toward a warning: which messages are keys -// is Node's to say, and the 7.0 error in #608 has to ask it rather than tighten these labels. -const emittedKeyWarnings = new Set(); - -function keyLabels(value: crypto.KeyLike): string[] { - if (typeof value !== "string" && !Buffer.isBuffer(value)) { - return []; - } - - const text = Buffer.isBuffer(value) ? value.toString("latin1") : value; - - return utils - .pemLabels(text) - .filter((label) => /(?:PRIVATE KEY|PUBLIC KEY|CERTIFICATE)$/.test(label)); -} - -function warnOnceForKey(code: string, message: string) { - if (!emittedKeyWarnings.has(code)) { - emittedKeyWarnings.add(code); - process.emitWarning(message, { code }); - } -} - -function warnIfSeveralPrivateKeys(privateKey: crypto.KeyLike) { - if (keyLabels(privateKey).filter((label) => label.endsWith("PRIVATE KEY")).length > 1) { - warnOnceForKey( - "XML_CRYPTO_SEVERAL_PRIVATE_KEYS", - "`privateKey` holds more than one private key, and only one of them is used to sign. This will be an error in 7.0. Give `privateKey` one private key.", - ); - } -} - -// Several certificates are not reported: that is also what a chain given leaf first looks like. -function warnIfPublicKeyAmongOthers(publicCert: crypto.KeyLike) { - const labels = keyLabels(publicCert); - - if (labels.length > 1 && labels.some((label) => label.endsWith("PUBLIC KEY"))) { - warnOnceForKey( - "XML_CRYPTO_PUBLIC_KEY_AMONG_OTHERS", - "`publicCert` holds a public key together with other keys or certificates, and only one of them is used to verify. This will be an error in 7.0. Give `publicCert` one key, and verify with each key in turn to trust several.", - ); - } -} - export class SignedXml { idMode?: "wssecurity"; idAttributes: string[]; @@ -413,9 +368,6 @@ export class SignedXml { if (key == null) { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } - if (key === this.publicCert) { - warnIfPublicKeyAmongOthers(key); - } // Check the signature verification to know whether to reset signature value or not. const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); @@ -513,7 +465,6 @@ export class SignedXml { if (this.privateKey == null) { throw new Error("Private key is required to compute signature"); } - warnIfSeveralPrivateKeys(this.privateKey); if (typeof callback === "function") { signer.getSignature(signedInfoCanon, this.privateKey, callback); } else { diff --git a/src/utils.ts b/src/utils.ts index ee91c6c5..64412736 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -366,16 +366,6 @@ export function pemCertificates(pem: string): string[] { return certificates.map(canonicalBase64); } -// A value this parser cannot read yields no labels, because callers only report on what Node's -// crypto goes on to load, and must not fail a value it can load. -export function pemLabels(pem: string): string[] { - try { - return pemMessages(normalizePemInput(pem)).map((message) => message.label); - } catch { - return []; - } -} - /** * Returns the decoded bytes of the one PEM message a value holds, whatever its label. * From c44655ea6626b2ada24c4d3adb663cd932abc2a8 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 11:32:46 -0500 Subject: [PATCH 5/7] test: prove the issuer is not trusted in one direction, without scaffolding Co-Authored-By: Claude Opus 5 --- test/signature-unit-tests.spec.ts | 48 +++++++++++-------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index f7ef9cd9..f391cfd3 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1726,18 +1726,14 @@ describe("Signature unit tests", function () { ); }); - describe("verifies with one key from a publicCert holding several", function () { + it("verifies with the first of two certificates, and not the second, which in a chain is the issuer's", function () { const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); - const pairs = { - A: { - privateKey: fs.readFileSync("./test/static/client.pem", "latin1"), - certificate: fs.readFileSync("./test/static/client_public.pem", "latin1"), - }, - // The bundle holds this key beside its certificates, and signs with it. - B: { privateKey: bundle, certificate: toPem(pemCertificates(bundle)[0], "CERTIFICATE") }, - }; + const publicCert = `${fs.readFileSync("./test/static/client_public.pem", "latin1")}${toPem( + pemCertificates(bundle)[0], + "CERTIFICATE", + )}`; - function sign(privateKey: string) { + function checkSignedBy(privateKey: string) { const sig = new SignedXml({ privateKey, canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", @@ -1752,32 +1748,20 @@ describe("Signature unit tests", function () { ], }); sig.computeSignature(""); + const xml = sig.getSignedXml(); - return sig.getSignedXml(); - } - - function checkSignature(xml: string, publicCert: string) { - const doc = new xmldom.DOMParser().parseFromString(xml); - const signature = xpath.select1("//*[local-name(.)='Signature']", doc); + const verifier = new SignedXml({ publicCert }); + const signature = xpath.select1( + "//*[local-name(.)='Signature']", + new xmldom.DOMParser().parseFromString(xml), + ); isDomNode.assertIsNodeLike(signature); - const sig = new SignedXml({ publicCert }); - sig.loadSignature(signature); + verifier.loadSignature(signature); - return sig.checkSignature(xml); + return verifier.checkSignature(xml); } - for (const [first, second] of [ - ["A", "B"], - ["B", "A"], - ] as const) { - it(`uses the first of two certificates, and not the second, which in a chain is the issuer's (${first} first)`, function () { - const publicCert = `${pairs[first].certificate}${pairs[second].certificate}`; - - expect(checkSignature(sign(pairs[first].privateKey), publicCert)).to.be.true; - expect(() => checkSignature(sign(pairs[second].privateKey), publicCert)).to.throw( - "invalid signature", - ); - }); - } + expect(checkSignedBy(fs.readFileSync("./test/static/client.pem", "latin1"))).to.be.true; + expect(() => checkSignedBy(bundle)).to.throw("invalid signature"); }); }); From 06bf74c474664d48f7aac437a131bd6151dd3fba Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Sat, 19 Sep 2026 11:42:49 -0500 Subject: [PATCH 6/7] test: check the second certificate verifies its own signature Without that control, rejecting the bundle's signature against the pair proved nothing: it would also pass if the signature were invalid under the second certificate for any other reason. Co-Authored-By: Claude Opus 5 --- test/signature-unit-tests.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index f391cfd3..c6fd483a 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -1728,12 +1728,10 @@ describe("Signature unit tests", function () { it("verifies with the first of two certificates, and not the second, which in a chain is the issuer's", function () { const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); - const publicCert = `${fs.readFileSync("./test/static/client_public.pem", "latin1")}${toPem( - pemCertificates(bundle)[0], - "CERTIFICATE", - )}`; + const first = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const second = toPem(pemCertificates(bundle)[0], "CERTIFICATE"); - function checkSignedBy(privateKey: string) { + function checkSignedBy(privateKey: string, publicCert: string) { const sig = new SignedXml({ privateKey, canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#", @@ -1761,7 +1759,9 @@ describe("Signature unit tests", function () { return verifier.checkSignature(xml); } - expect(checkSignedBy(fs.readFileSync("./test/static/client.pem", "latin1"))).to.be.true; - expect(() => checkSignedBy(bundle)).to.throw("invalid signature"); + expect(checkSignedBy(fs.readFileSync("./test/static/client.pem", "latin1"), first + second)).to + .be.true; + expect(checkSignedBy(bundle, second)).to.be.true; + expect(() => checkSignedBy(bundle, first + second)).to.throw("invalid signature"); }); }); From c9c5ca4aad464278177771e0750ba0d10a5a81a0 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Mon, 21 Sep 2026 07:14:23 -0500 Subject: [PATCH 7/7] docs: name the key each option supplies to Node's crypto Co-Authored-By: Claude Opus 5 --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 41a16ca4..96aea9b0 100644 --- a/README.md +++ b/README.md @@ -567,8 +567,8 @@ MIIBxDCCAW6gAwIBAgIQxUSX... ### One key per value -Signing and verification hand `privateKey` and `publicCert` to Node's crypto, which loads one key -from a value and ignores the rest. +`privateKey` when signing, and `publicCert` when verifying, are passed to Node's crypto, which uses +one key from the value. - `privateKey` holds one private key. A file that also holds its certificate, or its chain, is fine.