From 10983b3740fe081f1a82f657569f17a52c85bc28 Mon Sep 17 00:00:00 2001 From: Travis Turk <55367187+turkosaurus@users.noreply.github.com> Date: Sat, 27 Jun 2026 23:08:37 -0600 Subject: [PATCH 1/2] harden request auth and fix handler/source bugs Security (server.go): - Add missing returns so the CIDR allowlist and decrypt-error paths actually block instead of falling through to serving the secret. - Bind ClientPubKey, timestamp, and a single-use nonce into the signed request (crypto.go, client.go) so a captured request can't be replayed with a substituted response key. - Reject stale/future requests outside MaxClockSkew and exact replays via a nonce cache swept by a background goroutine; add Server.Close to stop it. Correctness: - Env.Load: check the lowercase key so mixed-case services keep all secrets. - Onepass.Load: key services lowercase to match server lookup. - WriteRegistry: close the file handle. - Register: normalize name so re-registering updates instead of duplicating. Add server_test.go covering the rejection paths and the happy path. --- client.go | 19 ++++- crypto.go | 20 ++++++ locket.go | 11 +-- locket_test.go | 1 + registry.go | 5 ++ server.go | 132 ++++++++++++++++++++++++++++++++--- server_test.go | 185 +++++++++++++++++++++++++++++++++++++++++++++++++ source.go | 5 +- 8 files changed, 360 insertions(+), 18 deletions(-) create mode 100644 server_test.go diff --git a/client.go b/client.go index 9181795..db19928 100644 --- a/client.go +++ b/client.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "time" ) // Client makes requests to a locket server, and must know the server address. @@ -24,8 +25,10 @@ type Client struct { // kvRequest is the request format for the client to send to the server. type kvRequest struct { Payload string `json:"payload"` // key for which cilent requests a value - PayloadSignature string `json:"signature"` // ed25519 signature of payload - ClientPubKey string `json:"client_pubkey"` // public key used to encrypt payload + PayloadSignature string `json:"signature"` // ed25519 signature over requestMessage() + ClientPubKey string `json:"client_pubkey"` // public key used to encrypt the response + Timestamp int64 `json:"timestamp"` // unix seconds, signed to bound replay + Nonce string `json:"nonce"` // single-use random value, signed to block replay } // NewClient creates a new client, fetches the server's encryption public key, @@ -94,9 +97,19 @@ func (c *Client) FetchSecret(name string) (string, error) { if err != nil { return "", fmt.Errorf("encrypt: %w", err) } + ts := time.Now().Unix() + nonce, err := newNonce() + if err != nil { + return "", fmt.Errorf("generate nonce: %w", err) + } request.Payload = cypher request.ClientPubKey = c.keyRsaPublic - sig, err := signEd25519(c.keyEd25519Private, name) + request.Timestamp = ts + request.Nonce = nonce + sig, err := signEd25519( + c.keyEd25519Private, + requestMessage(name, c.keyRsaPublic, ts, nonce), + ) if err != nil { return "", fmt.Errorf("sign: %w", err) } diff --git a/crypto.go b/crypto.go index 454a3ab..5791524 100644 --- a/crypto.go +++ b/crypto.go @@ -109,6 +109,26 @@ func NewPairEd25519() (string, string, error) { return string(publicKeyPEM), string(privateKeyPEM), nil } +// requestMessage builds the canonical string a client signs and the server +// verifies. Binding the client encryption pubkey, timestamp, and a single-use +// nonce into the signed material prevents an attacker from replaying a captured +// request with a substituted ClientPubKey (which would otherwise leak the +// secret to them), bounds the window in which any replay is accepted, and lets +// the server reject exact replays within that window. +func requestMessage(name, clientPubKey string, timestamp int64, nonce string) string { + return fmt.Sprintf("%s\n%s\n%d\n%s", name, clientPubKey, timestamp, nonce) +} + +// newNonce returns a base64-encoded random nonce used to make each request +// single-use, so the server can detect and reject replays. +func newNonce() (string, error) { + b := make([]byte, 16) + if _, err := rand.Read(b); err != nil { + return "", fmt.Errorf("read random: %w", err) + } + return base64.StdEncoding.EncodeToString(b), nil +} + // signEd25519 signs a message with privateKeyPEM generated by NewPairEd25519(), // and returns a base64 encoded signature. func signEd25519(privateKeyPEM, message string) (string, error) { diff --git a/locket.go b/locket.go index 1a00a1f..ec48a9f 100644 --- a/locket.go +++ b/locket.go @@ -3,18 +3,21 @@ package locket import ( "log/slog" "testing" + "time" logger "github.com/grackleclub/log" ) var Defaults = defaults{ - AllowCIDR: "10.0.0.0/24", - BitsizeRSA: 2048, + AllowCIDR: "10.0.0.0/24", + BitsizeRSA: 2048, + MaxClockSkew: 30 * time.Second, } type defaults struct { - AllowCIDR string // client requests from outside this CIDR are forbidden - BitsizeRSA int // bit size passed to RSA creation for client and server encryption + AllowCIDR string // client requests from outside this CIDR are forbidden + BitsizeRSA int // bit size passed to RSA creation for client and server encryption + MaxClockSkew time.Duration // max client/server clock difference before a request is rejected } // map[serviceName]keyPrivateSigning diff --git a/locket_test.go b/locket_test.go index 980dd8b..5f19a86 100644 --- a/locket_test.go +++ b/locket_test.go @@ -36,6 +36,7 @@ func TestE2E(t *testing.T) { server, err := NewServer(source, registry) require.NoError(t, err) + defer server.Close() handler := httptest.NewServer(http.HandlerFunc(server.Handler)) defer handler.Close() diff --git a/registry.go b/registry.go index 0868d2e..bbc2a53 100644 --- a/registry.go +++ b/registry.go @@ -31,6 +31,7 @@ func WriteRegistry(path string, data []RegEntry) error { if err != nil { return fmt.Errorf("create file: %w", err) } + defer f.Close() for i, item := range data { data[i].Name = strings.TrimSuffix(filepath.Base(item.Name), ".env") @@ -86,6 +87,10 @@ func Register(name string, registryPath string) (string, string, error) { return "", "", fmt.Errorf("generate key pair: %w", err) } + // match the normalization WriteRegistry applies, so re-registering the same + // service updates its entry rather than appending a duplicate. + name = strings.TrimSuffix(filepath.Base(name), ".env") + var registry []RegEntry _, err = os.Stat(registryPath) if err == nil { diff --git a/server.go b/server.go index 7bdbbea..32c3cd8 100644 --- a/server.go +++ b/server.go @@ -6,6 +6,8 @@ import ( "net" "net/http" "strings" + "sync" + "time" "github.com/google/uuid" ) @@ -15,6 +17,71 @@ type Server struct { registry []RegEntry // registered services keyRsaPublic string // encryption public key keyRsaPrivate string // encryption private key + seen *nonceCache // request nonces seen within the replay window +} + +// nonceCache tracks request nonces so the server can reject exact replays +// within the accepted clock-skew window. A background sweeper evicts entries +// once a replay of that request could no longer pass the timestamp freshness +// check, keeping the map bounded without scanning on the request path. +type nonceCache struct { + mu sync.Mutex + seen map[string]time.Time // nonce -> expiry + stop chan struct{} + stopOnce sync.Once +} + +// newNonceCache returns a cache whose sweeper evicts expired nonces every +// interval until close is called. +func newNonceCache(interval time.Duration) *nonceCache { + c := &nonceCache{ + seen: make(map[string]time.Time), + stop: make(chan struct{}), + } + go c.sweep(interval) + return c +} + +// observe records nonce with the given expiry and reports whether it was +// already present (i.e. a replay). Eviction happens out of band in sweep; a +// not-yet-swept expired nonce is harmless since stale requests are already +// rejected by the freshness check before reaching here. +func (c *nonceCache) observe(nonce string, expiry time.Time) bool { + c.mu.Lock() + defer c.mu.Unlock() + if _, ok := c.seen[nonce]; ok { + return true + } + c.seen[nonce] = expiry + return false +} + +// sweep periodically deletes expired nonces until the cache is closed. +func (c *nonceCache) sweep(interval time.Duration) { + if interval <= 0 { + interval = time.Minute + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-c.stop: + return + case now := <-ticker.C: + c.mu.Lock() + for n, exp := range c.seen { + if now.After(exp) { + delete(c.seen, n) + } + } + c.mu.Unlock() + } + } +} + +// close stops the sweeper goroutine. Safe to call more than once. +func (c *nonceCache) close() { + c.stopOnce.Do(func() { close(c.stop) }) } // kvResponse is the server's response to the client's request, @@ -35,6 +102,7 @@ func NewServer(opts source, registry []RegEntry) (*Server, error) { registry: registry, keyRsaPublic: rsaPublic, keyRsaPrivate: rsaPrivate, + seen: newNonceCache(Defaults.MaxClockSkew), } switch opts := opts.(type) { @@ -70,6 +138,12 @@ func NewServer(opts source, registry []RegEntry) (*Server, error) { } } +// Close releases the server's background resources (the nonce-cache sweeper). +// The Server must not be used after Close. +func (s *Server) Close() { + s.seen.close() +} + func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { id := uuid.New().String() log.Info("received request", @@ -104,8 +178,9 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { if err != nil { log.Error("decrypt payload", "request_id", id, "error", err) http.Error(w, "bad request", http.StatusBadRequest) + return } - log.Debug("request payload decrypted", "payload", payload, "request_id", id) + log.Debug("request payload decrypted", "request_id", id) // require from CIDR range DefaultAllowCIDR ip, _, err := net.SplitHostPort(r.RemoteAddr) @@ -129,23 +204,48 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { "allowCIDR", Defaults.AllowCIDR, ) http.Error(w, "forbidden", http.StatusForbidden) - } else { - log.Debug("IP allowed", + return + } + log.Debug("IP allowed", + "request_id", id, + "ip", r.RemoteAddr, + "allowCIDR", Defaults.AllowCIDR, + ) + + // a nonce is required to detect replays + if request.Nonce == "" { + log.Warn("request missing nonce", "request_id", id) + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + // reject stale or future-dated requests to bound replay + skew := time.Since(time.Unix(request.Timestamp, 0)) + if skew < 0 { + skew = -skew + } + if skew > Defaults.MaxClockSkew { + log.Warn("request timestamp outside allowed window", "request_id", id, - "ip", r.RemoteAddr, - "allowCIDR", Defaults.AllowCIDR, + "skew", skew, + "max", Defaults.MaxClockSkew, ) + http.Error(w, "forbidden", http.StatusForbidden) + return } - // verify signature against registry + // verify signature against registry; the signed message binds the + // client pubkey, timestamp, and nonce so a captured request cannot be + // replayed with a substituted ClientPubKey to redirect the secret. var matches bool var verifiedService string + message := requestMessage(payload, request.ClientPubKey, request.Timestamp, request.Nonce) log.Debug("verifying signature", "request_id", id) for _, svc := range s.registry { - match, err := verifyEd25519(svc.KeyPub, payload, request.PayloadSignature) + match, err := verifyEd25519(svc.KeyPub, message, request.PayloadSignature) if err != nil { log.Error("verify signature", "request_id", id, "error", err) - http.Error(w, "bad request", http.StatusBadRequest) + continue } if match { matches = true @@ -164,6 +264,19 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { ) } + // reject replays: a nonce is valid only until a replay could no longer + // pass the freshness check above. Checked after signature verification + // so unauthenticated requests cannot fill the cache. + expiry := time.Unix(request.Timestamp, 0).Add(Defaults.MaxClockSkew) + if s.seen.observe(request.Nonce, expiry) { + log.Warn("replayed request rejected", + "service", verifiedService, + "request_id", id, + ) + http.Error(w, "forbidden", http.StatusForbidden) + return + } + log.Debug("secrets for service", "service", verifiedService, "secrets_qty", len(s.secrets)) secrets, ok := s.secrets[strings.ToLower(verifiedService)] if !ok { @@ -191,6 +304,8 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { response := kvResponse{ Payload: ecryptedSecret, } + // header must be set before the body is written to take effect + w.Header().Set("Content-Type", "application/json") err = json.NewEncoder(w).Encode(response) if err != nil { log.Error("encode response", "request_id", id, "error", err) @@ -203,7 +318,6 @@ func (s *Server) Handler(w http.ResponseWriter, r *http.Request) { "ip", r.RemoteAddr, "request_id", id, ) - w.Header().Set("Content-Type", "application/json") default: log.Warn("method not allowed", "method", r.Method, "request_id", id, "ip", r.RemoteAddr) http.Error(w, "method not allowed", http.StatusMethodNotAllowed) diff --git a/server_test.go b/server_test.go new file mode 100644 index 0000000..51a9762 --- /dev/null +++ b/server_test.go @@ -0,0 +1,185 @@ +package locket + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +const ( + testSecretName = "SERVICE1_FOO" + testSecretValue = "foovalue" +) + +// newTestServer builds a server backed by the example .env and a single +// registered service, returning the running test server, the underlying +// *Server (for its encryption pubkey), and the service's ed25519 signing keys. +// No files are written, so example/testreg.yml is left untouched. +func newTestServer(t *testing.T) (*httptest.Server, *Server, string) { + t.Helper() + pub, priv, err := NewPairEd25519() + require.NoError(t, err) + + registry := []RegEntry{{Name: "SERVICE1", KeyPub: pub}} + source := Dotenv{ + Path: testEnvFile, + ServiceSecrets: testServiceMap, + } + server, err := NewServer(source, registry) + require.NoError(t, err) + t.Cleanup(server.Close) + + ts := httptest.NewServer(http.HandlerFunc(server.Handler)) + t.Cleanup(ts.Close) + return ts, server, priv +} + +// craftRequest builds a request body for secretName, signed by signingPriv, +// with the response to be encrypted to clientRSAPub. ts is exposed so tests can +// forge stale timestamps. +func craftRequest(t *testing.T, serverRSAPub, signingPriv, secretName, clientRSAPub string, ts int64) kvRequest { + t.Helper() + nonce, err := newNonce() + require.NoError(t, err) + payload, err := encryptRSA(serverRSAPub, secretName) + require.NoError(t, err) + sig, err := signEd25519(signingPriv, requestMessage(secretName, clientRSAPub, ts, nonce)) + require.NoError(t, err) + return kvRequest{ + Payload: payload, + PayloadSignature: sig, + ClientPubKey: clientRSAPub, + Timestamp: ts, + Nonce: nonce, + } +} + +func postRequest(t *testing.T, url string, req kvRequest) (*http.Response, []byte) { + t.Helper() + body, err := json.Marshal(req) + require.NoError(t, err) + resp, err := http.Post(url, "application/json", bytes.NewReader(body)) + require.NoError(t, err) + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return resp, b +} + +// assertSecretNotLeaked fails if a kvResponse anywhere in body decrypts to the +// secret with priv. It scans from the first '{' so it also catches the case +// where http.Error wrote "forbidden\n" before the handler wrongly appended the +// encrypted secret (the original missing-return CIDR bug). +func assertSecretNotLeaked(t *testing.T, body []byte, priv string) { + t.Helper() + idx := bytes.IndexByte(body, '{') + if idx < 0 { + return + } + var kv kvResponse + if err := json.Unmarshal(body[idx:], &kv); err != nil || kv.Payload == "" { + return + } + got, err := decryptRSA(priv, kv.Payload) + if err != nil { + return + } + require.NotEqual(t, testSecretValue, got, "secret leaked in non-200 response body") +} + +// TestHandlerHappyPath confirms a correctly signed, in-CIDR, fresh request +// still returns the secret after the security hardening. +func TestHandlerHappyPath(t *testing.T) { + ts, server, signingPriv := newTestServer(t) + clientPub, clientPriv, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + + req := craftRequest(t, server.keyRsaPublic, signingPriv, testSecretName, clientPub, time.Now().Unix()) + resp, body := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusOK, resp.StatusCode) + + var kv kvResponse + require.NoError(t, json.Unmarshal(body, &kv)) + got, err := decryptRSA(clientPriv, kv.Payload) + require.NoError(t, err) + require.Equal(t, testSecretValue, got) +} + +// TestHandlerRejectsPubkeySubstitution is the regression test for the +// unauthenticated-ClientPubKey flaw: a captured, validly-signed request replayed +// with the attacker's own response key must be rejected, and must not leak the +// secret encrypted to the attacker's key. +func TestHandlerRejectsPubkeySubstitution(t *testing.T) { + ts, server, signingPriv := newTestServer(t) + clientPub, _, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + attackerPub, attackerPriv, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + + // legitimate signed request, then swap in the attacker's response key + // while keeping the original signature and payload. + req := craftRequest(t, server.keyRsaPublic, signingPriv, testSecretName, clientPub, time.Now().Unix()) + req.ClientPubKey = attackerPub + + resp, body := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertSecretNotLeaked(t, body, attackerPriv) +} + +// TestHandlerRejectsOutOfCIDR is the regression test for the missing-return CIDR +// bug: a fully valid request from outside the allowed CIDR must be blocked and +// must not leak the secret. +func TestHandlerRejectsOutOfCIDR(t *testing.T) { + ts, server, signingPriv := newTestServer(t) + clientPub, clientPriv, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + + // test requests originate from 127.0.0.1; exclude it from the allowlist. + prev := Defaults.AllowCIDR + Defaults.AllowCIDR = "10.0.0.0/24" + t.Cleanup(func() { Defaults.AllowCIDR = prev }) + + req := craftRequest(t, server.keyRsaPublic, signingPriv, testSecretName, clientPub, time.Now().Unix()) + resp, body := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertSecretNotLeaked(t, body, clientPriv) +} + +// TestHandlerRejectsReplay is the regression test for the seen-nonce cache: an +// identical, validly-signed, in-window request replayed verbatim is served once +// and rejected the second time. +func TestHandlerRejectsReplay(t *testing.T) { + ts, server, signingPriv := newTestServer(t) + clientPub, clientPriv, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + + req := craftRequest(t, server.keyRsaPublic, signingPriv, testSecretName, clientPub, time.Now().Unix()) + + resp1, _ := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusOK, resp1.StatusCode) + + resp2, body2 := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusForbidden, resp2.StatusCode) + assertSecretNotLeaked(t, body2, clientPriv) +} + +// TestHandlerRejectsStaleTimestamp is the regression test for the replay +// window: a request whose signed timestamp is outside MaxClockSkew is rejected +// even though the signature itself is valid. +func TestHandlerRejectsStaleTimestamp(t *testing.T) { + ts, server, signingPriv := newTestServer(t) + clientPub, clientPriv, err := newPairRSA(Defaults.BitsizeRSA) + require.NoError(t, err) + + stale := time.Now().Add(-1 * time.Hour).Unix() + req := craftRequest(t, server.keyRsaPublic, signingPriv, testSecretName, clientPub, stale) + resp, body := postRequest(t, ts.URL, req) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + assertSecretNotLeaked(t, body, clientPriv) +} diff --git a/source.go b/source.go index a62884f..9503da6 100644 --- a/source.go +++ b/source.go @@ -65,7 +65,7 @@ func (e Env) Load() (map[string]Secrets, error) { "value", strings.Repeat("*", len(value)+jitter), ) nameLower := strings.ToLower(serviceName) - if _, ok := parent[serviceName]; !ok { + if _, ok := parent[nameLower]; !ok { // if service not yet in parent, add it parent[nameLower] = make(Secrets) } @@ -226,7 +226,8 @@ func (o Onepass) Load() (map[string]Secrets, error) { for _, secret := range serviceDetail.Fields { serviceSecrects[secret.Title] = secret.Value } - allSecrets[service.Title] = serviceSecrects + // server looks up services by lowercased name; key to match + allSecrets[strings.ToLower(service.Title)] = serviceSecrects log.Debug("loaded secrets for service", "qty", len(serviceSecrects), "service", service.Title) } } From 4ac32b8b49d25b9381ee833bb1760efa96655015 Mon Sep 17 00:00:00 2001 From: Travis Turk <55367187+turkosaurus@users.noreply.github.com> Date: Sat, 27 Jun 2026 23:15:19 -0600 Subject: [PATCH 2/2] test: cover Env.Load mixed-case and Register dedup fixes Lock in the two correctness fixes that previously ran without assertions: - Env.Load retains all secrets for a mixed-case service name. - Register normalizes names so re-registering updates instead of duplicating. --- registry_test.go | 26 ++++++++++++++++++++++++++ source_test.go | 23 +++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/registry_test.go b/registry_test.go index dd43024..f74335a 100644 --- a/registry_test.go +++ b/registry_test.go @@ -3,6 +3,7 @@ package locket import ( "os" "path" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -34,6 +35,31 @@ func TestReadWrite(t *testing.T) { } } +// TestRegisterNoDuplicate is the regression test for Register name +// normalization: re-registering a service (including with a .env suffix that +// WriteRegistry strips) must update the existing entry rather than append a +// duplicate. +func TestRegisterNoDuplicate(t *testing.T) { + reg := filepath.Join(t.TempDir(), "registry.yml") + + _, _, err := Register("svc.env", reg) + require.NoError(t, err) + + pub2, _, err := Register("svc.env", reg) + require.NoError(t, err) + + // the plain name normalizes to the same entry too + _, _, err = Register("svc", reg) + require.NoError(t, err) + + entries, err := ReadRegistryFile(reg) + require.NoError(t, err) + require.Len(t, entries, 1, "re-registering the same service must not duplicate") + require.Equal(t, "svc", entries[0].Name) + // last write wins on the key + require.NotEqual(t, pub2, entries[0].KeyPub) +} + func TestRegister(t *testing.T) { testRegistry := path.Join("example", "test-registry.yml") services := []string{"service A", "service B", "service C"} diff --git a/source_test.go b/source_test.go index d42c8e0..e80eb57 100644 --- a/source_test.go +++ b/source_test.go @@ -68,6 +68,29 @@ func TestLoadEnv(t *testing.T) { } } +// TestLoadEnvMixedCaseService is the regression test for the Env.Load existence +// check: a mixed-case service name must retain every one of its secrets, not +// just the last one loaded (the bug reset the service map on each secret because +// it checked the original-case key against a lowercase-keyed map). +func TestLoadEnvMixedCaseService(t *testing.T) { + secretNames := []string{"REGRESSION_A", "REGRESSION_B", "REGRESSION_C"} + for i, name := range secretNames { + require.NoError(t, os.Setenv(name, fmt.Sprintf("v%d", i))) + t.Cleanup(func() { os.Unsetenv(name) }) + } + + source := Env{ + ServiceSecrets: map[string][]string{"MixedSvc": secretNames}, + } + secrets, err := source.Load() + require.NoError(t, err) + + // server looks services up lowercased + svc, ok := secrets["mixedsvc"] + require.True(t, ok, "mixed-case service should be present (lowercased)") + require.Len(t, svc, len(secretNames), "all secrets for the service must be retained") +} + // testing requires a file (part of .git) to be loaded into env // to then test env loading func putFileToEnv() error {