refactor(local): use shared idp fixture - #92
Conversation
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change moves the local DemoIDP into ChangesLocal IdP migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Skaffold
participant SecretGenerator
participant Helm
participant EventBusIDP
Skaffold->>SecretGenerator: Generate event-bus configuration and RSA signing key
Skaffold->>Helm: Render and deploy IdP resources
Helm->>EventBusIDP: Mount generated Secrets
sequenceDiagram
participant MQTTClient
participant HTTPRoute
participant EventBusService
participant EventBusIDP
MQTTClient->>HTTPRoute: Request /token
HTTPRoute->>EventBusService: Route request to port 5556
EventBusService->>EventBusIDP: Validate client credentials
EventBusIDP-->>MQTTClient: Return OAuth access token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
/ok to test c35fe41 |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-31 02:33:58 UTC | Commit: c35fe41 |
🛡️ CodeQL Analysis🚨 Found 1 issue(s) Severity Breakdown:
📋 Top Issues💡 Note: Enable GitHub Advanced Security to see full details in the Security tab. 🕐 Last updated: 2026-07-31 02:35:49 UTC | Commit: c35fe41 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
local/infra/README.md (1)
103-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlign the README HTTPRoute example with the deployed route.
Add exact matches for
/healthzand/jwks.json;local/idp/httproute.yamlalready routes all three endpoints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@local/infra/README.md` around lines 103 - 124, Update the HTTPRoute YAML example in the README to include exact path matches for /healthz and /jwks.json alongside the existing /token match, matching the three endpoints configured in local/idp/httproute.yaml.Makefile (1)
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding executed unit tests for the IdP server's auth logic.
This line only compiles
local/idp/server(-run '^$'matches no test names). The package implementshandleTokenandmintJWT, which parse credentials, choose grant types, and mint JWTs. No test file for this package is in the reviewed file set. Add unit tests forhandleTokenandmintJWTand change this to run them, so regressions in credential handling or claim/audience selection are caught before deployment.Do you want me to draft
main_test.gocovering the password and client_credentials grant paths?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 44, Replace the compile-only command with execution of unit tests for the IdP server, adding coverage for handleToken and mintJWT, including password and client_credentials grant handling, credential parsing, and JWT claim/audience selection. Ensure the Makefile target runs these tests rather than filtering all test names out.local/idp/server/main.go (1)
200-221: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse constant-time comparison for client secrets and passwords.
c.Secret != r.Form.Get("client_secret")andu.Password != r.Form.Get("password")compare secrets with the!=operator. This operator does not run in constant time, so it leaks timing information about how much of the guessed secret matches. This server is a local test fixture, so the immediate risk is low. Usecrypto/subtle.ConstantTimeCompareto avoid the pattern anyway, in case this code becomes a template for other credential checks.🔒️ Proposed fix using constant-time comparison
+ "crypto/subtle" + c, ok := s.clients[r.Form.Get("client_id")] - if !ok || c.Secret != r.Form.Get("client_secret") { + if !ok || subtle.ConstantTimeCompare([]byte(c.Secret), []byte(r.Form.Get("client_secret"))) != 1 { writeOAuthError(w, http.StatusUnauthorized, "invalid_client", "client authentication failed") return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@local/idp/server/main.go` around lines 200 - 221, Update the client authentication check in the request handler to compare c.Secret and the submitted client_secret with crypto/subtle.ConstantTimeCompare, and apply the same constant-time comparison to u.Password and the submitted password in the password grant flow. Preserve the existing invalid_client and invalid_grant responses and ensure comparisons handle differing secret lengths correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@local/mqtt-client/pkg/auth/oauth2.go`:
- Around line 24-30: Normalize idpURL in GetOIDCTokenContext by removing
trailing slashes before constructing TokenURL, ensuring the result contains
exactly one separator before “token”. Add a test case covering an IDP URL that
ends with a slash and verify the request targets /token.
In `@local/mqtt-client/tests/functional/topic_auth_test.go`:
- Around line 47-65: Update the token setup in the test around the three
GetOIDCTokenContext calls to create one bounded context with a local timeout,
following the pattern used in nats_leaf_test.go. Pass that context to all three
token requests and ensure its cancellation is released appropriately.
---
Nitpick comments:
In `@local/idp/server/main.go`:
- Around line 200-221: Update the client authentication check in the request
handler to compare c.Secret and the submitted client_secret with
crypto/subtle.ConstantTimeCompare, and apply the same constant-time comparison
to u.Password and the submitted password in the password grant flow. Preserve
the existing invalid_client and invalid_grant responses and ensure comparisons
handle differing secret lengths correctly.
In `@local/infra/README.md`:
- Around line 103-124: Update the HTTPRoute YAML example in the README to
include exact path matches for /healthz and /jwks.json alongside the existing
/token match, matching the three endpoints configured in
local/idp/httproute.yaml.
In `@Makefile`:
- Line 44: Replace the compile-only command with execution of unit tests for the
IdP server, adding coverage for handleToken and mintJWT, including password and
client_credentials grant handling, credential parsing, and JWT claim/audience
selection. Ensure the Makefile target runs these tests rather than filtering all
test names out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 406f78ae-550f-4792-a135-2dcc5bd96073
⛔ Files ignored due to path filters (1)
local/idp/server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
.gitignoreMakefiledocs/getting-started.mdlocal/agent-gateway/charts/demoidp/.helmignorelocal/agent-gateway/skaffold.fixtures.yamllocal/agent-gateway/skaffold.images.yamllocal/agent-gateway/tests/functional/auth_dispatch_test.golocal/agent-gateway/values/values.local-common.yamllocal/event-bus/k8s/local-dev-values.yamllocal/event-bus/skaffold.yamllocal/idp/chart/Chart.yamllocal/idp/chart/templates/demoidp.yamllocal/idp/chart/values.yamllocal/idp/httproute.yamllocal/idp/namespace.yamllocal/idp/prepare-secrets.shlocal/idp/secret-generator/credentials.yaml.tmpllocal/idp/secret-generator/kustomization.yamllocal/idp/server/Dockerfilelocal/idp/server/go.modlocal/idp/server/main.golocal/idp/skaffold.yamllocal/infra/README.mdlocal/infra/keycloak/keycloak.yamllocal/infra/keycloak/operator/kustomization.yamllocal/infra/keycloak/realm-import.yamllocal/infra/keycloak/skaffold.yamllocal/infra/skaffold.yamllocal/mqtt-client/pkg/auth/oauth2.golocal/mqtt-client/pkg/auth/oauth2_test.golocal/mqtt-client/pkg/config/config.golocal/mqtt-client/tests/functional/nats_leaf_test.golocal/mqtt-client/tests/functional/topic_auth_test.golocal/scripts/cleanup.shlocal/scripts/prepare-dependencies.shlocal/validate.sh
💤 Files with no reviewable changes (8)
- local/infra/keycloak/keycloak.yaml
- local/agent-gateway/charts/demoidp/.helmignore
- local/scripts/prepare-dependencies.sh
- local/infra/skaffold.yaml
- local/infra/keycloak/operator/kustomization.yaml
- local/infra/keycloak/skaffold.yaml
- local/infra/keycloak/realm-import.yaml
- local/agent-gateway/skaffold.images.yaml
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
/ok to test 754e65c |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
local/idp/server/main_test.go (1)
116-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
/jwks.jsonand thekidheader.The test verifies the access token with
signingKey.PublicKeydirectly. It never exercises the JWKS endpoint, so a broken or missing/jwks.jsonresponse stays undetected. Local callers depend on/jwks.jsonfor verification.Fetch the JWKS through
s.routes(), then select the key by thekidheader of the parsed token and use it fortoken.Claims. Consider also a case for a grant-type mismatch, whichhandleTokenrejects withunauthorized_clientand status 400.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@local/idp/server/main_test.go` around lines 116 - 124, Extend the access-token test around the parsed token to fetch JWKS through s.routes(), read the token’s kid header, select the matching public key, and pass that key to token.Claims instead of signingKey.PublicKey. Add coverage for a grant-type mismatch rejected by handleToken, asserting unauthorized_client with HTTP status 400.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@local/idp/server/main_test.go`:
- Around line 116-124: Extend the access-token test around the parsed token to
fetch JWKS through s.routes(), read the token’s kid header, select the matching
public key, and pass that key to token.Claims instead of signingKey.PublicKey.
Add coverage for a grant-type mismatch rejected by handleToken, asserting
unauthorized_client with HTTP status 400.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85170127-08a8-4243-a55e-a7ca42113896
📒 Files selected for processing (7)
Makefilelocal/idp/server/main.golocal/idp/server/main_test.golocal/infra/README.mdlocal/mqtt-client/pkg/auth/oauth2.golocal/mqtt-client/pkg/auth/oauth2_test.golocal/mqtt-client/tests/functional/topic_auth_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- local/idp/server/main.go
- Makefile
- local/infra/README.md
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
/ok to test 077d9ab |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@local/idp/chart/templates/secrets.yaml`:
- Around line 18-31: Update the generated secret reads in the template to
validate their inputs with Helm’s required check before base64 encoding: apply
this to both the config.json expression and the tls.key expression, using clear
missing-file messages so rendering fails when either generated file is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 79066a12-9f71-447d-a2ad-732ad7c4234d
📒 Files selected for processing (4)
.gitignorelocal/idp/chart/templates/secrets.yamllocal/idp/prepare-secrets.shlocal/idp/skaffold.yaml
💤 Files with no reviewable changes (1)
- local/idp/skaffold.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- local/idp/prepare-secrets.sh
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
/ok to test 67d5d57 |
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
/ok to test 467f461 |
2 participants
Summary
local/idpSkaffold module/tokenand/jwks.jsonWhy
The merged local stack deployed two identity-provider fixtures. The lightweight
DemoIdP already supports the controlled issuer, audience, expiry, and
signing-key cases required by Agent Gateway tests, so it can serve both local
consumers without carrying Keycloak infrastructure.
Impact
This is an intentional clean break. Local callers must use
IDP_URL,/token,and
/jwks.json; no Keycloak URL aliases remain.Validation
make checkgo -C local/idp/server test ./...go -C local/mqtt-client test ./pkg/... ./internal/... ./cmd/...helm lint local/idp/chart --set image.repository=test --set image.tag=testskaffold diagnose --yaml-only -f skaffold.yamlFull Kind e2e was not run locally because the shared local environment was in
use by other Codex tasks; CI should exercise that path.
Summary by CodeRabbit
New Features
Security
Documentation