diff --git a/script/command-executor.ts b/script/command-executor.ts index a050034..91e4754 100644 --- a/script/command-executor.ts +++ b/script/command-executor.ts @@ -215,6 +215,11 @@ function appRename(command: cli.IAppRenameCommand): Promise { }); } +/** Must match the base packageFileFromPath zips with, or the signature covers keys the package lacks. */ +export function signatureManifestBase(filePath: string): string { + return path.dirname(filePath); +} + export function resolvePrivateKey(value: string): string { return value.trimStart().startsWith("-----BEGIN") ? value // inline PEM content @@ -1266,7 +1271,7 @@ export const release = (command: cli.IReleaseCommand): Promise => { map.set(path.basename(filePath), fileHash); return new hashUtils.PackageManifest(map).computePackageHash(); }) - : hashUtils.generatePackageHashFromDirectory(filePath, path.join(filePath, "..")); + : hashUtils.generatePackageHashFromDirectory(filePath, signatureManifestBase(filePath)); return hashPromise.then((packageHash: string) => createRS256JWT(privateKey, packageHash)); }; diff --git a/test/cli.ts b/test/cli.ts index 3239314..86006c3 100644 --- a/test/cli.ts +++ b/test/cli.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import * as assert from "assert"; +import * as crypto from "crypto"; import * as fs from "fs"; import * as sinon from "sinon"; import Q = require("q"); @@ -9,6 +10,7 @@ import * as path from "path"; import * as codePush from "../script/types"; import * as cli from "../script/types/cli"; import * as cmdexec from "../script/command-executor"; +import * as hashUtils from "../script/hash-utils"; import * as os from "os"; import moment = require("moment"); @@ -223,6 +225,10 @@ export class SdkStub { return Q(null); } + public isAuthenticated(): Q.Promise { + return Q(true); + } + public release(): Q.Promise { return Q("Successfully released"); } @@ -1798,6 +1804,49 @@ describe("CLI", () => { .done(); }); + it("signs a directory release against the base the package is zipped with", (done: Mocha.Done): void => { + // The call site, not the helper: releasing "." used to sign keys the zip did not contain, and still + // reported success. + const { privateKey } = crypto.generateKeyPairSync("rsa", { + modulusLength: 2048, + privateKeyEncoding: { type: "pkcs8", format: "pem" }, + publicKeyEncoding: { type: "spki", format: "pem" }, + }); + + const bases: string[] = []; + sandbox.stub(hashUtils, "generatePackageHashFromDirectory").callsFake((dir: string, base: string) => { + bases.push(base); + return Q("deadbeef"); + }); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "dpctl-sign-")); + fs.writeFileSync(path.join(dir, "main.jsbundle"), "bundle contents"); + const previousCwd = process.cwd(); + process.chdir(dir); + + const finish = (error?: any): void => { + process.chdir(previousCwd); + fs.rmSync(dir, { recursive: true, force: true }); + if (error) return done(error); + assert.deepStrictEqual(bases, [path.dirname(".")], "must key the manifest the way the package is zipped"); + done(); + }; + + cmdexec + .execute({ + type: cli.CommandType.release, + appName: "a", + deploymentName: "Staging", + description: "", + mandatory: false, + rollout: null, + appStoreVersion: "1.0.0", + package: ".", + privateKey, + }) + .done(() => finish(), finish); + }); + function releaseHelperFunction(command: cli.IReleaseCommand, done: Mocha.Done, expectedError: string): void { cmdexec.execute(command).done( (): void => { diff --git a/test/hash-utils.ts b/test/hash-utils.ts index 9b7dc8d..b5ae634 100644 --- a/test/hash-utils.ts +++ b/test/hash-utils.ts @@ -5,6 +5,7 @@ import * as assert from "assert"; import * as crypto from "crypto"; import * as fs from "fs"; import * as hashUtils from "../script/hash-utils"; +import * as cmdexec from "../script/command-executor"; var { mkdirp } = require("mkdirp"); import * as os from "os"; import * as path from "path"; @@ -205,4 +206,74 @@ describe("Hashing utility", () => { }); }); }); + // --------------------------------------------------------------------------- + // The signed hash must be keyed the same way the zip is, or the signature covers keys the package does + // not contain. path.join(dir, "..") and path.dirname(dir) differ only for "." and "./", which is + // exactly what `dpctl release MyApp . 1.0.0` passes. + // --------------------------------------------------------------------------- + + describe("signed manifest base path", () => { + let workDir: string; + let previousCwd: string; + + beforeEach(() => { + workDir = fs.mkdtempSync(path.join(os.tmpdir(), "dpctl-hash-base-")); + fs.writeFileSync(path.join(workDir, "main.jsbundle"), "bundle contents"); + previousCwd = process.cwd(); + process.chdir(workDir); + }); + + afterEach(() => { + process.chdir(previousCwd); + fs.rmSync(workDir, { recursive: true, force: true }); + }); + + it('keys the manifest the way the zip does when the release path is "."', (done) => { + hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) => { + const keys = Array.from(manifest.toMap().keys()); + assert.deepStrictEqual(keys, ["main.jsbundle"], `got ${JSON.stringify(keys)}`); + done(); + }, done); + }); + + it("the CLI keys the signed manifest against the base the SDK zips with", () => { + assert.strictEqual(cmdexec.signatureManifestBase("."), path.dirname(".")); + assert.strictEqual(cmdexec.signatureManifestBase("."), "."); + assert.strictEqual(cmdexec.signatureManifestBase("/tmp/build"), "/tmp"); + assert.notStrictEqual(cmdexec.signatureManifestBase("."), path.join(".", "..")); + }); + + it('path.join(dir, "..") is NOT path.dirname(dir) for "."', () => { + // The test above only bites while these disagree. + assert.strictEqual(path.dirname("."), "."); + assert.strictEqual(path.join(".", ".."), ".."); + assert.notStrictEqual(path.join(".", ".."), path.dirname(".")); + }); + + it('signs a hash that matches the zip when the release path is "."', (done) => { + // The base the CLI actually passes, not a copy of it. + const signed = hashUtils.generatePackageHashFromDirectory(".", cmdexec.signatureManifestBase(".")); + const onDevice = hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) => + manifest.computePackageHash() + ); + + q.all([signed, onDevice]).then((hashes: string[]) => { + assert.strictEqual(hashes[0], hashes[1], "the signed hash must match what the zip implies"); + done(); + }, done); + }); + + it('would not have matched with the old path.join(dir, "..") base', (done) => { + // Keeps the fix load-bearing rather than cosmetic. + const buggy = hashUtils.generatePackageHashFromDirectory(".", path.join(".", "..")); + const onDevice = hashUtils.generatePackageManifestFromDirectory(".", path.dirname(".")).then((manifest: PackageManifest) => + manifest.computePackageHash() + ); + + q.all([buggy, onDevice]).then((hashes: string[]) => { + assert.notStrictEqual(hashes[0], hashes[1], "the old base produced a hash that verified nowhere"); + done(); + }, done); + }); + }); });