Migrate from shared secret to client credential for publisher api#620
Merged
Migrate from shared secret to client credential for publisher api#620
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces static publisher API-key auth with OAuth2 client-credentials: evaluation job obtains Bearer tokens, agent-manager enforces JWT validation via JWKS (dev-only unsigned fallback removed), Thunder bootstraps a publisher OAuth client, and Helm/CI/manifests/configs were updated to remove static API-key usage and wire IdP credentials and secrets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EvalJob as Evaluation Job
participant IdP as Identity Provider (Thunder)
participant Agent as Agent Manager Service
participant JWKS as JWKS/KeyManager
EvalJob->>IdP: POST /token (client_id, client_secret, grant_type=client_credentials)
IdP-->>EvalJob: 200 OK (access_token, expires_in)
EvalJob->>Agent: POST /api/v1/publisher/scores (Authorization: Bearer <token>)
Agent->>JWKS: Fetch JWKS and validate JWT signature / audience / expiry
JWKS-->>Agent: JWKS response
Agent-->>EvalJob: 200 Accepted (scores published)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/api/app.go (1)
68-79:⚠️ Potential issue | 🔴 CriticalAdd publisher-specific authorization enforcement.
The
PublishScoresroute currently accepts any valid JWT token without restricting access to publisher-only clients. The middleware validates the token signature and expiry but does not enforce a publisher-specific scope or client identity. The controller does not perform additional claim checks, meaning any bearer token with a valid signature can publish scores.Implement one of the following:
- Add a dedicated authorization middleware on the publisher sub-mux that validates a
scores:publishscope or publisher client identifier- Add an explicit scope/client check inside
PublishScoresbefore processing the request🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/api/app.go` around lines 68 - 79, The publisher routes accept any valid JWT because there is no publisher-specific authorization; add enforcement by either (A) creating and attaching a dedicated middleware on the publisher sub-router that inspects token claims set by params.AuthMiddleware and rejects requests missing the scores:publish scope or a specific client_id, and register that middleware on publisherMux (before RegisterMonitorPublisherRoutes or when wrapping publisherHandler), or (B) add an explicit claim check at the start of the PublishScores handler in the MonitorScoresPublisherController to read the JWT claims from the request context, verify the presence of the scores:publish scope or allowed publisher client identifier, and return 403 when absent; reference RegisterMonitorPublisherRoutes, publisherMux, params.AuthMiddleware, and PublishScores when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@deployments/helm-charts/wso2-amp-evaluation-extension/templates/publisher-secret.yaml`:
- Around line 4-5: The Secret "amp-publisher-credentials" is being created in
.Release.Namespace but the Workflow is rendered into
.Values.ampEvaluation.workflowNamespace | default "default", causing a namespace
mismatch; update the publisher-secret.yaml template (metadata.name
amp-publisher-credentials) to set metadata.namespace to the workflow namespace
expression (use .Values.ampEvaluation.workflowNamespace | default "default")
instead of .Release.Namespace so the Secret is created in the same namespace the
Workflow/job runs in and IDP_CLIENT_SECRET can be resolved at runtime.
In `@deployments/helm-charts/wso2-amp-thunder-extension/values.yaml`:
- Around line 159-163: The values.yaml currently hard-codes
ampPublisherClient.clientSecret which creates a shared default; change
ampPublisherClient to accept an externally supplied or bootstrap-generated
secret instead: remove the committed literal for clientSecret and instead add a
reference field (e.g., secretName or leave clientSecret blank/placeholder) so
the chart consumers must supply a secret or the chart templates can generate one
at install time; ensure the deployment/secret templates consume
ampPublisherClient.secretName (or clientSecret value when provided) and remove
the requirement that it be matched in ampEvaluation.publisher.clientSecret by
documenting the needed value (or using a lookup to the created k8s Secret) so no
well-known default is committed.
---
Outside diff comments:
In `@agent-manager-service/api/app.go`:
- Around line 68-79: The publisher routes accept any valid JWT because there is
no publisher-specific authorization; add enforcement by either (A) creating and
attaching a dedicated middleware on the publisher sub-router that inspects token
claims set by params.AuthMiddleware and rejects requests missing the
scores:publish scope or a specific client_id, and register that middleware on
publisherMux (before RegisterMonitorPublisherRoutes or when wrapping
publisherHandler), or (B) add an explicit claim check at the start of the
PublishScores handler in the MonitorScoresPublisherController to read the JWT
claims from the request context, verify the presence of the scores:publish scope
or allowed publisher client identifier, and return 403 when absent; reference
RegisterMonitorPublisherRoutes, publisherMux, params.AuthMiddleware, and
PublishScores when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 619d389e-3991-4312-8931-0cc73653947f
📒 Files selected for processing (20)
.github/workflows/agent-manager-service-pr-checks.yaml.gitignoreagent-manager-service/.env.exampleagent-manager-service/api/app.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/monitor_scores_publisher_controller.goagent-manager-service/wiring/wire_gen.godeployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/deployment.yamldeployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/secret.yamldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-evaluation-extension/templates/publisher-secret.yamldeployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/cluster-workflow-monitor-evaluation.yamldeployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/workflow-monitor-evaluation.yamldeployments/helm-charts/wso2-amp-evaluation-extension/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamldeployments/scripts/setup-openchoreo.shevaluation-job/main.pyevaluation-job/test_main.py
💤 Files with no reviewable changes (7)
- .github/workflows/agent-manager-service-pr-checks.yaml
- agent-manager-service/.env.example
- agent-manager-service/config/config.go
- deployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/deployment.yaml
- deployments/helm-charts/wso2-agent-manager/values.yaml
- agent-manager-service/config/config_loader.go
- agent-manager-service/controllers/monitor_scores_publisher_controller.go
nadheesh
force-pushed
the
migrate-publisher-key-v2
branch
from
cc40524 to
f016c79
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/middleware/jwtassertion/auth.go (1)
199-225:⚠️ Potential issue | 🔴 CriticalFail closed when
KEY_MANAGER_JWKS_URLis missing.This branch still authenticates by base64-decoding the payload and trusting its claims. Because
KEY_MANAGER_JWKS_URLdefaults to empty inagent-manager-service/config/config_loader.goand is not set indeployments/docker-compose.yml, a forged three-part JWT with an allowediss/audcan pass this middleware without ever proving Thunder issued it. That is an auth bypass against the new publisher flow.🔒 Proposed fix
- } else { - // No JWKS URL configured - extract claims without signature validation - // This is only allowed for tokens from configured trusted issuers - extractedClaims, err := extractClaimsFromJWT(tokenString) - if err != nil { - return nil, fmt.Errorf("failed to extract claims: %w", err) - } - claims = extractedClaims - - // Ensures we only skip signature validation for configured trusted issuers - if err := validateIssuer(claims.Issuer, cfg.KeyManagerConfigurations.Issuer); err != nil { - return nil, fmt.Errorf("JWKS signature validation required for issuer '%s'", - claims.Issuer) - } - - // Validate expiration using RegisteredClaims.ExpiresAt - if claims.ExpiresAt != nil { - if !claims.ExpiresAt.After(time.Now()) { - return nil, fmt.Errorf("token has expired") - } - } - - // Validate audience - if err := validateAudience(claims.Audience, cfg.KeyManagerConfigurations.Audience); err != nil { - return nil, err - } - - return claims, nil + } else { + return nil, fmt.Errorf("KEY_MANAGER_JWKS_URL must be configured for JWT validation") }If you still need an insecure local escape hatch, gate it behind an explicit dev-only flag and keep it off by default.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/middleware/jwtassertion/auth.go` around lines 199 - 225, The code currently allows unsigned JWTs when cfg.KeyManagerConfigurations.JwksURL is empty by calling extractClaimsFromJWT and then validating issuer/audience only; instead, fail closed: in the auth.go branch that handles missing JWKS (where extractClaimsFromJWT, validateIssuer, validateAudience are used and cfg.KeyManagerConfigurations.JwksURL is checked), return an error if KeyManagerConfigurations.JwksURL is empty unless an explicit dev-only escape hatch is enabled (e.g., a new config flag like KeyManagerConfigurations.AllowInsecureLocalTokens), and only permit the insecure extraction when that flag is true and the environment is dev; otherwise require a JWKS URL and reject the token. Ensure error messages mention the missing JWKS (KEY_MANAGER_JWKS_URL) so callers know why authentication failed.
♻️ Duplicate comments (1)
deployments/helm-charts/wso2-amp-thunder-extension/values.yaml (1)
160-163:⚠️ Potential issue | 🟠 MajorDon't ship a default publisher client secret.
ampPublisherClient.clientSecretis still a committed literal, and the inline note requires OpenBao to mirror the same value. That recreates the well-known/shared-secret and manual cross-chart sync problem this migration is supposed to eliminate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/helm-charts/wso2-amp-thunder-extension/values.yaml` around lines 160 - 163, The values.yaml currently ships a committed literal for ampPublisherClient.clientSecret which recreates a shared-secret drift issue; remove the hardcoded "amp-publisher-client-secret" value (or set ampPublisherClient.clientSecret to null/empty) and update the inline comment to instruct operators to supply the secret via external secret management/OpenBao (do not hardcode), ensuring the chart consumes the secret name or value injected at deploy time rather than embedding it in values.yaml for ampPublisherClient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/helm-charts/wso2-agent-manager/values.yaml`:
- Around line 123-126: The publisher endpoint currently only uses
JWTAuthMiddleware and lacks scope checks; update the publisher route or the
PublishScores handler to call the existing HasAllScopes(ctx,
[]string{"scores:publish"}) (or equivalent scope-check helper) after JWT
validation and return a 403 on failure. Locate the route registration that wires
JWTAuthMiddleware to the PublishScores controller and add the scope-check
invocation there (or inside PublishScores) ensuring the middleware chain
enforces the "scores:publish" scope before processing the request.
---
Outside diff comments:
In `@agent-manager-service/middleware/jwtassertion/auth.go`:
- Around line 199-225: The code currently allows unsigned JWTs when
cfg.KeyManagerConfigurations.JwksURL is empty by calling extractClaimsFromJWT
and then validating issuer/audience only; instead, fail closed: in the auth.go
branch that handles missing JWKS (where extractClaimsFromJWT, validateIssuer,
validateAudience are used and cfg.KeyManagerConfigurations.JwksURL is checked),
return an error if KeyManagerConfigurations.JwksURL is empty unless an explicit
dev-only escape hatch is enabled (e.g., a new config flag like
KeyManagerConfigurations.AllowInsecureLocalTokens), and only permit the insecure
extraction when that flag is true and the environment is dev; otherwise require
a JWKS URL and reject the token. Ensure error messages mention the missing JWKS
(KEY_MANAGER_JWKS_URL) so callers know why authentication failed.
---
Duplicate comments:
In `@deployments/helm-charts/wso2-amp-thunder-extension/values.yaml`:
- Around line 160-163: The values.yaml currently ships a committed literal for
ampPublisherClient.clientSecret which recreates a shared-secret drift issue;
remove the hardcoded "amp-publisher-client-secret" value (or set
ampPublisherClient.clientSecret to null/empty) and update the inline comment to
instruct operators to supply the secret via external secret management/OpenBao
(do not hardcode), ensuring the chart consumes the secret name or value injected
at deploy time rather than embedding it in values.yaml for ampPublisherClient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cd9f011-65ad-412b-9a7d-d04c759fa254
📒 Files selected for processing (23)
.github/workflows/agent-manager-service-pr-checks.yamlagent-manager-service/.env.exampleagent-manager-service/api/app.goagent-manager-service/config/config.goagent-manager-service/config/config_loader.goagent-manager-service/controllers/monitor_scores_publisher_controller.goagent-manager-service/middleware/jwtassertion/auth.goagent-manager-service/wiring/wire_gen.godeployments/docker-compose.ymldeployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/configmap.yamldeployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/deployment.yamldeployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/secret.yamldeployments/helm-charts/wso2-agent-manager/values.yamldeployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/cluster-workflow-monitor-evaluation.yamldeployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/workflow-monitor-evaluation.yamldeployments/helm-charts/wso2-amp-evaluation-extension/values.yamldeployments/helm-charts/wso2-amp-thunder-extension/templates/amp-thunder-bootstrap.yamldeployments/helm-charts/wso2-amp-thunder-extension/values.yamldeployments/quick-start/uninstall.shdeployments/scripts/setup-openchoreo.shdeployments/single-cluster/values-openbao.yamlevaluation-job/main.pyevaluation-job/test_main.py
💤 Files with no reviewable changes (5)
- .github/workflows/agent-manager-service-pr-checks.yaml
- agent-manager-service/.env.example
- deployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/configmap.yaml
- deployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/deployment.yaml
- agent-manager-service/controllers/monitor_scores_publisher_controller.go
✅ Files skipped from review due to trivial changes (2)
- deployments/single-cluster/values-openbao.yaml
- agent-manager-service/wiring/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (7)
- agent-manager-service/api/app.go
- deployments/helm-charts/wso2-amp-evaluation-extension/values.yaml
- deployments/helm-charts/wso2-agent-manager/templates/agent-manager-service/secret.yaml
- evaluation-job/main.py
- evaluation-job/test_main.py
- agent-manager-service/config/config.go
- deployments/helm-charts/wso2-amp-evaluation-extension/templates/workflow-templates/cluster-workflow-monitor-evaluation.yaml
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-manager-service/middleware/jwtassertion/auth.go (1)
155-200:⚠️ Potential issue | 🟠 MajorRefresh JWKS once on
kidmiss.Now that Line 199 makes JWKS the only validation path, a normal IdP signing-key rotation can fail closed for up to
jwksCacheTTL: the cached set is reused, the newkidis not found, and every publisher request gets a 401 until the cache expires. Retry the lookup once with a forced JWKS refresh before rejecting the token so rotation works without waiting an hour.🔁 Possible fix
- jwks, err := fetchJWKS(cfg.KeyManagerConfigurations.JWKSUrl) + jwks, err := fetchJWKS(cfg.KeyManagerConfigurations.JWKSUrl, false) if err != nil { return nil, fmt.Errorf("failed to fetch JWKS: %w", err) } - // Find the key with matching kid - for _, key := range jwks.Keys { - if key.Kid == kid { - return convertJWKToPublicKey(&key) - } - } - - return nil, fmt.Errorf("unable to find key with kid: %s", kid) + findKey := func(keys []JSONWebKey) (*rsa.PublicKey, error) { + for _, key := range keys { + if key.Kid == kid { + return convertJWKToPublicKey(&key) + } + } + return nil, nil + } + + if key, err := findKey(jwks.Keys); err != nil { + return nil, err + } else if key != nil { + return key, nil + } + + jwks, err = fetchJWKS(cfg.KeyManagerConfigurations.JWKSUrl, true) + if err != nil { + return nil, fmt.Errorf("failed to refresh JWKS: %w", err) + } + if key, err := findKey(jwks.Keys); err != nil { + return nil, err + } else if key != nil { + return key, nil + } + + return nil, fmt.Errorf("unable to find key with kid: %s", kid)-func fetchJWKS(jwksURL string) (*JWKS, error) { +func fetchJWKS(jwksURL string, forceRefresh bool) (*JWKS, error) { jwksCacheMutex.RLock() - if jwksCache != nil && time.Since(jwksCacheTime) < jwksCacheTTL { + if !forceRefresh && jwksCache != nil && time.Since(jwksCacheTime) < jwksCacheTTL { defer jwksCacheMutex.RUnlock() return jwksCache, nil } jwksCacheMutex.RUnlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/middleware/jwtassertion/auth.go` around lines 155 - 200, The JWKS lookup should retry once with a forced refresh when the token's kid is not found to avoid failing closed during key rotation: inside the jwt.ParseWithClaims keyfunc (where you call fetchJWKS(cfg.KeyManagerConfigurations.JWKSUrl) and loop jwks.Keys to match key.Kid to kid), on the path that currently returns "unable to find key with kid" call fetchJWKS again forcing a refresh (or call a new fetchJWKSForced/fetchJWKS(cfg..., force=true)) and re-scan the returned jwks.Keys one more time; only if the second scan still misses return the error. Keep using convertJWKToPublicKey and preserve the existing signing-method/kid checks and error wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@agent-manager-service/middleware/jwtassertion/auth.go`:
- Around line 155-200: The JWKS lookup should retry once with a forced refresh
when the token's kid is not found to avoid failing closed during key rotation:
inside the jwt.ParseWithClaims keyfunc (where you call
fetchJWKS(cfg.KeyManagerConfigurations.JWKSUrl) and loop jwks.Keys to match
key.Kid to kid), on the path that currently returns "unable to find key with
kid" call fetchJWKS again forcing a refresh (or call a new
fetchJWKSForced/fetchJWKS(cfg..., force=true)) and re-scan the returned
jwks.Keys one more time; only if the second scan still misses return the error.
Keep using convertJWKToPublicKey and preserve the existing signing-method/kid
checks and error wrapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e72ffe0a-07f8-4b91-9006-d4b89de9a2e3
📒 Files selected for processing (3)
agent-manager-service/controllers/monitor_scores_publisher_controller.goagent-manager-service/middleware/jwtassertion/auth.godeployments/docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- deployments/docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- agent-manager-service/controllers/monitor_scores_publisher_controller.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
agent-manager-service/middleware/jwtassertion/auth.go (1)
198-211: Also validatenbfin the unsigned local-dev path.Fail-closing non-local environments is a good change. The remaining gap is that this branch only rejects expired tokens; a token with a future
nbfstill passes locally, which can hide time-based auth issues until later.⏱️ Small parity fix
if claims.ExpiresAt != nil && !claims.ExpiresAt.After(time.Now()) { return nil, fmt.Errorf("token has expired") } + if claims.NotBefore != nil && claims.NotBefore.After(time.Now()) { + return nil, fmt.Errorf("token is not valid yet") + } } else { return nil, fmt.Errorf("KEY_MANAGER_JWKS_URL must be configured for JWT validation") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agent-manager-service/middleware/jwtassertion/auth.go` around lines 198 - 211, In the local-dev branch (when cfg.IsLocalDevEnv) you currently only check claims.ExpiresAt; update the unsigned-path logic in the function that calls extractClaimsFromJWT(tokenString) to also reject tokens whose claims.NotBefore (nbf) is set in the future: after assigning claims = extractedClaims, add a check that if claims.NotBefore != nil and claims.NotBefore.After(time.Now()) then return an error like "token not valid yet (nbf)"; ensure you reference the same claims variable and preserve the existing expiresAt check so both time-based validations run in extractClaimsFromJWT caller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@agent-manager-service/middleware/jwtassertion/auth.go`:
- Around line 198-211: In the local-dev branch (when cfg.IsLocalDevEnv) you
currently only check claims.ExpiresAt; update the unsigned-path logic in the
function that calls extractClaimsFromJWT(tokenString) to also reject tokens
whose claims.NotBefore (nbf) is set in the future: after assigning claims =
extractedClaims, add a check that if claims.NotBefore != nil and
claims.NotBefore.After(time.Now()) then return an error like "token not valid
yet (nbf)"; ensure you reference the same claims variable and preserve the
existing expiresAt check so both time-based validations run in
extractClaimsFromJWT caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17b07032-2eeb-46fe-bff2-4851f8caeb00
📒 Files selected for processing (2)
agent-manager-service/middleware/jwtassertion/auth.godeployments/docker-compose.yml
✅ Files skipped from review due to trivial changes (1)
- deployments/docker-compose.yml
hanzjk
reviewed
Mar 24, 2026
| --set ampEvaluation.image.repository="amp-evaluation-monitor" \ | ||
| --set ampEvaluation.publisher.endpoint="http://agent-manager-service:8080" \ | ||
| --set ampEvaluation.publisher.apiKey="dev-publisher-api-key" | ||
| --set ampEvaluation.publisher.idpTokenUrl="http://amp-thunder-extension-service.amp-thunder.svc.cluster.local:8090/oauth2/token" \ |
Contributor
There was a problem hiding this comment.
Is the quick start guide script also updated?
Contributor
Author
There was a problem hiding this comment.
The quick-start install.sh / install-helpers.sh never explicitly set ampEvaluation.publisher.apiKey — that override only existed in setup-openchoreo.sh, which is updated in this PR. The quick-start deploys the evaluation and Thunder extension Helm charts using their default values, which now carry the correct OAuth2 config (idpTokenUrl, clientId). OpenBao is seeded via the values-openbao.yaml fetched from the repo at install time, which already has the amp-publisher-client-secret entry added in this PR. The only quick-start file that needed changing was uninstall.sh (evaluation namespace fix), and that's already included.
hanzjk
reviewed
Mar 24, 2026
| if c.apiKey == "" || subtle.ConstantTimeCompare([]byte(providedKey), []byte(c.apiKey)) != 1 { | ||
| log.Warn("Unauthorized access attempt to publish scores", "ip", getClientIP(r)) | ||
| http.Error(w, "Invalid or missing API key", http.StatusUnauthorized) | ||
| // Enforce publisher-only access: only the amp-publisher-client may call this endpoint |
Contributor
There was a problem hiding this comment.
Shall we remove this authorization logic from controller and move it to a middleware
Contributor
Author
There was a problem hiding this comment.
Done — moved to a PublisherClientAuthMiddleware in the jwtassertion package (commit 3dcdb0c4). The client ID is an internal constant inside the middleware. It is applied inside RegisterMonitorPublisherRoutes itself, so the call site in app.go stays consistent with all other route registrations.
hanzjk
reviewed
Mar 24, 2026
|
|
||
| // Register publisher routes outside JWT middleware (uses API-key auth instead) | ||
| // Register publisher routes with JWT middleware (uses OAuth2 client_credentials token) | ||
| publisherMux := http.NewServeMux() |
Contributor
There was a problem hiding this comment.
Do we need a separate mux for the publisher? Can Publisher routes now be part of apiMux?
Contributor
Author
There was a problem hiding this comment.
Done — the separate publisherMux and its duplicated middleware stack have been removed (commit 3dcdb0c4). Publisher routes are now registered directly on apiMux under the /publisher/ path prefix, so the external URL (/api/v1/publisher/monitors/.../scores) is unchanged.
hanzjk
reviewed
Mar 24, 2026
hanzjk
approved these changes
Mar 24, 2026
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
2 participants
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.
Purpose
Resolves: #436
Summary by CodeRabbit
New Features
Chores
Tests