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{}{} + } +}