Harden request auth and fix handler/source bugs

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)
 	}

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) {

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

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()

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 {

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"}

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)