fix: use crypto/rand instead of math/rand for ECDSA key generation

[raballew]

The OIDC signing key was derived from math/rand seeded with a SHA-256 hash of an environment variable, making it cryptographically insecure and deterministic. Replaced with crypto/rand.Reader via ecdsa.GenerateKey for proper cryptographic randomness.

Also removes the unused filippo.io/keygen dependency.

Closes #350

[netlify]

❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →

Name	Link
🔨 Latest commit	943cefa
🔍 Latest deploy log	https://app.netlify.com/projects/jumpstarter-docs/deploys/69cbe2e0938b31000846c4ed

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaced deterministic ECDSA key generation (seeded from CONTROLLER_KEY environment variable using math/rand) with cryptographically secure random key generation using crypto/rand. Removed the filippo.io/keygen dependency and updated all call sites from NewSignerFromSeed to NewSignerWithRandomKey.

Changes

Cohort / File(s)	Summary
Core OIDC Implementation controller/internal/oidc/op.go, controller/internal/oidc/op_test.go	Deleted NewSignerFromSeed function and added NewSignerWithRandomKey for cryptographically secure random key generation. Removed seed-based deterministic key derivation logic. Added comprehensive tests verifying token signing/validation and randomness of generated keys.
Dependency Management controller/go.mod	Removed direct dependency on filippo.io/keygen and indirect dependency on filippo.io/bigmod (no longer needed with crypto/rand approach).
Application Entrypoint controller/cmd/main.go	Updated OIDC signer initialization to use NewSignerWithRandomKey instead of NewSignerFromSeed, eliminating dependency on CONTROLLER_KEY environment variable.
Test Infrastructure Updates controller/internal/controller/client_controller_test.go, controller/internal/controller/exporter_controller_test.go, controller/internal/controller/lease_controller_test.go, controller/internal/controller/suite_test.go	Updated test setup in four files to use NewSignerWithRandomKey instead of NewSignerFromSeed for consistent OIDC signer initialization across test cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 No more seeds from secret keys, no more math/rand's weak decree!
With crypto/rand we dance so free, true randomness for thee!
Each signer now holds unique might, no token theft in sight! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: replacing math/rand with crypto/rand for ECDSA key generation, which is the primary objective of the PR.
Description check	✅ Passed	The description directly relates to the changeset, explaining the security issue with math/rand and the replacement with crypto/rand, plus removal of unused dependency.
Linked Issues check	✅ Passed	The PR fully addresses issue #350 by replacing math/rand with crypto/rand.Reader in ecdsa.GenerateKey, eliminating deterministic key generation and the CONTROLLER_KEY environment variable dependency.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue: new random-key signer function, removal of seed-based signer, dependency cleanup, and corresponding test updates.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

controller/internal/oidc/op.go (1)

47-49: Pre-existing limitation: hardcoded key ID impedes rotation.

The ID() method still returns a hardcoded "default" value. Issue #350 mentions this as a concern for key rotation. Consider tracking this as a follow-up improvement.

Would you like me to open an issue to track adding key ID generation for proper key rotation support?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controller/internal/oidc/op.go` around lines 47 - 49, The Signer.ID() method
currently returns a hardcoded "default" which blocks key rotation; update the
ID() implementation to return a real key identifier (e.g., a stored field or
derived fingerprint) by adding/using a unique kid on the Signer (e.g.,
Signer.kid or derive from the public key via sha256/thumbprint) and ensure any
code that constructs Signer (constructor or Load/Rotate methods) sets that kid
consistently so rotations update it; locate the Signer struct and ID() method in
op.go and replace the literal with the chosen dynamic field or derivation, and
add tests to verify ID changes when keys rotate.

controller/internal/controller/suite_test.go (1)

62-62: Remove unused CONTROLLER_KEY environment variable setup.

Line 62 sets CONTROLLER_KEY which is no longer used anywhere in the codebase. The environment variable is defined in the Kubernetes deployment spec but never actually read by application code, and the test setup is dead code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@controller/internal/controller/suite_test.go` at line 62, Remove the dead
test setup that sets CONTROLLER_KEY: delete the os.Setenv("CONTROLLER_KEY",
"somekey") call in the test initialization (the unused CONTROLLER_KEY
environment variable) so the test suite no longer configures an environment
variable that the code never reads; ensure no other tests rely on CONTROLLER_KEY
before removing and run tests to verify.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@controller/internal/controller/suite_test.go`:
- Line 62: Remove the dead test setup that sets CONTROLLER_KEY: delete the
os.Setenv("CONTROLLER_KEY", "somekey") call in the test initialization (the
unused CONTROLLER_KEY environment variable) so the test suite no longer
configures an environment variable that the code never reads; ensure no other
tests rely on CONTROLLER_KEY before removing and run tests to verify.

In `@controller/internal/oidc/op.go`:
- Around line 47-49: The Signer.ID() method currently returns a hardcoded
"default" which blocks key rotation; update the ID() implementation to return a
real key identifier (e.g., a stored field or derived fingerprint) by
adding/using a unique kid on the Signer (e.g., Signer.kid or derive from the
public key via sha256/thumbprint) and ensure any code that constructs Signer
(constructor or Load/Rotate methods) sets that kid consistently so rotations
update it; locate the Signer struct and ID() method in op.go and replace the
literal with the chosen dynamic field or derivation, and add tests to verify ID
changes when keys rotate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 98a5697c-a070-408d-b0e2-301f60abd5c6

📥 Commits

Reviewing files that changed from the base of the PR and between 03fc412 and 943cefa.

⛔ Files ignored due to path filters (1)

• controller/go.sum is excluded by !**/*.sum

📒 Files selected for processing (8)

• controller/cmd/main.go
• controller/go.mod
• controller/internal/controller/client_controller_test.go
• controller/internal/controller/exporter_controller_test.go
• controller/internal/controller/lease_controller_test.go
• controller/internal/controller/suite_test.go
• controller/internal/oidc/op.go
• controller/internal/oidc/op_test.go

💤 Files with no reviewable changes (1)

• controller/go.mod

[mangelajo]

If you do this, then there is no way to authenticate clients again.

We use a secret key, created once, for the tokens. That comes mounted as an env var to the service.

If you use a random one how will you authenticate tokens when they come?

[mangelajo]

If you do this, then there is no way to authenticate clients again.

We use a secret key, created once, for the tokens. That comes mounted as an env var to the service.

If you use a random one how will you authenticate tokens when they come?

I am probably missing something, will re-read after PTO :)

@NickCao

[NickCao]

If you do this, then there is no way to authenticate clients again.

Yes that's the case, we need a key stable across controller restarts to be able to validate previously signed jwt tokens.

And the secret is generated at installation time by controller/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/secrets-job.yaml, not chosen by the user, thus should be secure by default.