Merged
Conversation
SachaProbo
approved these changes
Apr 21, 2026
The OAuth2/OIDC server accepted its signing key via a file path (key-file), while every other PEM key in the probod config (SAML private key, ACME account key) is embedded inline. Switch the field to a private-key string so the convention is uniform. The signing key is operator-supplied material that must outlive any process restart, so the bootstrap builder now treats OAUTH2_SERVER_SIGNING_KEY as required and refuses to start without one; silently minting a fresh key per boot would break token validation across rollouts. The OAUTH2_SERVER_* env vars otherwise flow through builder.Build like the existing SAML block so the new OAuth2Server section is populated end-to-end. Rework the e2e harness to render its config via bootstrap at test setup, which removes the static e2e/console/testdata/config.yaml and the previously generated test-only PEM file. A per-run RSA key is minted via bootstrap.GenerateOAuth2SigningKey (kept public for test tooling) and injected through the builder env map. CI now passes ACME_ROOT_CA inline instead of mutating a YAML on disk. Signed-off-by: Bryan Frimin <bryan@getprobo.com>
Contributor
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="pkg/bootstrap/builder.go">
<violation number="1" location="pkg/bootstrap/builder.go:340">
P1: `validateRequired` now makes OAuth2 signing key mandatory, but the new `getOAuth2SigningKey` no longer auto-generates one. This causes `Build()` to fail when no key is supplied instead of falling back to generated credentials.</violation>
</file>
<file name="pkg/bootstrap/builder_test.go">
<violation number="1" location="pkg/bootstrap/builder_test.go:51">
P2: Tests now enforce `OAUTH2_SERVER_SIGNING_KEY` as required, which contradicts the intended auto-generation fallback and can lock in incorrect bootstrap behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
|
|
||
| if b.oauth2SigningKey == "" && b.getEnv("OAUTH2_SERVER_SIGNING_KEY") == "" { |
Contributor
There was a problem hiding this comment.
P1: validateRequired now makes OAuth2 signing key mandatory, but the new getOAuth2SigningKey no longer auto-generates one. This causes Build() to fail when no key is supplied instead of falling back to generated credentials.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pkg/bootstrap/builder.go, line 340:
<comment>`validateRequired` now makes OAuth2 signing key mandatory, but the new `getOAuth2SigningKey` no longer auto-generates one. This causes `Build()` to fail when no key is supplied instead of falling back to generated credentials.</comment>
<file context>
@@ -340,6 +337,10 @@ func (b *Builder) validateRequired() error {
}
}
+ if b.oauth2SigningKey == "" && b.getEnv("OAUTH2_SERVER_SIGNING_KEY") == "" {
+ missing = append(missing, "OAUTH2_SERVER_SIGNING_KEY")
+ }
</file context>
c92cba5 to
c4e81ed
Compare
Committing a fully-materialised cfg/dev.yaml hid the dev configuration surface and blocked the OAuth2 signing-key inlining change: the new config requires a per-dev private key that must not be committed. Replace the checked-in file with a dev-config Make target that shells out to probod-bootstrap with dev-safe defaults and a stable RSA signing key stashed under cfg/.dev-oauth2-signing-key.pem on first run. The recipe sources cfg/dev.env when present so devs can override any setting without editing the Makefile; cfg/dev.env.example ships the full list of overridable knobs. cfg/dev.yaml, cfg/dev.env, and the signing key are all gitignored. Update README, CONTRIBUTING, and contrib/claude/config.md to describe the new workflow. Signed-off-by: Bryan Frimin <bryan@getprobo.com>
Apply the review feedback on the dev-config target: - Treat cfg/dev.env as a prerequisite via $(wildcard ...) so edits to it re-trigger cfg/dev.yaml without the dev having to delete the output first; update the help string accordingly. - Drop the @ silence prefix on the recipe body so failures are debuggable; the values are all known dev placeholders, no leak. - Call out in cfg/dev.env.example that the file is sourced as a POSIX shell snippet (not Docker-compose .env semantics), and list the previously-missing overrides: observability addrs, PG_DEBUG, SMTP auth/TLS, AUTH_COOKIE_DURATION, and the per-worker LLM knobs (probo-agent, evidence-describer). Signed-off-by: Bryan Frimin <bryan@getprobo.com>
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="GNUmakefile">
<violation number="1" location="GNUmakefile:164">
P2: `cfg/dev.yaml` generation uses `cfg/dev.env` but does not depend on it, so env changes won’t trigger regeneration.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Use the conventional .env / .env.example location at the repo root instead of cfg/dev.env / cfg/dev.env.example. .env is what contributors expect, keeps cfg/ a pure generated-config directory, and shares the same file if we ever add another dev target that needs the same overrides. Signed-off-by: Bryan Frimin <bryan@getprobo.com>
The dev-config Make target was missing the ACME_ROOT_CA env var, causing probod to fail with an untrusted certificate error when connecting to the local Pebble ACME server. Signed-off-by: Sacha Al Himdani <sacha@getprobo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AuthConfig.OAuth2Server.SigningKeys[].KeyFile(path on disk) toPrivateKey(inline PEM string) so the OAuth2/OIDC server follows the same config convention as the SAML private key and the ACME account key. probod now decodes the PEM directly instead of callingos.ReadFile.pkg/bootstrapabout the OAuth2 server: a newGenerateOAuth2SigningKeyhelper mints a 2048-bit RSA PEM on demand,builder.BuildpopulatesOAuth2ServerwithOAUTH2_SERVER_*env vars (falling back to auto-generation when no key is supplied), and durations have sensible prod defaults (1h / 30d / 10m / 10m).e2e/console/testdata/config.yamland the test-only PEM it referenced;e2e/internal/testutilnow renders the e2e config throughbootstrap.NewBuilderat setup, writes it to a per-run temp file, and hands the path to probod. CI injects the PebblerootCA.pemviaACME_ROOT_CAinline instead of mutating a YAML on disk.Test plan
make lint(Go + JS) passesgo test ./pkg/bootstrap/... ./pkg/probod/...passes (new bootstrap tests cover auto-gen, env-driven, and preset paths)go build ./...includingcmd/probod-bootstrapmake test-e2eboots probod with a bootstrap-generated config (no PEM file on disk) and the OIDC/OAuth2 flows succeed/.well-known/openid-configuration+ JWKS serve a key whosekidmatches the one used to sign issuedid_tokensSummary by cubic
Inline OAuth2/OIDC signing keys (PEM) in config and route OAuth2 server settings through
pkg/bootstrap. Addmake dev-configto generatecfg/dev.yamlfrom repo-root.envwith a per-dev RSA key and injectACME_ROOT_CAto trust the local Pebble ACME server.New Features
pkg/probodreadsprobod.auth.oauth2-server.signing-keys[].private-key(inline PEM) to sign tokens.pkg/bootstrapaddsGenerateOAuth2SigningKey()(2048-bit RSA), mapsOAUTH2_SERVER_*with defaults (KID "default"; access 1h, refresh 30d, auth/device codes 10m), and requiresOAUTH2_SERVER_SIGNING_KEY.make dev-configwritescfg/dev.yaml, stashes a stable RSA key atcfg/.dev-oauth2-signing-key.pem, sources./.env(ships.env.example), and injectsACME_ROOT_CAfromcompose/pebble/certs/rootCA.pem. E2E config is rendered viabootstrap.NewBuilderwith a per-run key; the statice2e/console/testdata/config.yamlandPROBO_E2E_CONFIGare removed. CI passesACME_ROOT_CAvia env.Migration
signing-keys[].key-filewithsigning-keys[].private-key(inline PEM).OAUTH2_SERVER_SIGNING_KEY(optionalOAUTH2_SERVER_SIGNING_KEY_KID);bootstrapwill not auto-generate a key.Written for commit 565b715. Summary will update on new commits.