diff --git a/README.md b/README.md index e6650ef4..7909aeec 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ signature algorithms enabled at same time. When signing a xml document you can pass the following options to the `SignedXml` constructor to customize the signature process: - `privateKey` - **[required]** a `Buffer` or pem encoded `String` containing your private key -- `publicCert` - **[optional]** a `Buffer` or pem encoded `String` containing your public key +- `publicCert` - **[optional]** the X.509 certificate to publish in ``, or a chain of them, as PEM or as one certificate's base64 without the PEM boundaries, in a `String` or `Buffer`. A value that holds no certificate, such as a public key, produces no ``. - `signatureAlgorithm` - **[required]** one of the supported [signature algorithms](#signature-algorithms). Ex: `sign.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"` - `canonicalizationAlgorithm` - **[required]** one of the supported [canonicalization algorithms](#canonicalization-and-transformation-algorithms). Ex: `sign.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#WithComments"` @@ -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,18 @@ MIIBxDCCAW6gAwIBAgIQxUSX... -----END CERTIFICATE----- ``` +### One key per value + +`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. +- 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. +- To trust several independent keys, verify with each in turn, as node-saml does for its `idpCert` + array. + ### What the parser accepts `toPem()`, `pemToDer()` and `pemCertificates()` read diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 66902e71..79ef66dd 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -38,6 +38,20 @@ function findSignatureElements(node: Node): Element[] { return signatures.filter(isDomNode.isElementNode); } +function certificatesToPublish(publicCert: string): string[] { + const certificates = utils.pemCertificates(publicCert); + if (certificates.length > 0) { + return certificates; + } + + try { + return [utils.bareCertificate(publicCert)]; + } catch { + // Not a certificate in either form, so there is none to publish, and KeyInfo is optional. + return []; + } +} + const warnOriginalXmlWithIds = deprecate( () => {}, "`getOriginalXmlWithIds()` is deprecated and will be removed in a future version. Use the `location` option of `computeSignature()` to place the signature, then `getSignedXml()`. For a detached signature, put an ID attribute the signer recognizes on each referenced element (`wsu:Id` for WS-Security), sign that document, and send it alongside `getSignatureXml()`.", @@ -229,7 +243,7 @@ export class SignedXml { } // A KeyObject holds a key and never a certificate, so there is no X509Data to build from it. - const certificates = typeof publicCert === "string" ? utils.pemCertificates(publicCert) : []; + const certificates = typeof publicCert === "string" ? certificatesToPublish(publicCert) : []; // X509Data requires at least one child: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data if (certificates.length === 0) { diff --git a/src/utils.ts b/src/utils.ts index 64412736..f449a7c8 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -408,6 +408,19 @@ function pemText(value: string | Buffer): string { : value.toString("base64"); } +// Refuses PEM rather than passing it through as toPem() does, so no key can come out as a certificate. +export function bareCertificate(value: string): string { + const text = normalizePemInput(value); + const data = text.replace(/\n/g, ""); + + if (!BASE64_TEXT_REGEX.test(text) || !isBase64Data(data)) { + throw new Error("Invalid PEM format."); + } + assertX509Certificate(data); + + return canonicalBase64(data); +} + /** * Returns a value as canonical PEM: one message per certificate or key, wrapped at 64 characters. * The value may be a PEM message, several of them, base64 data with the label supplied by the diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 4367660b..7e51e477 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"; @@ -1442,6 +1442,27 @@ describe("Signature unit tests", function () { it("when publicCert is a KeyObject, which holds a key and never a certificate", function () { expect(selectKeyInfo({ publicCert: crypto.createPublicKey(privateKey) })).to.be.empty; }); + + it("when publicCert is a private key", function () { + expect(selectKeyInfo({ publicCert: privateKey })).to.be.empty; + }); + + it("when publicCert is the base64 of a private key, without boundaries", function () { + const der = crypto.createPrivateKey(privateKey).export({ type: "pkcs8", format: "der" }); + + expect(selectKeyInfo({ publicCert: der.toString("base64") })).to.be.empty; + }); + + it("when publicCert is the base64 of a certificate with more data after it", function () { + const der = fs.readFileSync("./test/static/client_public.der"); + const publicCert = Buffer.concat([der, Buffer.from("more")]).toString("base64"); + + expect(selectKeyInfo({ publicCert })).to.be.empty; + }); + + it("when publicCert is not a certificate in any form", function () { + expect(selectKeyInfo({ publicCert: "not a certificate" })).to.be.empty; + }); }); describe("getCertFromKeyInfo", function () { @@ -1504,7 +1525,7 @@ describe("Signature unit tests", function () { }); }); - function signWithPublicCert(publicCert: string) { + function signWithPublicCert(publicCert: string | Buffer) { const sig = new SignedXml({ privateKey: fs.readFileSync("./test/static/client.pem"), publicCert, @@ -1525,7 +1546,7 @@ describe("Signature unit tests", function () { } // The text of each X509Certificate that signing with this publicCert puts into KeyInfo. - function publishedCertificates(publicCert: string): string[] { + function publishedCertificates(publicCert: string | Buffer): string[] { const doc = new xmldom.DOMParser().parseFromString(signWithPublicCert(publicCert)()); const certificates = xpath.select("//*[local-name(.)='X509Certificate']", doc); isDomNode.assertIsArrayOfNodes(certificates); @@ -1564,6 +1585,15 @@ describe("Signature unit tests", function () { expect(signWithPublicCert(publicCert)).to.throw("Invalid PEM format."); }); + it("publishes a publicCert given as the base64 of a certificate, without boundaries", function () { + const lines = fs.readFileSync("./test/static/client_public.pem", "latin1").trim().split("\n"); + const data = lines.slice(1, -1); + + expect(publishedCertificates(data.join(""))).to.deep.equal([data.join("")]); + expect(publishedCertificates(data.join("\n"))).to.deep.equal([data.join("")]); + expect(publishedCertificates(Buffer.from(data.join("\n")))).to.deep.equal([data.join("")]); + }); + it("signs a BER certificate into KeyInfo with its octets as given", function () { // RFC 7468 section 5.1 allows BER, and XML Signature 1.1 says an implementation SHOULD NOT // re-encode a certificate: https://www.w3.org/TR/xmldsig-core1/#sec-X509Data @@ -1725,4 +1755,43 @@ describe("Signature unit tests", function () { "#unique-id", ); }); + + 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 first = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const second = toPem(pemCertificates(bundle)[0], "CERTIFICATE"); + + function checkSignedBy(privateKey: string, publicCert: 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(""); + const xml = sig.getSignedXml(); + + const verifier = new SignedXml({ publicCert }); + const signature = xpath.select1( + "//*[local-name(.)='Signature']", + new xmldom.DOMParser().parseFromString(xml), + ); + isDomNode.assertIsNodeLike(signature); + verifier.loadSignature(signature); + + return verifier.checkSignature(xml); + } + + 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"); + }); }); diff --git a/test/utils-tests.spec.ts b/test/utils-tests.spec.ts index 8da5baf6..f37305e5 100644 --- a/test/utils-tests.spec.ts +++ b/test/utils-tests.spec.ts @@ -86,6 +86,8 @@ describe("Utils tests", function () { "a line break between quanta": "QUJD\nREVG", "a line break anywhere in the data": "QU\nJDRE\nVG", "blanks in the data": " QU JD\tREVG ", + // RFC 7468 Figure 1 'base64finl': https://www.rfc-editor.org/rfc/rfc7468#section-3 + "a pad split across a line ending": "QUJDCg=\n=", }; const rejected = { @@ -101,6 +103,8 @@ describe("Utils tests", function () { "a character outside the base64 alphabet": "QU-JD", "nothing at all": "", "only blanks": " ", + "only padding": "==", + "a padded line with more data after it": "QUJDCg==\nQUJD", }; Object.entries(accepted).forEach(([description, data]) => { @@ -200,6 +204,31 @@ describe("Utils tests", function () { ); }); + for (const [name, prefix] of [ + ["a UTF-8 BOM", Buffer.from([0xef, 0xbb, 0xbf])], + ["blank lines", Buffer.from("\r\n \n")], + ] as const) { + it(`a Buffer holding a PEM file that opens with ${name}, rather than DER`, function () { + const file = Buffer.concat([prefix, fs.readFileSync("./test/static/client_public.pem")]); + + expect(utils.toPem(file)).to.equal(normalizedPem); + }); + } + + for (const [name, value, label] of [ + ["blank lines around a message", `\n\n${normalizedPem}\n\n`, undefined], + ["a line ending after bare base64", `${body.join("\n")}\n`, "CERTIFICATE"], + [ + "blanks and CRLF around bare base64", + `\r\n \r\n${body.join("\r\n")}\r\n\t\r\n`, + "CERTIFICATE", + ], + ] as const) { + it(name, function () { + expect(utils.toPem(value, label)).to.equal(normalizedPem); + }); + } + it("several certificates in one value", function () { const bundle = fs.readFileSync("./test/static/client_bundle.pem", "latin1"); @@ -382,6 +411,49 @@ describe("Utils tests", function () { ); }); }); + + describe("rejects a value it cannot account for whole", function () { + const normalizedPem = fs.readFileSync("./test/static/client_public.pem", "latin1"); + const lines = normalizedPem.trim().split("\n"); + const header = lines[0]; + const footer = lines[lines.length - 1]; + const body = lines.slice(1, -1); + const last = body.length - 1; + + for (const [place, value] of [ + ["the header's line", [`${header}${body[0]}`, ...body.slice(1), footer].join("\n")], + [ + "the footer's line", + [header, ...body.slice(0, last), `${body[last]}${footer}`].join("\n"), + ], + ["both boundaries' line", `${header}${body.join("")}${footer}`], + ] as const) { + it(`data on ${place}`, function () { + expect(() => utils.toPem(value)).to.throw("Invalid PEM format."); + }); + } + + it("a message that is opened and never closed", function () { + // node-saml's ReDoS regression; the rejection is asserted, not the time it takes. + const unclosed = `-----BEGIN CERTIFICATE-----\r\n${"AAAA\r\n".repeat(26)}!`; + + expect(() => utils.toPem(unclosed)).to.throw("Invalid PEM format."); + }); + + it("a header followed directly by its footer", function () { + expect(() => utils.toPem(`${header}\n${footer}\n`)).to.throw("Invalid PEM format."); + }); + + for (const [place, value] of [ + ["before the message", `subject=/CN=client\n${normalizedPem}`], + ["after the message", `${normalizedPem}Issued for testing.\n`], + ["between two messages", `${normalizedPem}and its issuer:\n${normalizedPem}`], + ] as const) { + it(`explanatory text ${place}`, function () { + expect(() => utils.toPem(value)).to.throw("Invalid PEM format."); + }); + } + }); }); describe("pemToDer", function () {