fix(bug): work around bugs in sshpk

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<ArrayBuffer> {
-    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"],

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)
     },

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<boolean> => {
+	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)
+	})
+})

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-----"),
+		})
+	})
+})

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-----
+`