From 656ecfbdd532239d8c83b5fa5db5aa210a26e98d Mon Sep 17 00:00:00 2001 From: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> Date: Tue, 27 Oct 2026 20:39:10 +0300 Subject: [PATCH] Fail closed when the home policy signature cannot be trusted verifyHomePolicy() only rejected a home policy.json when VerifyDocument reported signed && !valid. Every other outcome of the trust check fell through to "no verification required" and loaded the file: - a signature that could not be decoded (verr != nil) was ignored; - a machine key that existed but could not be loaded (kerr != nil) skipped the check entirely; - a stripped signature (signed == false) was treated as a legacy file even on a machine that had already signed policies; - a corrupt file was skipped, so the loader fell back to the project-local policy, which carries no Security fields at all. Each of those let someone who can write policy.json - or merely strip its signature - have an untrusted policy enforced, with the guard's own checks disabled through the missing Security block. The loader now applies one accepting rule on a keyed machine: a signature that verifies against this machine's key. Anything else returns ErrPolicyTampered, which guard/main.go already turns into a deny plus an audit record. Loading no longer creates the signing key. verifyHomePolicy used to call LoadOrCreateKeyPair on every load, so a single run keyed the install; an upgrade that still had a pre-signature, unsigned policy.json then rejected that same policy on its second run. With no key pair present an unsigned file is treated as pre-signature and loads, while a signed one is refused because there is nothing here to check it against. Writes still create the key pair through localstore, which is where signing belongs. Signed-off-by: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> --- packages/crypto/crypto_test.go | 14 +++- packages/crypto/keys.go | 14 ++++ packages/guard/guardcore/config.go | 62 +++++++++++++++--- packages/guard/guardcore/config_test.go | 86 +++++++++++++++++++++++-- 4 files changed, 159 insertions(+), 17 deletions(-) diff --git a/packages/crypto/crypto_test.go b/packages/crypto/crypto_test.go index db5a661..6b31c1f 100644 --- a/packages/crypto/crypto_test.go +++ b/packages/crypto/crypto_test.go @@ -176,4 +176,16 @@ func TestFingerprintIsStableAndDistinct(t *testing.T) { if Fingerprint(kp1.Public) == Fingerprint(kp2.Public) { t.Fatal("fingerprint should differ between distinct keys") } -} \ No newline at end of file +} +func TestHasKeyPair(t *testing.T) { + dir := t.TempDir() + if HasKeyPair(dir) { + t.Error("HasKeyPair = true on an empty directory, want false") + } + if _, err := LoadOrCreateKeyPair(dir); err != nil { + t.Fatal(err) + } + if !HasKeyPair(dir) { + t.Error("HasKeyPair = false after LoadOrCreateKeyPair created the pair, want true") + } +} diff --git a/packages/crypto/keys.go b/packages/crypto/keys.go index 343e3a4..481eefa 100644 --- a/packages/crypto/keys.go +++ b/packages/crypto/keys.go @@ -38,6 +38,20 @@ func GenerateKeyPair() (KeyPair, error) { return KeyPair{Public: pub, Private: priv}, nil } +// HasKeyPair reports whether this machine already has a signing key pair in +// dir. Callers use it to tell a first run without a signing identity (where an +// unsigned document can only be legacy) from a machine that has signed before +// (where an unsigned document has had its signature stripped). +func HasKeyPair(dir string) bool { + if _, err := os.Stat(filepath.Join(dir, privateKeyFile)); err != nil { + return false + } + if _, err := os.Stat(filepath.Join(dir, publicKeyFile)); err != nil { + return false + } + return true +} + // LoadOrCreateKeyPair reads the machine signing key from dir, generating and // persisting a new one on first use. The private key is written with 0600 // permissions; the public key with 0644 so it can be read for verification diff --git a/packages/guard/guardcore/config.go b/packages/guard/guardcore/config.go index 453a871..96bded7 100644 --- a/packages/guard/guardcore/config.go +++ b/packages/guard/guardcore/config.go @@ -6,6 +6,7 @@ package guardcore import ( "encoding/json" "errors" + "fmt" "os" "path/filepath" @@ -30,14 +31,58 @@ func securityInsidePolicy(doc []byte) (*shared.Security, bool) { return &sec, true } -// ErrPolicyTampered is returned when the home policy.json carries a -// signature that fails verification against this machine's recorded key - -// a direct rewrite that bypassed SolonGate entirely. Project-local policy -// files are never signature-checked: LoadLocalPolicyFile already strips -// their Security/SelfProtect fields below, so they carry no trust to forge. +// ErrPolicyTampered is returned when the home policy.json cannot be shown to be +// the file SolonGate wrote: the signature is missing on a machine that has a +// signing key, does not verify against this machine's key, cannot be decoded, +// or cannot be checked at all. Every one of those cases used to fall through to +// "no verification required" and load the file anyway, which let anyone who +// could rewrite policy.json - or merely strip its signature - have an +// untrusted policy enforced. Project-local policy files are never +// signature-checked: LoadLocalPolicyFile already strips their Security/ +// SelfProtect fields below, so they carry no trust to forge. var ErrPolicyTampered = errors.New("policy.json failed signature verification: " + "it was modified outside SolonGate; refusing to trust it") +// verifyHomePolicy fails closed: the only outcomes that let the home policy be +// applied are a signature that verifies against this machine's key, or an +// unsigned file on a machine that has no signing identity yet - a policy written +// before signatures existed. A key that exists but cannot be loaded, a signature +// that cannot be decoded, a signature that does not verify, and a stripped +// signature on a keyed machine are all treated as tampering. +func verifyHomePolicy(b []byte) error { + dir := filepath.Join(shared.SGDir(), "crypto") + if !pcrypto.HasKeyPair(dir) { + // Reading a policy must not create keys. LoadOrCreateKeyPair would make + // the first run key this install, and then its own unsigned policy would + // be rejected on the second run. + _, signed, _, verr := pcrypto.VerifyDocument(b, nil) + switch { + case signed: + // Signed by something, and there is no key here to check it against. + // That is not a legacy file, so it is not ours. + return fmt.Errorf("%w (it carries a signature, but this machine has no signing key to check it with)", ErrPolicyTampered) + case verr != nil: + return fmt.Errorf("%w (it is not a readable policy document: %v)", ErrPolicyTampered, verr) + } + return nil + } + + kp, err := pcrypto.LoadOrCreateKeyPair(dir) + if err != nil { + return fmt.Errorf("%w (the machine signing key could not be loaded: %v)", ErrPolicyTampered, err) + } + _, signed, valid, verr := pcrypto.VerifyDocument(b, kp.Public) + switch { + case verr != nil: + return fmt.Errorf("%w (the signature could not be checked: %v)", ErrPolicyTampered, verr) + case signed && !valid: + return ErrPolicyTampered + case !signed: + return fmt.Errorf("%w (it carries no signature, and this machine already has a signing key)", ErrPolicyTampered) + } + return nil +} + func LoadLocalPolicyFile(cwd string) (*shared.PolicyCache, error) { if cwd == "" { if wd, err := os.Getwd(); err == nil { @@ -51,11 +96,8 @@ func LoadLocalPolicyFile(cwd string) (*shared.PolicyCache, error) { continue } if p == own { - kp, kerr := pcrypto.LoadOrCreateKeyPair(filepath.Join(shared.SGDir(), "crypto")) - if kerr == nil { - if _, signed, valid, verr := pcrypto.VerifyDocument(b, kp.Public); verr == nil && signed && !valid { - return nil, ErrPolicyTampered - } + if err := verifyHomePolicy(b); err != nil { + return nil, err } } var env *shared.PolicyCache diff --git a/packages/guard/guardcore/config_test.go b/packages/guard/guardcore/config_test.go index c74aef0..a40bbc3 100644 --- a/packages/guard/guardcore/config_test.go +++ b/packages/guard/guardcore/config_test.go @@ -247,21 +247,25 @@ func TestLoadLocalPolicyFileOwnTakesPrecedenceOverProjectLocal(t *testing.T) { } } -func TestLoadLocalPolicyFileSkipsACorruptOwnFileAndFallsBackToProjectLocal(t *testing.T) { +func TestLoadLocalPolicyFileCorruptOwnFileIsRejectedNotSkipped(t *testing.T) { sgHome(t) cwd := t.TempDir() + // A home policy that cannot be parsed is a broken trust anchor, not an + // absent one. Skipping it used to fall through to the project-local policy, + // which carries no Security fields at all - so corrupting the file quietly + // downgraded the guard instead of stopping it. writeOwnPolicy(t, []byte(`{not valid json at all`)) writeProjectPolicy(t, cwd, marshal(t, map[string]interface{}{ "id": "proj-pol", "name": "Project", "mode": "advisory", })) env, err := LoadLocalPolicyFile(cwd) - if err != nil { - t.Fatalf("unexpected error: %v", err) + if !errors.Is(err, ErrPolicyTampered) { + t.Fatalf("got err=%v, want ErrPolicyTampered for a corrupt own policy", err) } - if env == nil || env.Policy == nil || env.Policy.ID != "proj-pol" { - t.Fatalf("expected to fall back to the project-local policy, got %+v", env) + if env != nil { + t.Errorf("expected no cache for a corrupt own policy, got %+v", env) } } @@ -279,4 +283,74 @@ func TestLoadLocalPolicyFileResolvesCwdFromGetwdWhenEmpty(t *testing.T) { if env == nil || env.Policy == nil || env.Policy.ID != "home-pol" { t.Fatalf("expected the own policy to load with an empty cwd, got %+v", env) } -} \ No newline at end of file +} +func TestLoadLocalPolicyFileOwnMalformedSignatureIsRejected(t *testing.T) { + sgHome(t) + cwd := t.TempDir() + + // A signature field that is present but cannot even be decoded: the loader + // used to skip the check on any verification error and trust the file. + doc := map[string]interface{}{ + "policy": map[string]interface{}{"id": "home-pol", "name": "Home", "mode": "advisory"}, + "_sig": "%not-base64%", + } + writeOwnPolicy(t, marshal(t, doc)) + + env, err := LoadLocalPolicyFile(cwd) + if !errors.Is(err, ErrPolicyTampered) { + t.Fatalf("got err=%v, want ErrPolicyTampered for an undecodable signature", err) + } + if env != nil { + t.Errorf("expected no cache for an undecodable signature, got %+v", env) + } +} + +func TestLoadLocalPolicyFileOwnUnsignedAfterTheMachineHasASigningKeyIsRejected(t *testing.T) { + sgHome(t) + cwd := t.TempDir() + + // Once this machine has a signing key, its own policy.json is always written + // signed, so a missing signature means the signature was stripped. + if _, err := pcrypto.LoadOrCreateKeyPair(filepath.Join(shared.SGDir(), "crypto")); err != nil { + t.Fatal(err) + } + doc := map[string]interface{}{ + "policy": map[string]interface{}{"id": "home-pol", "name": "Home", "mode": "advisory"}, + } + writeOwnPolicy(t, marshal(t, doc)) + + env, err := LoadLocalPolicyFile(cwd) + if !errors.Is(err, ErrPolicyTampered) { + t.Fatalf("got err=%v, want ErrPolicyTampered for a stripped signature", err) + } + if env != nil { + t.Errorf("expected no cache for a stripped signature, got %+v", env) + } +} + +func TestLoadLocalPolicyFileDoesNotCreateASigningKey(t *testing.T) { + sgHome(t) + cwd := t.TempDir() + + // Pre-signature installs and the parity fixtures write an unsigned + // policy.json. Loading it must not create a key pair: if it did, the next run + // would see a keyed machine and reject the very same file. + doc := map[string]interface{}{ + "policy": map[string]interface{}{"id": "home-pol", "name": "Home", "mode": "advisory"}, + "security": map[string]interface{}{"protectedPaths": []string{"/tmp/keep"}}, + } + writeOwnPolicy(t, marshal(t, doc)) + + for i := 0; i < 2; i++ { + env, err := LoadLocalPolicyFile(cwd) + if err != nil { + t.Fatalf("load %d: got err=%v, want the unsigned policy to load", i+1, err) + } + if env == nil || env.Policy == nil || env.Policy.ID != "home-pol" { + t.Fatalf("load %d: got %+v, want the home policy", i+1, env) + } + if pcrypto.HasKeyPair(filepath.Join(shared.SGDir(), "crypto")) { + t.Fatalf("load %d: loading a policy created a signing key", i+1) + } + } +}