Repository navigation
docs: document environment agent authentication (inbound + outbound) - #23
Conversation
5f5b858 to
9c8592d
Compare
9c8592d to
c7d50c7
Compare
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can switch off images and animations for a plain-text comment |
PR Summary by QodoDocument inbound and outbound environment agent authentication
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="content/docs/getting-started/environment-agent-authentication.md" line_range="183" />
<code_context>
auth is enabled.
> **Service providers:** Service providers reach the control plane through the
-> environment agent, not direct HTTP calls. End-to-end authentication for the
</code_context>
<issue_to_address>
**Agent credential enables impersonation**
When an operator follows the reference setup and an attacker gains access to the agent's environment, the agent's `DCM_AUTH_CLIENT_SECRET` is also `AUTH_PROXY_SECRET`, so an attacker who extracts the agent environment can send forged `X-Forwarded-User` and `X-Forwarded-Preferred-Username` headers to the reachable control plane and be resolved as another actor, including a privileged one.
Recommend a dedicated OAuth client secret for the agent and keep it distinct from `AUTH_PROXY_SECRET`.
</issue_to_address>
### Comment 2
<location path="content/docs/getting-started/environment-agent-authentication.md" line_range="79-80" />
<code_context>
+> clients for service accounts (used by SPs registering with the agent) don't
+> ship with an audience mapper out of the box. Rather than reject every token
+> from an unmapped client, the agent skips the audience check when
+> `AGENT_AUTH_JWT_AUDIENCE` is empty, and only enforces `aud` matching when you
+> explicitly set it. Set it once your SP clients have a matching audience
+> mapper.
+
</code_context>
<issue_to_address>
**Unrelated realm tokens reach agent API**
When `AGENT_AUTH_JWT_AUDIENCE` is left at its documented empty default and a caller has a valid token for another client in the issuer realm, the agent validates the token's issuer but skips audience validation, so a bearer token issued for an unrelated client in the same realm can call the provider registration and listing APIs, exposing them to callers who were not issued agent tokens.
Recommend configuring a dedicated audience and matching SP client mappers before enabling inbound authentication in production, and treat an unset audience as development-only.
</issue_to_address>
### Comment 3
<location path="content/docs/getting-started/environment-agent-authentication.md" line_range="40" />
<code_context>
+as the only interim mitigation.
+
+`GET /api/v1alpha1/health` is the only endpoint that stays public — the bypass
+checks both the method and the path, so `GET /health` is unauthenticated but any
+other method at that path still requires a token.
+
</code_context>
<issue_to_address>
**Health probes miss endpoint**
When a reader follows the literal `/health` path when configuring or calling a health probe, the reader requests a path different from the documented public route `/api/v1alpha1/health`, so the health check is not reached.
Use `/api/v1alpha1/health` in the bypass sentence, or explicitly identify `/health` as shorthand or an internal router path.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: content/docs/getting-started/environment-agent-authentication.md:183, content/docs/getting-started/environment-agent-authentication.md:80, content/docs/getting-started/environment-agent-authentication.md:40
y-first
left a comment
y-first
left a comment
There was a problem hiding this comment.
Good, thorough coverage of both auth surfaces. A few issues to address before merge — see inline comments.
| as the only interim mitigation. | ||
|
|
||
| `GET /api/v1alpha1/health` is the only endpoint that stays public — the bypass | ||
| checks both the method and the path, so `GET /health` is unauthenticated but any |
There was a problem hiding this comment.
[HIGH] Path inconsistency in health-bypass sentence
The sentence uses the bare path /health while the full endpoint /api/v1alpha1/health is used everywhere else in the file (lines 39, 110, 113). A reader could misread this as meaning only the short path is public or that there are two separate routes.
Suggest:
checks both the method and the path, so `GET /api/v1alpha1/health` is unauthenticated but any
other method at that path still requires a token.
There was a problem hiding this comment.
Updated: only GET /api/v1alpha1/health bypasses authentication; every other request, including other methods at that exact path, requires a token. This matches the middleware's exact method and path check (internal/auth/middleware.go).
| `DCM_REGISTRATION_MAX_BACKOFF`). A failed heartbeat is logged and retried on | ||
| the next heartbeat tick, controlled by `AGENT_HEARTBEAT_INTERVAL` (30 seconds | ||
| by default); heartbeat failures do not use the registration backoff. | ||
| - **Static tokens do not refresh.** Once the token expires, the control plane |
There was a problem hiding this comment.
[MEDIUM] Static token expiry also halts heartbeat — only registration is called out
This sentence says registration halts, but when the static token is expired the control plane returns 401 on both the registration call and every heartbeat call. A user keeping a long-lived agent running could miss that heartbeat silently stops succeeding too.
Suggest:
returns `401`, which the agent treats as non-retryable — registration and heartbeat both halt
until the agent is restarted with a fresh token.
There was a problem hiding this comment.
I checked the current registrar and client code. A 401 is non-retryable for registration, so registration retries stop. After a successful registration, though, the heartbeat ticker continues calling sendHeartbeat and logs each failure; the next tick tries again (internal/dcm/registrar.go, internal/dcm/client.go). I updated the guide to say heartbeat attempts continue but fail until restart with a fresh token. I did not use “heartbeat halts” because it does not match the implementation.
| ```bash | ||
| curl -s \ | ||
| -H "Authorization: Bearer ${SP_TOKEN}" \ | ||
| -H "Content-Type: application/json" \ |
There was a problem hiding this comment.
[MEDIUM] Port 8081 appears without context here
A reader who hasn't set up the environment agent yet won't know where port 8081 comes from. This section doesn't explain how the agent is actually started (--with-environment-agent flag in deploy-dcm.sh), nor that 8081 is its default API port (overridable via --agent-port / AGENT_PORT, which matters when combining with a standalone KubeVirt SP that also uses 8081).
Suggest adding a note before the curl example, or a cross-reference sentence in the intro, e.g.:
The agent is started alongside the DCM stack via
deploy-dcm.sh --with-environment-agent; see Local Setup. The default API port is8081(override with--agent-port/AGENT_PORT).
There was a problem hiding this comment.
Added the agent launch flags, the required --agent-embedded-sps list, the 8081 default and overrides, and the port conflict with the standalone KubeVirt SP. The launch requirements and port behavior are verified against the utilities deployment script and its README.
| | Symptom | Things to check | | ||
| | -------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | SP registration returns `401` from the agent | Auth enabled on the agent (`AGENT_AUTH_DISABLED=false`); the SP's token is valid and not expired; issuer matches `AGENT_AUTH_ISSUER_URL`; if `AGENT_AUTH_JWT_AUDIENCE` is set, the token's `aud` must match. | | ||
| | Agent registration/heartbeat to the control plane returns `401` | Check the `DCM_AUTH_*` client credentials and token endpoint, then verify that the token issuer and (when enabled) audience match the control plane's `AUTH_*` settings. Add a `dcm-api` mapper for a custom client if required; the reference `dcm-proxy` client already has one. | |
There was a problem hiding this comment.
[LOW] aud shorthand reads as jargon in end-user context
The rest of the guide (including this table's own row for AGENT_AUTH_JWT_AUDIENCE) uses the word "audience" consistently. Using aud bare here without expansion could confuse a reader who hasn't memorised JWT claim names.
Suggest: "informational unless you intended to enforce the aud claim".
There was a problem hiding this comment.
Expanded the configuration row to “Expected token audience (the aud claim)” and changed the troubleshooting text to “token audience claim” so the JWT shorthand is introduced and explained.
| ### How it works | ||
|
|
||
| 1. At startup, the agent discovers OIDC metadata from the issuer. When it | ||
| validates a token, it fetches signing keys from the issuer's JWKS endpoint as |
There was a problem hiding this comment.
This says the agent fetches JWKS keys at startup. It fetches the OIDC config document at startup (which has the JWKS URL). The actual signing keys come later, on first token check. Maybe Discovers the issuer's OIDC configuration is more accurate?
There was a problem hiding this comment.
I tightened this sentence to distinguish the two steps: startup OIDC discovery reads issuer metadata, including the JWKS URL; signing keys are fetched on first token validation as needed and cached. This matches NewOIDCValidator in internal/auth/jwt.go.
| as the only interim mitigation. | ||
|
|
||
| Only `GET /api/v1alpha1/health` stays public. The bypass checks both the method | ||
| and exact path, so `GET /health` and every other method or path require a token. |
There was a problem hiding this comment.
The first half of this sentence says the public path is /api/v1alpha1/health, but then the second half shortens it to /health. The agent has no /health route, no?
There was a problem hiding this comment.
Updated to name the complete public route, GET /api/v1alpha1/health, and removed the /health shorthand. Only that method and exact path bypass authentication, matching internal/auth/middleware.go.
Adds a dedicated Environment Agent Authentication guide covering the two independent auth surfaces introduced by environment-agent#35 and environment-agent#38: - Inbound: AGENT_AUTH_DISABLED / AGENT_AUTH_ISSUER_URL / AGENT_AUTH_JWT_AUDIENCE protecting the agent's own REST API (external SP registration, provider listing), health-path bypass, RFC 7807 error format, and the intentional audience fail-open behavior. - Outbound: DCM_AUTH_TOKEN / DCM_AUTH_TOKEN_ENDPOINT / DCM_AUTH_CLIENT_ID / DCM_AUTH_CLIENT_SECRET for the agent's own registration/heartbeat calls to the control plane, mode precedence, token refresh and failure behavior, Keycloak service-account setup, and the known reference-realm audience-mapper gap. Addresses the open documentation request from environment-agent#35 (review thread on internal/config/config.go, dcm-project/environment-agent#35 (comment)): 'put agent auth enablement on the website... same idea as the CP guide.' Content validated against the latest reviewed commits on both PR branches (config.go, jwt.go, middleware.go, token.go, client.go, main.go, openapi.yaml, README.md, decisions.md) rather than assumed from the PR descriptions alone. Updates the control-plane Authentication guide's Service-providers callout and troubleshooting row to link to the new page instead of a vague 'still landing' note, and cross-links from Local Setup and the Getting Started index. Signed-off-by: Gabriel Farache <gfarache@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: gabriel-farache <gfarache@redhat.com>
Signed-off-by: gabriel-farache <gfarache@redhat.com>
Signed-off-by: gabriel-farache <gfarache@redhat.com>
fa17e15 to
4aabeab
Compare
Adds a dedicated Environment Agent Authentication guide covering the two independent auth surfaces introduced by environment-agent#35 and environment-agent#38:
dcm-proxyclient with audience-mapper guidance for custom clients.Addresses the open documentation request from
environment-agent#35 (review thread on internal/config/config.go, dcm-project/environment-agent#35 (comment)): 'put agent auth enablement on the website... same idea as the CP guide.'
Content validated against the latest reviewed commits on both PR branches (config.go, jwt.go, middleware.go, token.go, client.go, main.go, openapi.yaml, README.md, decisions.md) rather than assumed from the PR descriptions alone.
Updates the control-plane Authentication guide to replace the obsolete 'still landing' callout and distinguish SP-to-agent API authentication from agent-to-control-plane registration and heartbeat authentication in troubleshooting. Cross-links the new guide from Local Setup and the Getting Started index.
Summary by Sourcery
Document the environment agent's separate inbound and outbound authentication surfaces and update related getting-started guidance.
New Features:
Enhancements:
Documentation: