From 0e8e6926ebb6b0657a2ef51d09368a9a6f992380 Mon Sep 17 00:00:00 2001 From: Shankar Date: Thu, 16 Apr 2026 19:43:19 +0000 Subject: [PATCH] security: use crypto/rand for agent API keys (fixes C-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces math/rand-based agent API key generation in internal/service/agent.go with crypto/rand.Read over a 32-byte buffer encoded with base64.RawURLEncoding, yielding a 43-character URL-safe unpadded ASCII string (256 bits of entropy). generateAPIKey now returns (string, error); Register and RegisterAgent propagate entropy-source failures. hashAPIKey is unchanged — the SHA-256 hashed-at-rest invariant is preserved. Fixes C-1 (CWE-338: Use of Cryptographically Weak Pseudo-Random Number Generator) from certctl-audit-report.md. Changes: - internal/service/agent.go: new imports (crypto/rand, encoding/base64); generateAPIKey rewritten to return (string, error); Register and RegisterAgent updated to propagate the error. - internal/service/agent_test.go: TestGenerateAPIKey_Properties regression test (non-empty, length 43, valid base64url, 32 decoded bytes, no collisions over 64 calls). No entropy-failure test — Go 1.24+ (issue #66821) makes crypto/rand errors fatal, so that branch is defensively unreachable. Verification: - go build ./cmd/server/... ./cmd/agent/... ./cmd/mcp-server/... ./cmd/cli/... → pass - go vet ./... → pass - go test -race (CI scope, 43 packages) → pass - golangci-lint v2.11.4 run ./... → 0 issues - govulncheck ./... → 0 vulnerabilities in certctl code - Coverage: service 68.9% / handler 83.6% / domain 82.0% / middleware 63.8% (all above CI gates 55/60/40/30) - grep math/rand in internal/ and cmd/ → zero production hits - No caller assumes the old 32-char length or legacy charset --- internal/service/agent.go | 30 ++++++++++++++++-------- internal/service/agent_test.go | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/internal/service/agent.go b/internal/service/agent.go index 43f4f86..43b9962 100644 --- a/internal/service/agent.go +++ b/internal/service/agent.go @@ -2,11 +2,12 @@ package service import ( "context" + "crypto/rand" "crypto/sha256" + "encoding/base64" "encoding/hex" "fmt" "log/slog" - "math/rand" "time" "github.com/shankar0123/certctl/internal/domain" @@ -57,8 +58,11 @@ func (s *AgentService) Register(ctx context.Context, name string, hostname strin return nil, "", fmt.Errorf("agent name and hostname are required") } - // Generate API key - apiKey := generateAPIKey() + // Generate API key. crypto/rand failure is non-recoverable — propagate immediately. + apiKey, err := generateAPIKey() + if err != nil { + return nil, "", fmt.Errorf("failed to generate agent api key: %w", err) + } apiKeyHash := hashAPIKey(apiKey) now := time.Now() @@ -380,7 +384,10 @@ func (s *AgentService) GetAgent(ctx context.Context, id string) (*domain.Agent, // RegisterAgent creates and registers a new agent (handler interface method). func (s *AgentService) RegisterAgent(ctx context.Context, agent domain.Agent) (*domain.Agent, error) { agent.ID = generateID("agent") - apiKey := generateAPIKey() + apiKey, err := generateAPIKey() + if err != nil { + return nil, fmt.Errorf("failed to generate agent api key: %w", err) + } agent.APIKeyHash = hashAPIKey(apiKey) agent.Status = domain.AgentStatusOnline now := time.Now() @@ -487,14 +494,17 @@ func (s *AgentService) CertificatePickup(ctx context.Context, agentID, certID st return string(certPEM), nil } -// generateAPIKey creates a random API key for an agent. -func generateAPIKey() string { - const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789" +// generateAPIKey creates a cryptographically secure random API key for an agent. +// It fills a 32-byte buffer from crypto/rand (256 bits of entropy) and encodes it with +// base64.RawURLEncoding, yielding a 43-character URL-safe, unpadded ASCII string. +// The plaintext key is shown to the caller exactly once; only its SHA-256 hash is stored. +// Fixes C-1 (CWE-338: previously used math/rand, which is not cryptographically secure). +func generateAPIKey() (string, error) { b := make([]byte, 32) - for i := range b { - b[i] = charset[rand.Intn(len(charset))] + if _, err := rand.Read(b); err != nil { + return "", fmt.Errorf("generate agent api key: %w", err) } - return string(b) + return base64.RawURLEncoding.EncodeToString(b), nil } // hashAPIKey hashes an API key using SHA256. diff --git a/internal/service/agent_test.go b/internal/service/agent_test.go index c9d1809..4d1a6b9 100644 --- a/internal/service/agent_test.go +++ b/internal/service/agent_test.go @@ -2,6 +2,7 @@ package service import ( "context" + "encoding/base64" "log/slog" "testing" "time" @@ -594,3 +595,44 @@ func TestListAgents(t *testing.T) { t.Errorf("expected total 2, got %d", total) } } + +// TestGenerateAPIKey_Properties is the core regression test for C-1 (CWE-338). +// It verifies that generateAPIKey produces cryptographically random, +// unpadded base64url-encoded, 32-byte (256-bit) keys that never collide +// across consecutive calls. Exact length and alphabet are verified against +// base64.RawURLEncoding so any silent change to entropy or encoding fails +// fast. +// +// Note on the error branch: since Go 1.24 (issue #66821) crypto/rand.Read +// treats entropy-source failures as fatal — the process is terminated +// rather than returning an error. The defensive `if err != nil` branch +// in generateAPIKey is therefore unreachable from tests on modern Go. +// It is kept to preserve the documented (string, error) contract and +// to remain correct on older Go toolchains or future changes. +func TestGenerateAPIKey_Properties(t *testing.T) { + seen := make(map[string]struct{}, 64) + for i := 0; i < 64; i++ { + k, err := generateAPIKey() + if err != nil { + t.Fatalf("generateAPIKey failed: %v", err) + } + if k == "" { + t.Fatal("expected non-empty API key") + } + // base64.RawURLEncoding of 32 bytes yields exactly 43 chars. + if got, want := len(k), 43; got != want { + t.Fatalf("expected key length %d, got %d (%q)", want, got, k) + } + decoded, err := base64.RawURLEncoding.DecodeString(k) + if err != nil { + t.Fatalf("key %q not valid base64url: %v", k, err) + } + if len(decoded) != 32 { + t.Fatalf("expected 32 decoded bytes (256 bits entropy), got %d", len(decoded)) + } + if _, dup := seen[k]; dup { + t.Fatalf("collision detected after %d calls; weak PRNG?", i+1) + } + seen[k] = struct{}{} + } +}