From 46bebee72f4b92e200a7f8bd297023b689922a54 Mon Sep 17 00:00:00 2001 From: Bryan Thompson Date: Wed, 25 Mar 2026 08:06:30 -0500 Subject: [PATCH] feat: add fix-signature subcommand for externally signed bundles Enterprise HSM signers (GaraSign, SignServer, Venafi, Azure SignTool) append PKCS#7 signatures after the ZIP EOCD without updating comment_length. Claude Desktop's adm-zip parser rejects these files. PR #204 fixed this for `mcpb sign` but external signers remain broken. This adds `mcpb fix-signature` which applies the same EOCD fix post-hoc, so enterprises can add one step to their signing pipeline. Validated end-to-end with Autodesk's GaraSign-signed Fusion MCPB: - adm-zip, unzip, Python zipfile all accept the fixed bundle - Claude Desktop installs it successfully - Signature bytes (MCPB_SIG_V1 + PKCS#7 + MCPB_SIG_END) remain intact - Exactly 2 bytes changed (EOCD comment_length field) Relates to #204, #21, #194 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/cli/cli.ts | 35 ++++++++++++++++++- src/node/sign.ts | 46 ++++++++++++++++++++++++ test/sign.e2e.test.ts | 81 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 1 deletion(-) diff --git a/src/cli/cli.ts b/src/cli/cli.ts index 817b659..eb34da6 100644 --- a/src/cli/cli.ts +++ b/src/cli/cli.ts @@ -6,7 +6,12 @@ import { existsSync, readFileSync, statSync } from "fs"; import { basename, dirname, join, resolve } from "path"; import { fileURLToPath } from "url"; -import { signMcpbFile, unsignMcpbFile, verifyMcpbFile } from "../node/sign.js"; +import { + fixSignatureMcpbFile, + signMcpbFile, + unsignMcpbFile, + verifyMcpbFile, +} from "../node/sign.js"; import { cleanMcpb, validateManifest } from "../node/validate.js"; import { initExtension } from "./init.js"; import { packExtension } from "./pack.js"; @@ -355,5 +360,33 @@ program } }); +// Fix-signature command (for externally signed bundles) +program + .command("fix-signature ") + .description( + "Fix ZIP EOCD comment_length for externally signed MCPB bundles (e.g. GaraSign, SignServer)", + ) + .action((mcpbFile: string) => { + try { + const mcpbPath = resolve(mcpbFile); + + if (!existsSync(mcpbPath)) { + console.error(`ERROR: MCPB file not found: ${mcpbFile}`); + process.exit(1); + } + + console.log(`Fixing signature for ${basename(mcpbPath)}...`); + fixSignatureMcpbFile(mcpbPath); + console.log( + `Signature fixed — bundle is now installable in Claude Desktop`, + ); + } catch (error) { + console.error( + `ERROR: ${error instanceof Error ? error.message : "Unknown error"}`, + ); + process.exit(1); + } + }); + // Parse command line arguments program.parse(); diff --git a/src/node/sign.ts b/src/node/sign.ts index bd4c8c4..b70731b 100644 --- a/src/node/sign.ts +++ b/src/node/sign.ts @@ -426,6 +426,52 @@ export async function verifyCertificateChain( } } +/** + * Fixes the ZIP EOCD comment_length for an externally signed MCPB file. + * + * External signing tools (GaraSign, SignServer, Venafi, etc.) append a + * PKCS#7 signature block after the ZIP EOCD without updating comment_length. + * Strict ZIP parsers like adm-zip (used by Claude Desktop) reject these files. + * This function sets comment_length to encompass the trailing signature bytes, + * making the file a spec-valid ZIP while preserving the signature intact. + * + * @param mcpbPath Path to the signed MCPB file to fix + */ +export function fixSignatureMcpbFile(mcpbPath: string): void { + const fileContent = readFileSync(mcpbPath); + + // Verify this file has a signature block + const { pkcs7Signature } = extractSignatureBlock(fileContent); + if (!pkcs7Signature) { + throw new Error("File is not signed — nothing to fix"); + } + + // Find the EOCD record + const eocdOffset = findEocdOffset(fileContent); + if (eocdOffset === -1) { + throw new Error("No ZIP end-of-central-directory record found"); + } + + const currentCommentLength = fileContent.readUInt16LE(eocdOffset + 20); + const actualTrailing = fileContent.length - (eocdOffset + 22); + + if (currentCommentLength === actualTrailing) { + console.log("EOCD comment_length already correct — no fix needed"); + return; + } + + if (actualTrailing > 65535) { + throw new Error( + `Signature block too large for ZIP comment field (${actualTrailing} > 65535)`, + ); + } + + // Apply the fix — same approach as signMcpbFile() from PR #204 + const updatedContent = Buffer.from(fileContent); + updatedContent.writeUInt16LE(actualTrailing, eocdOffset + 20); + writeFileSync(mcpbPath, updatedContent); +} + /** * Removes signature from a MCPB file */ diff --git a/test/sign.e2e.test.ts b/test/sign.e2e.test.ts index 9af52a0..f0ca6ac 100755 --- a/test/sign.e2e.test.ts +++ b/test/sign.e2e.test.ts @@ -14,6 +14,8 @@ import forge from "node-forge"; import * as path from "path"; import { + extractSignatureBlock, + fixSignatureMcpbFile, signMcpbFile, unsignMcpbFile, verifyMcpbFile, @@ -514,4 +516,83 @@ describe("MCPB Signing E2E Tests", () => { // Clean up fs.unlinkSync(testFile); }); + + it("should fix externally-signed bundle with incorrect EOCD comment_length", () => { + // Simulate an external signer (e.g. GaraSign): sign normally, then + // reset comment_length to 0 as if the signer didn't update the EOCD + const testFile = path.join(TEST_DIR, "test-fix-external.mcpb"); + fs.copyFileSync(TEST_MCPB, testFile); + signMcpbFile(testFile, SELF_SIGNED_CERT, SELF_SIGNED_KEY); + + // Read signed file and zero out comment_length to simulate external signer + const signedContent = fs.readFileSync(testFile); + let eocdOffset = -1; + for (let i = signedContent.length - 22; i >= 0; i--) { + if (signedContent.readUInt32LE(i) === 0x06054b50) { + eocdOffset = i; + break; + } + } + expect(eocdOffset).toBeGreaterThanOrEqual(0); + const brokenContent = Buffer.from(signedContent); + brokenContent.writeUInt16LE(0, eocdOffset + 20); // Zero out comment_length + fs.writeFileSync(testFile, brokenContent); + + // Verify it's broken (comment_length doesn't match trailing bytes) + const beforeFix = fs.readFileSync(testFile); + expect(beforeFix.readUInt16LE(eocdOffset + 20)).toBe(0); + + // Fix the signature + fixSignatureMcpbFile(testFile); + + // Verify comment_length is now correct + const afterFix = fs.readFileSync(testFile); + const actualTrailing = afterFix.length - (eocdOffset + 22); + expect(afterFix.readUInt16LE(eocdOffset + 20)).toBe(actualTrailing); + expect(actualTrailing).toBeGreaterThan(0); + + // Verify signature block is still intact + const { pkcs7Signature } = extractSignatureBlock(afterFix); + expect(pkcs7Signature).toBeDefined(); + + // Clean up + fs.unlinkSync(testFile); + }); + + it("should no-op on bundle already fixed by mcpb sign", () => { + // Sign normally (mcpb sign already fixes EOCD) + const testFile = path.join(TEST_DIR, "test-fix-noop.mcpb"); + fs.copyFileSync(TEST_MCPB, testFile); + signMcpbFile(testFile, SELF_SIGNED_CERT, SELF_SIGNED_KEY); + + // Record file content before fix attempt + const beforeContent = fs.readFileSync(testFile); + + // fix-signature should be a no-op (logs "already correct") + const consoleSpy = jest.spyOn(console, "log").mockImplementation(); + fixSignatureMcpbFile(testFile); + expect(consoleSpy).toHaveBeenCalledWith( + "EOCD comment_length already correct — no fix needed", + ); + consoleSpy.mockRestore(); + + // File should be unchanged + const afterContent = fs.readFileSync(testFile); + expect(afterContent.equals(beforeContent)).toBe(true); + + // Clean up + fs.unlinkSync(testFile); + }); + + it("should throw on unsigned file", () => { + const testFile = path.join(TEST_DIR, "test-fix-unsigned.mcpb"); + fs.copyFileSync(TEST_MCPB, testFile); + + expect(() => fixSignatureMcpbFile(testFile)).toThrow( + "File is not signed", + ); + + // Clean up + fs.unlinkSync(testFile); + }); });