diff --git a/packages/crypto/crypto_test.go b/packages/crypto/crypto_test.go index db5a6615..6b31c1f7 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 343e3a46..481eefa2 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 453a8718..96bded77 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 c74aef0b..a40bbc30 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) + } + } +}