From b82c3d1b497b125ae78cacd487ac60836dd3a29a Mon Sep 17 00:00:00 2001 From: Andrew Heberle Date: Tue, 6 Oct 2026 19:49:52 +0800 Subject: [PATCH] fix(bug): work around bugs in sshpk --- packages/core/src/krl.ts | 69 ++++++++---- packages/core/tests/keys/ed25519.ts | 14 +++ packages/core/tests/krl.test.ts | 105 ++++++++++++++++++ .../tests/ed25519-ca.test.ts | 27 +++++ .../workers-integration/tests/keys/ed25519.ts | 11 ++ 5 files changed, 207 insertions(+), 19 deletions(-) create mode 100644 packages/core/tests/krl.test.ts create mode 100644 packages/workers-integration/tests/ed25519-ca.test.ts diff --git a/packages/core/src/krl.ts b/packages/core/src/krl.ts index a546daa8..f66dd769 100644 --- a/packages/core/src/krl.ts +++ b/packages/core/src/krl.ts @@ -1,4 +1,5 @@ -import type { PrivateKey } from "sshpk" +import type { AlgorithmPart, PrivateKey } from "sshpk" +import { toFixedWidth } from "./sshsig/sig_parser" // ── Constants ──────────────────────────────────────────────────────────────── @@ -94,20 +95,25 @@ const HASH_ALGORITHM = "sha512" // ── Helpers ────────────────────────────────────────────────────────────────── /** - * PEM string → raw DER bytes (strips header/footer and decodes base64). - * Works for both PKCS8 ("-----BEGIN PRIVATE KEY-----") and any other PEM type. + * Get the raw bytes of a named part of an sshpk key. */ -function pemToDer(pem: string): Uint8Array { - const lines = pem - .trim() - .split("\n") - .filter((l) => !l.startsWith("-----")) - const binary = atob(lines.join("")) - const bytes = new Uint8Array(binary.length) - for (let i = 0; i < binary.length; i++) { - bytes[i] = binary.charCodeAt(i) +function keyPart(key: PrivateKey, name: AlgorithmPart): Uint8Array { + const part = key.parts.find((p) => p.name === name) + if (part === undefined) { + throw new Error(`Missing ${name} part in ${key.type} key`) } - return bytes + return part.data +} + +/** + * Base64url-encode bytes without padding, as used for JWK values. + */ +function base64url(bytes: Uint8Array): string { + let binary = "" + for (const byte of bytes) { + binary += String.fromCharCode(byte) + } + return btoa(binary).replace(/\+/g, "-").replace(/\//g, "_").replace(/=+$/, "") } /** @@ -119,15 +125,21 @@ async function importPrivateKey(caKey: PrivateKey): Promise<{ sigAlgo: string // SSH signature algorithm name webCryptoAlgo: SubtleCryptoSignAlgorithm }> { - const pkcs8Pem = caKey.toString("pkcs8") - const der = pemToDer(pkcs8Pem) const type = caKey.type // "ed25519" | "ecdsa" | etc. const curve = (caKey as unknown as { curve?: string }).curve if (type === "ed25519") { + // import from the raw key parts as a JWK rather than via sshpk's PKCS#8 + // output, which drops a leading zero byte from the seed (about 1 key in + // 512) and is then rejected by WebCrypto + const seed = keyPart(caKey, "k") + const publicKey = keyPart(caKey, "A") + if (seed.length !== 32 || publicKey.length !== 32) { + throw new Error(`Unexpected Ed25519 key part lengths: k=${seed.length} A=${publicKey.length}`) + } const cryptoKey = await crypto.subtle.importKey( - "pkcs8", - der, + "jwk", + { kty: "OKP", crv: "Ed25519", d: base64url(seed), x: base64url(publicKey) }, { name: "Ed25519" }, false, ["sign"], @@ -141,26 +153,45 @@ async function importPrivateKey(caKey: PrivateKey): Promise<{ let namedCurve: string let sigAlgo: string let hash: string + let size: number // bytes in each coordinate and the private key if (curve === "nistp256") { namedCurve = "P-256" sigAlgo = "ecdsa-sha2-nistp256" hash = "SHA-256" + size = 32 } else if (curve === "nistp384") { namedCurve = "P-384" sigAlgo = "ecdsa-sha2-nistp384" hash = "SHA-384" + size = 48 } else if (curve === "nistp521") { namedCurve = "P-521" sigAlgo = "ecdsa-sha2-nistp521" hash = "SHA-512" + size = 66 } else { throw new Error(`Unsupported ECDSA curve: ${curve}`) } + // import from the raw key parts as a JWK rather than via sshpk's PKCS#8 + // output, which drops leading zero bytes from the private key so it is + // shorter than RFC 5915 requires (about half of P-521 keys) + const point = keyPart(caKey, "Q") + if (point.length !== 1 + size * 2 || point[0] !== 0x04) { + throw new Error(`Unexpected ECDSA public key encoding for ${curve}`) + } + // d is an SSH mpint, so may be short or have a leading sign byte + const d = toFixedWidth(keyPart(caKey, "d"), size) const cryptoKey = await crypto.subtle.importKey( - "pkcs8", - der, + "jwk", + { + kty: "EC", + crv: namedCurve, + d: base64url(d), + x: base64url(point.subarray(1, 1 + size)), + y: base64url(point.subarray(1 + size)), + }, { name: "ECDSA", namedCurve }, false, ["sign"], diff --git a/packages/core/tests/keys/ed25519.ts b/packages/core/tests/keys/ed25519.ts index 7b8079ad..904fd588 100644 --- a/packages/core/tests/keys/ed25519.ts +++ b/packages/core/tests/keys/ed25519.ts @@ -27,10 +27,24 @@ foOq9xilaxUIu1Rz3PzDAAAAAAECAwQF -----END OPENSSH PRIVATE KEY----- ` +// a CA key whose seed starts with 0x00 followed by a byte below 0x80, which +// sshpk writes to PKCS#8 with the leading zero dropped +export const leadingZeroCaKey = `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW +QyNTUxOQAAACCID/KNdIMcAOAkNTKYgXBmzI3Fz2Z7N1+Ea8AmcRHK7wAAAIi7BqbYuwam +2AAAAAtzc2gtZWQyNTUxOQAAACCID/KNdIMcAOAkNTKYgXBmzI3Fz2Z7N1+Ea8AmcRHK7w +AAAEAAPuqDE5ftCR+/QfVH5xhX8nyuNV2sKzm1vaP6oPAwyIgP8o10gxwA4CQ1MpiBcGbM +jcXPZns3X4RrwCZxEcrvAAAAAAECAwQF +-----END OPENSSH PRIVATE KEY----- +` + export const key = { ca(): PrivateKey { return parsePrivateKey(caKey) }, + leadingZeroCa(): PrivateKey { + return parsePrivateKey(leadingZeroCaKey) + }, host(): PrivateKey { return parsePrivateKey(hostKey) }, diff --git a/packages/core/tests/krl.test.ts b/packages/core/tests/krl.test.ts new file mode 100644 index 00000000..498a15a1 --- /dev/null +++ b/packages/core/tests/krl.test.ts @@ -0,0 +1,105 @@ +import { describe, it, expect } from "vitest" +import { generatePrivateKey, parsePrivateKey, type PrivateKey } from "sshpk" +import { KRLBuilder } from "../src/krl" +import { verify } from "../src/sshsig" +import { key as ecdsaKey } from "./keys/ecdsa" +import { key as ed25519Key } from "./keys/ed25519" + +const namespace = "krl@com.github.serverless-ssh-ca.andrewheberle" + +const signAndVerify = async (caKey: PrivateKey): Promise => { + const krl = new KRLBuilder(caKey).addSerials([1n, 2n]) + const bytes = new Uint8Array(krl.generate()) + return await verify(await krl.signature(), bytes, { namespace }) +} + +// ECDSA CA keys whose private key is shorter than the curve size, which sshpk +// writes to PKCS#8 without the required leading zero bytes +const shortEcdsaKeys: { curve: string, size: number, key: string }[] = [ + { + curve: "nistp256", + size: 32, + key: `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAaAAAABNlY2RzYS +1zaGEyLW5pc3RwMjU2AAAACG5pc3RwMjU2AAAAQQSTqJtmbzPJxBeEaaaGnBCJl7wy6NtE +gucGgLu0/H9TdWWhdb1ATAEi7NpIytWqz178Yc4e9R7qCpxykkv4Crw4AAAAmJlKqFWZSq +hVAAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBJOom2ZvM8nEF4Rp +poacEImXvDLo20SC5waAu7T8f1N1ZaF1vUBMASLs2kjK1arPXvxhzh71HuoKnHKSS/gKvD +gAAAAfIBQ9jXgJRzFedwzXtoCk+XghYBr6kCO2oEihGtZxyQAAAAAB +-----END OPENSSH PRIVATE KEY----- +`, + }, + { + curve: "nistp384", + size: 48, + key: `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAiAAAABNlY2RzYS +1zaGEyLW5pc3RwMzg0AAAACG5pc3RwMzg0AAAAYQTbbBWX/NJzq75VGHWNQJlGHA43OV4W +YWzSGVN3hbLT6660+s5Lsop3WeG3+nGp7hpB55TJ+7fcxGoFXrzRy5cdt+r+1gIxlr8W9o +327ZO+rpxhLJ5fgltB7c7XDb6NFSYAAADIVFzx4VRc8eEAAAATZWNkc2Etc2hhMi1uaXN0 +cDM4NAAAAAhuaXN0cDM4NAAAAGEE22wVl/zSc6u+VRh1jUCZRhwONzleFmFs0hlTd4Wy0+ +uutPrOS7KKd1nht/pxqe4aQeeUyfu33MRqBV680cuXHbfq/tYCMZa/FvaN9u2Tvq6cYSye +X4JbQe3O1w2+jRUmAAAAMACKte4SuHu5WQXjhfxewKIXVMxJT4OLpzjje0hd885k9ZVg8L +IbCMXdm1eIfOqDtQAAAAA= +-----END OPENSSH PRIVATE KEY----- +`, + }, + { + curve: "nistp521", + size: 66, + key: `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAArAAAABNlY2RzYS +1zaGEyLW5pc3RwNTIxAAAACG5pc3RwNTIxAAAAhQQAmxwB4z4xvHo6x22qQW8gqXLZedxP +xv0MPuJ2eIeC2SrKmKozzQXrPuXwNkA7ak0/3Db6/M3WQEgTJutRz+RiABsBmouo3Pn3XF +hbURUin00A5ChAj/fvLEQ4JSV6QS15xetE9FA1G0YsJNbqHZ7U+zFtQzg9ne2lsmCRo+wg +QYlPs5YAAAEAIfw0ryH8NK8AAAATZWNkc2Etc2hhMi1uaXN0cDUyMQAAAAhuaXN0cDUyMQ +AAAIUEAJscAeM+Mbx6OsdtqkFvIKly2XncT8b9DD7idniHgtkqypiqM80F6z7l8DZAO2pN +P9w2+vzN1kBIEybrUc/kYgAbAZqLqNz591xYW1EVIp9NAOQoQI/37yxEOCUlekEtecXrRP +RQNRtGLCTW6h2e1PsxbUM4PZ3tpbJgkaPsIEGJT7OWAAAAQgCJFNndYKQvGQlm6fTBGcMD +UpekFqxBDdqrBaePlCRSVbcA7dChkfZ/AfSZYyu9jfNlPwk4viJeWzxsKY80ABhXZQAAAA +ABAg== +-----END OPENSSH PRIVATE KEY----- +`, + }, +] + +describe("KRLBuilder signature", () => { + it.each([ + ["ECDSA CA", ecdsaKey.ca()], + ["Ed25519 CA", ed25519Key.ca()], + ["Ed25519 CA with a leading zero seed byte", ed25519Key.leadingZeroCa()], + ] as [string, PrivateKey][])("%s should produce a valid signature", async (_, caKey) => { + expect(await signAndVerify(caKey)).toBe(true) + }) + + it.each(shortEcdsaKeys)("ECDSA $curve CA with a short private key should produce a valid signature", async ({ key, size }) => { + const caKey = parsePrivateKey(key) + + // guards against the fixture being replaced with an ordinary key + const d = caKey.parts.find((p) => p.name === "d")?.data ?? [] + const start = d.findIndex((b) => b !== 0) + expect(d.length - start).toBeLessThan(size) + + expect(await signAndVerify(caKey)).toBe(true) + }) + + // covers every encoding sshpk produces for the private key: short, full + // width and with a leading sign byte + it.each(["nistp256", "nistp384", "nistp521"] as const)("should sign with generated ECDSA %s CA keys", async (curve) => { + const failures: string[] = [] + for (let i = 0; i < 100; i++) { + const caKey = generatePrivateKey("ecdsa", { curve }) + if (!(await signAndVerify(caKey))) { + failures.push(caKey.toString("openssh")) + } + } + expect(failures).toEqual([]) + }, 60000) + + it("leading zero seed fixture should match the problem pattern", () => { + // guards against the fixture being replaced with an ordinary key + const seed = ed25519Key.leadingZeroCa().parts.find((p) => p.name === "k")?.data + expect(seed?.[0]).toBe(0x00) + expect(seed?.[1]).toBeLessThan(0x80) + }) +}) diff --git a/packages/workers-integration/tests/ed25519-ca.test.ts b/packages/workers-integration/tests/ed25519-ca.test.ts new file mode 100644 index 00000000..48a918c5 --- /dev/null +++ b/packages/workers-integration/tests/ed25519-ca.test.ts @@ -0,0 +1,27 @@ +import { env, exports } from "cloudflare:workers" +import { + adminSecretsStore, + // @ts-ignore: this import errors but is fine in tests +} from "cloudflare:test" +import { describe, it, expect } from "vitest" +import { leadingZeroPrivateKeyString } from "./keys/ed25519" + +// an Ed25519 CA key that sshpk cannot export as valid PKCS#8 must still be +// able to sign KRLs +const admin = adminSecretsStore(env.PRIVATE_KEY) +await admin.create(leadingZeroPrivateKeyString) + +describe("Ed25519 CA key with a leading zero seed byte", () => { + it.each([ + "/api/v3/user/krl", + "/api/v3/host/krl", + ])("GET %s responds with a signed KRL", async (path) => { + const response = await exports.default.fetch(`http://example.com${path}`) + + expect(response.status).toBe(200) + expect(await response.json()).toMatchObject({ + krl: expect.any(String), + signature: expect.stringContaining("-----BEGIN SSH SIGNATURE-----"), + }) + }) +}) diff --git a/packages/workers-integration/tests/keys/ed25519.ts b/packages/workers-integration/tests/keys/ed25519.ts index ee71f801..61dbde8e 100644 --- a/packages/workers-integration/tests/keys/ed25519.ts +++ b/packages/workers-integration/tests/keys/ed25519.ts @@ -25,3 +25,14 @@ gKLq7bj9fYWLuUXmvsaHAAAAEHVzZXJAZXhhbXBsZS5jb20BAgMEBQ== export const userPrivateKey = (): PrivateKey => { return parsePrivateKey(userPrivateKeyString) } + +// a CA key whose seed starts with 0x00 followed by a byte below 0x80, which +// sshpk writes to PKCS#8 with the leading zero dropped +export const leadingZeroPrivateKeyString = `-----BEGIN OPENSSH PRIVATE KEY----- +b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtzc2gtZW +QyNTUxOQAAACCID/KNdIMcAOAkNTKYgXBmzI3Fz2Z7N1+Ea8AmcRHK7wAAAIi7BqbYuwam +2AAAAAtzc2gtZWQyNTUxOQAAACCID/KNdIMcAOAkNTKYgXBmzI3Fz2Z7N1+Ea8AmcRHK7w +AAAEAAPuqDE5ftCR+/QfVH5xhX8nyuNV2sKzm1vaP6oPAwyIgP8o10gxwA4CQ1MpiBcGbM +jcXPZns3X4RrwCZxEcrvAAAAAAECAwQF +-----END OPENSSH PRIVATE KEY----- +`