ECDSA signing key derived from math/rand instead of crypto/rand

[raballew]

Description

The OIDC signing key in controller/internal/oidc/op.go:35-38 is derived using math/rand seeded by 8 bytes of SHA-256(CONTROLLER_KEY). This is not a cryptographically secure PRNG.

• Same CONTROLLER_KEY always produces the same ECDSA private key
• math/rand.Seed() reduces to ~2^63 distinct PRNG states
• Tokens are valid for 365 days with hardcoded kid: "default", preventing graceful rotation
• Any leak of CONTROLLER_KEY (env vars, crash dump, secrets-job compromise) yields instant key recovery

Suggested Fix

Replace rand.NewSource(int64(...)) / rand.New(source) with crypto/rand.Reader passed directly to keygen.ECDSALegacy. Remove SHA-256 truncation and seed derivation.

Impact

Token forgery if CONTROLLER_KEY is leaked. Deterministic keys across environments sharing the same CONTROLLER_KEY.

[mangelajo]

This is incorrect, that key is used for validating existing tokens, and cannot be rotated.

An alternative could be cert-based jwt tokens for the internal authentication, with a migration path, but again, if the cert key is leaked, you have the same problem as leaking the controller or router keys.

[raballew]

I am curious math/rand isnt cryptographically secure anyhow. I agree on the CONTROLLER_KEY part.

[ambient-code]

Investigation: ECDSA signing key derivation

The design is intentionally deterministic, and that's fine

After reading controller/internal/oidc/op.go, I can confirm the signing key is derived from CONTROLLER_KEY so that controller restarts produce the same ECDSA key, allowing previously-issued JWT tokens (which have a 365-day expiry) to remain valid without requiring persistent key storage. This is a reasonable design choice — if CONTROLLER_KEY is compromised, the attacker can derive the signing key no matter what derivation method is used.

However, there is a real entropy reduction

The current code does:

hash := sha256.Sum256(seed)                                         // 256 bits
source := rand.NewSource(int64(binary.BigEndian.Uint64(hash[:8])))  // truncated to 64 bits
reader := rand.New(source)                                          // math/rand PRNG with 64-bit state
key, err := keygen.ECDSALegacy(elliptic.P256(), reader)

This takes a 256-bit SHA-256 hash but only uses the first 64 bits as a seed for math/rand. The resulting ECDSA P-256 key (which normally provides ~128 bits of security) can only be one of ~2^64 possible keys. An attacker who does not know CONTROLLER_KEY but wants to brute-force the signing key only needs to search ~2^64 candidates instead of ~2^128. While 2^64 is not trivially breakable today, it is well below the security margin you'd want for a signing key.

A fix that preserves token continuity is not straightforward

As @mangelajo noted, changing the derivation would produce a different key, invalidating all existing tokens. If you wanted to fix this without breaking existing deployments, you could:

1. Use the full 256-bit SHA-256 output as a seed for a CSPRNG (e.g., via golang.org/x/crypto/hkdf → crypto/ecdsa.GenerateKey), but this would change the derived key.
2. Support both the old and new key simultaneously during a migration period — validate tokens against both keys, but only sign new tokens with the new key. After old tokens expire (up to 365 days), drop the old key.

Practical risk assessment

The real-world risk depends on the threat model. If CONTROLLER_KEY is a high-entropy secret (e.g., 32+ random bytes), an attacker without access to it would still need ~2^64 work to find the signing key — expensive but not impossible for well-funded attackers. If CONTROLLER_KEY is a low-entropy value, the situation is worse since SHA-256 + truncation doesn't help.

The most practical attack vector remains compromising CONTROLLER_KEY itself, which makes this a defense-in-depth concern rather than a critical vulnerability. The fix is worth doing eventually (especially with a dual-key migration path), but it's not urgent.

---

This investigation was performed by an automated agent (Ambient/Claude).