From 0fd8b341b9278190d6dc78b06ab515545d7ecf9a Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 13 Oct 2026 22:17:19 +0000 Subject: [PATCH] Sign or timestamp an unchanged tree's manifest in freshen (closes #190) freshen skipped the write whenever no file was added, changed or removed, so --sign-key and --include-timestamps on an unchanged tree did nothing. It now skips only when the manifest would come out the same: nothing in the tree changed, the manifest is signed by the --sign-key key (unsigned without one), and it has a createdAt timestamp exactly when --include-timestamps asks for one. A file whose recorded mode is not what --include-permissions asks for already counts as changed. Deviation: the mfer package gains SecretKeyFingerprint and a loaded manifest's CreatedAt, which the comparison needs. Model: opus-5-5 --- internal/cli/freshen.go | 59 +++++++++++++++++++++++++----------- internal/cli/freshen_test.go | 52 +++++++++++++++++++++++++++++++ internal/cli/signing_test.go | 47 ++++++++++++++++++++++++++++ mfer/manifest.go | 6 ++++ mfer/openpgp.go | 13 ++++++++ mfer/openpgp_test.go | 15 +++++++++ 6 files changed, 175 insertions(+), 17 deletions(-) diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index b071a4b..ec74eb3 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -365,29 +365,39 @@ func writeFreshenedManifest( } // newFreshenBuilder constructs the manifest builder configured from CLI -// flags. A --sign-key given with an empty value is refused. -func (mfa *CLIApp) newFreshenBuilder(cmd *cli.Command) (*mfer.Builder, error) { +// flags, and returns the fingerprint of the --sign-key key, or "" when +// none is given. A --sign-key given with an empty value is refused. +func (mfa *CLIApp) newFreshenBuilder( + cmd *cli.Command, +) (*mfer.Builder, string, error) { builder := mfer.NewBuilder() if cmd.Bool("include-timestamps") { builder.SetIncludeTimestamps(true) } + var signKeyFingerprint string + // Set up signing options if sign-key is provided if cmd.IsSet("sign-key") { signKey := cmd.String("sign-key") if signKey == "" { - return nil, errEmptySignKey + return nil, "", errEmptySignKey } signing, err := mfa.signingOptions(signKey) if err != nil { - return nil, err + return nil, "", err + } + + signKeyFingerprint, err = mfer.SecretKeyFingerprint(signing.SecretKey) + if err != nil { + return nil, "", fmt.Errorf("%s: %w", signKey, err) } builder.SetSigningOptions(signing) } - return builder, nil + return builder, signKeyFingerprint, nil } // freshenScan runs the scan phase against the loaded manifest entries @@ -499,12 +509,19 @@ func runFreshenHash( return nil } -// loadExistingEntries loads the manifest and indexes its file entries +// existingManifest is what freshen uses of the manifest it rewrites. +type existingManifest struct { + entriesByPath map[string]*mfer.MFFilePath + signer string // fingerprint of the key it is signed with, "" if unsigned + hasCreatedAt bool +} + +// loadExistingManifest loads the manifest and indexes its file entries // by path. It refuses a signed manifest when no --sign-key is given, as // the rewritten manifest would be unsigned. -func (mfa *CLIApp) loadExistingEntries( +func (mfa *CLIApp) loadExistingManifest( cmd *cli.Command, manifestPath string, -) (map[string]*mfer.MFFilePath, error) { +) (*existingManifest, error) { log.Infof("loading manifest from %s", manifestPath) // Load existing manifest @@ -529,7 +546,11 @@ func (mfa *CLIApp) loadExistingEntries( existingByPath[f.GetPath()] = f } - return existingByPath, nil + return &existingManifest{ + entriesByPath: existingByPath, + signer: string(manifest.Signer()), + hasCreatedAt: manifest.CreatedAt() != nil, + }, nil } func (mfa *CLIApp) freshenManifestOperation( @@ -537,20 +558,18 @@ func (mfa *CLIApp) freshenManifestOperation( ) error { log.Debug("freshenManifestOperation()") - showProgress := cmd.Bool("progress") - // Find manifest file manifestPath, err := mfa.resolveFreshenManifestPath(cmd) if err != nil { return err } - builder, err := mfa.newFreshenBuilder(cmd) + builder, signKeyFingerprint, err := mfa.newFreshenBuilder(cmd) if err != nil { return err } - existingByPath, err := mfa.loadExistingEntries(cmd, manifestPath) + existing, err := mfa.loadExistingManifest(cmd, manifestPath) if err != nil { return err } @@ -562,7 +581,7 @@ func (mfa *CLIApp) freshenManifestOperation( // Phase 1: Scan filesystem scanner, removed, err := mfa.freshenScan(cmd, manifestPath, absBase, - existingByPath) + existing.entriesByPath) if err != nil { return err } @@ -579,7 +598,7 @@ func (mfa *CLIApp) freshenManifestOperation( hasher := &freshenHasher{ fs: mfa.Fs, absBase: absBase, - showProgress: showProgress, + showProgress: cmd.Bool("progress"), totalHashBytes: totalHashBytes, filesToHash: filesToHash, startHash: time.Now(), @@ -595,8 +614,14 @@ func (mfa *CLIApp) freshenManifestOperation( log.Infof("freshen complete: %d unchanged, %d changed, %d added, %d removed", scanner.unchanged, scanner.changed, scanner.added, removed) - // Skip writing if nothing changed - if scanner.changed == 0 && scanner.added == 0 && removed == 0 { + // Skip writing only when the manifest would come out the same: nothing + // in the tree changed, and the manifest already holds what --sign-key, + // --include-timestamps and --include-permissions ask for. The scan + // counts a file whose recorded mode is not what --include-permissions + // asks for as changed. + if scanner.changed == 0 && scanner.added == 0 && removed == 0 && + existing.signer == signKeyFingerprint && + existing.hasCreatedAt == cmd.Bool("include-timestamps") { log.Infof("manifest unchanged, skipping write") return nil diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index 02b41df..919457f 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -84,6 +84,23 @@ func manifestFiles(t *testing.T, fs afero.Fs, path string) []*mfer.MFFilePath { return manifest.Files() } +// manifestCreatedAt returns the createdAt timestamp of the manifest at +// path, or nil if it has none. +func manifestCreatedAt(t *testing.T, fs afero.Fs, path string) *mfer.Timestamp { + t.Helper() + + runMu.Lock() + defer runMu.Unlock() + + manifest, err := mfer.NewManifestFromFile(&mfer.ManifestFromFileOptions{ + Path: path, + Fs: fs, + }) + require.NoError(t, err) + + return manifest.CreatedAt() +} + // TestFreshenUnchanged freshens a tree that has not changed since gen // made its manifest: the manifest must list the same entries as before. func TestFreshenUnchanged(t *testing.T) { @@ -141,6 +158,41 @@ func TestFreshenRecordsModeOnlyWhenAsked(t *testing.T) { recordedModes()) } +// TestFreshenIncludeTimestampsOnUnchangedTree runs freshen +// --include-timestamps on a tree that has not changed since gen made its +// manifest, which has no createdAt timestamp. freshen must rewrite the +// manifest with one. Run again with the flag it must leave the manifest as +// it is; run without it, it must rewrite the manifest without one. +func TestFreshenIncludeTimestampsOnUnchangedTree(t *testing.T) { + t.Parallel() + + fs := afero.NewOsFs() + root, manifestPath := setupFreshenDir(t, fs, map[string]string{testFileTxt: "hello"}) + + freshenWithTimestamps := func() { + opts := testOpts([]string{ + testApp, cmdFreshen, "-q", "--include-timestamps", + testFlagBase, root, manifestPath, + }, fs) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + } + + freshenWithTimestamps() + assert.NotNil(t, manifestCreatedAt(t, fs, manifestPath)) + + before, err := afero.ReadFile(fs, manifestPath) + require.NoError(t, err) + + freshenWithTimestamps() + + after, err := afero.ReadFile(fs, manifestPath) + require.NoError(t, err) + assert.Equal(t, before, after, "manifest that has a createdAt was rewritten") + + runFreshen(t, fs, root, manifestPath) + assert.Nil(t, manifestCreatedAt(t, fs, manifestPath)) +} + // assertManifestLists asserts that the manifest at manifestPath lists // exactly the files in want, each with the size and SHA-256 hash of its // content in want and the mtime of the file of that name under root. diff --git a/internal/cli/signing_test.go b/internal/cli/signing_test.go index f1d1efc..9ebca33 100644 --- a/internal/cli/signing_test.go +++ b/internal/cli/signing_test.go @@ -127,6 +127,53 @@ func TestFreshenSignedManifestNeedsSignKey(t *testing.T) { assert.Len(t, manifestFiles(t, fs, manifestPath), 2) } +// TestFreshenSignsUnchangedTree runs freshen with --sign-key on a tree that +// has not changed since gen made its unsigned manifest. freshen must +// rewrite the manifest signed by that key. Run again with the same key it +// must leave the manifest as it is; with another key it must rewrite it +// signed by that one. +func TestFreshenSignsUnchangedTree(t *testing.T) { + t.Setenv(envSignKeyPassphrase, "") + + fs := afero.NewOsFs() + _, manifestPath := setupFreshenDir(t, fs, map[string]string{testFileTxt: "hello"}) + + newKeyFile := func() (string, string) { + secretKey, fingerprint := testSecretKey(t, nil, nil) + keyFile := filepath.Join(t.TempDir(), "key.asc") + require.NoError(t, afero.WriteFile(fs, keyFile, secretKey, 0o600)) + + return keyFile, fingerprint + } + + freshenAndCheckSignedBy := func(keyFile, fingerprint string) { + opts := testOpts([]string{ + testApp, cmdFreshen, "-q", testFlagSignKey, keyFile, manifestPath, + }, fs) + require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + + opts = testOpts([]string{ + testApp, cmdCheck, "-q", + "--" + flagRequireSignature, fingerprint, manifestPath, + }, fs) + require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + } + + keyFile, fingerprint := newKeyFile() + freshenAndCheckSignedBy(keyFile, fingerprint) + + before, err := afero.ReadFile(fs, manifestPath) + require.NoError(t, err) + + freshenAndCheckSignedBy(keyFile, fingerprint) + + after, err := afero.ReadFile(fs, manifestPath) + require.NoError(t, err) + assert.Equal(t, before, after, "manifest already signed by the key was rewritten") + + freshenAndCheckSignedBy(newKeyFile()) +} + // TestSignWithProtectedKeyNeedsPassphrase runs gen with a protected key, // with MFER_SIGN_KEY_PASSPHRASE empty and no terminal to ask on. gen must // fail, naming the variable, and write no manifest. diff --git a/mfer/manifest.go b/mfer/manifest.go index 88527a6..2730612 100644 --- a/mfer/manifest.go +++ b/mfer/manifest.go @@ -58,6 +58,12 @@ func (m *manifest) Signer() []byte { return m.pbOuter.GetSigner() } +// CreatedAt returns the createdAt timestamp of a loaded manifest, or nil +// if the manifest has none. +func (m *manifest) CreatedAt() *Timestamp { + return m.pbInner.GetCreatedAt() +} + // signatureString generates the canonical string used for signing/verification. // Format: MAGIC-UUID-MULTIHASH where UUID and multihash are hex-encoded. // Requires pbOuter to be set with Uuid and Sha256 fields. diff --git a/mfer/openpgp.go b/mfer/openpgp.go index 37ef5aa..758badf 100644 --- a/mfer/openpgp.go +++ b/mfer/openpgp.go @@ -83,6 +83,19 @@ func SecretKeyIsProtected(secretKey []byte) (bool, error) { return isProtected(key), nil } +// SecretKeyFingerprint returns the fingerprint of the OpenPGP secret key +// secretKey, armored or binary, as a manifest signed with it records its +// signer. It fails unless secretKey holds one version 4 primary key with +// its secret key. +func SecretKeyFingerprint(secretKey []byte) (string, error) { + key, err := readSecretKey(secretKey) + if err != nil { + return "", err + } + + return fingerprint(key), nil +} + // CheckSigningKey fails unless opts can sign now: opts.SecretKey must hold // one version 4 primary key with a secret key that may sign and has not // expired or been revoked, and opts.Passphrase must unlock it when it is diff --git a/mfer/openpgp_test.go b/mfer/openpgp_test.go index 1dc22dd..cb97c32 100644 --- a/mfer/openpgp_test.go +++ b/mfer/openpgp_test.go @@ -281,6 +281,21 @@ func TestSecretKeyIsProtectedWithoutPassphrase(t *testing.T) { assert.False(t, protected) } +// TestSecretKeyFingerprint checks that SecretKeyFingerprint gives the +// signer that a manifest signed with the key records. +func TestSecretKeyFingerprint(t *testing.T) { + t.Parallel() + + opts := testSigningOptions(t) + + got, err := SecretKeyFingerprint(opts.SecretKey) + require.NoError(t, err) + + m, err := NewManifestFromReader(bytes.NewReader(signedTestManifest(t, opts))) + require.NoError(t, err) + assert.Equal(t, string(m.Signer()), got) +} + // TestSigningKeyFileWithTwoKeys signs with a key file that holds two keys, // as gpg writes it for a user ID that two keys have. It names no one key // to sign with, so signing must fail. -- 2.54.0