Implement Auth

[gabriel-farache]

Implement Auth mechanism to protect the environment agent's endpoints (register, list SP, ...)
Health is left unprotected

[gabriel-farache]

/agentic_review

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Token validator lacks a nil guard ✓ Resolved 📘 Rule violation ≡ Correctness ⭐ New

Description

NewOIDCValidator forwards ctx to oidc.NewProvider without checking whether the required
context dependency is nil. A nil context supplied by a startup or test caller therefore reaches the
OIDC library instead of triggering the constructor's explicit dependency panic.

Code

internal/auth/jwt.go[R32-33]

+func NewOIDCValidator(ctx context.Context, issuerURL, audience string) (*OIDCValidator, error) {
+	provider, err := oidc.NewProvider(ctx, issuerURL)

Evidence

Compliance rule 2788523 requires constructors to panic when required dependencies are nil. The new
constructor accepts a required context.Context and immediately passes it to the OIDC provider
without a nil check.

Rule 2788523: Required constructor dependencies must panic on nil
internal/auth/jwt.go[32-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`NewOIDCValidator` does not explicitly reject a nil required context dependency.

## Fix Focus Areas
- internal/auth/jwt.go[32-35]

## Recommended Fix
Add a nil check at the beginning of `NewOIDCValidator` that panics with a clear message before calling `oidc.NewProvider`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Rejected requests lack audit logs ✓ Resolved 🐞 Bug ◔ Observability ⭐ New

Description

Server.Run registers RequestLogger inside authMW, while auth.Middleware writes a 401 and
returns without invoking its next handler, so the logger's deferred INFO event is never installed.
Every protected request with a missing, malformed, or invalid token therefore bypasses the normal
request audit trail, including its method, path, status, and duration.

Code

internal/apiserver/server.go[R58-59]

+	r.Use(s.authMW)
+	r.Use(RequestLogger(s.logger))

Evidence

The middleware ordering places authentication before the request logger, and each authentication
failure path returns without invoking the next handler. Because the request logger emits its INFO
record only from a deferred function installed after that middleware is entered, rejected requests
never reach the code that records the request and outcome.

internal/apiserver/server.go[55-59]
internal/auth/middleware.go[35-44]
internal/apiserver/middleware.go[39-65]
internal/apiserver/middleware.go[41-66]
.ai/specs/environment-agent.spec.md[145-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Authentication wraps `RequestLogger`, so missing, malformed, or invalid credentials return a 401 before the request logger is entered. Ensure rejected requests receive an equivalent structured audit log without changing the required authentication-before-request-logger ordering or losing JWT identity attributes on successful requests.

## Fix Focus Areas
- internal/apiserver/server.go[58-59]
- internal/auth/middleware.go[35-44]
- internal/apiserver/middleware.go[51-63]

## Recommended Fix
Preserve the existing middleware ordering while ensuring every authentication rejection emits exactly one per-request INFO audit record with method, path, status `401`, and duration. Either add equivalent logging to the authentication failure paths or restructure the shared logging context flow so early returns are recorded while successful requests retain the existing logger behavior and populated JWT identity attributes; add tests proving that missing and invalid credentials each produce exactly one request log entry.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Token failures expose verifier details ✓ Resolved 🐞 Bug ⛨ Security ⭐ New

Description

Middleware passes JWTValidator.Validate errors verbatim to writeAuthError, which serializes
them as the client-facing problem detail. Malformed, expired, incorrectly signed, or claim-decoding
failures can therefore disclose internal OIDC validation and parsing diagnostics to unauthenticated
callers.

Code

internal/auth/middleware.go[R41-44]

+			claims, err := cfg.JWTValidator.Validate(r.Context(), token)
+			if err != nil {
+				writeAuthError(w, r, cfg.Logger, err.Error())
+				return

Evidence

The validator preserves underlying verification and claim-extraction errors, middleware forwards
err.Error() as the detail, and the shared error writer serializes non-internal details without
redaction.

internal/auth/jwt.go[50-60]
internal/auth/middleware.go[41-44]
internal/auth/middleware.go[72-79]
internal/httperror/problem.go[13-24]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
OIDC verifier and claim-decoding errors are returned verbatim in unauthenticated HTTP responses, exposing internal validation diagnostics.

## Fix Focus Areas
- internal/auth/middleware.go[41-44]
- internal/auth/middleware.go[72-79]
- internal/auth/jwt.go[50-60]

## Recommended Fix
Log the underlying validation error server-side with appropriate request context, but pass a fixed generic detail such as `invalid Bearer token` to `writeAuthError`. Update tests to verify internal validator messages never appear in the response body.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View medium (3)

4. Other health methods skip authentication ✗ Dismissed 🐞 Bug ≡ Correctness

Description

Middleware exempts requests solely when r.URL.Path equals healthPath and does not require the
GET method. Any unauthenticated method at that path therefore reaches downstream routing and
validation, even though only GET is declared as the public health operation.

Code

internal/auth/middleware.go[R30-32]

+			if r.URL.Path == healthPath {
+				next.ServeHTTP(w, r)
+				return

Evidence

The bypass condition checks only the URL path, whereas the authentication specification and
generated route define the exemption as GET /api/v1alpha1/health. All other paths pass through
bearer extraction and validation, confirming that omission of the method check broadens the
exemption.

internal/auth/middleware.go[29-45]
.ai/specs/environment-agent.spec.md[2125-2128]
.ai/specs/environment-agent.spec.md[2155-2160]
internal/api/server/server.gen.go[301-306]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The health-path exemption bypasses authentication for every HTTP method instead of only the public GET health operation.

## Fix Focus Areas
- internal/auth/middleware.go[29-33]

## Recommended Fix
Require both `r.Method == http.MethodGet` and `r.URL.Path == healthPath` before bypassing authentication, and add a test showing another method at the health path still requires a bearer token.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Token checks outlast request deadlines ✓ Resolved 🐞 Bug ☼ Reliability

Description

Server.Run registers authMW outside RequestTimeout, so JWTValidator.Validate receives a
context without the configured per-request deadline. When validation must wait for OIDC key
retrieval or other verifier work, protected requests can remain active beyond
AGENT_SERVER_REQUEST_TIMEOUT and consume server resources until another cancellation occurs.

Code

internal/apiserver/server.go[57]

+	r.Use(s.authMW)

Evidence

The server registers authentication before the timeout middleware, while the auth middleware
validates the token before invoking its downstream handler. RequestTimeout creates the deadline
only when its own handler is entered, and the OIDC verifier uses the context supplied by auth, so
that deadline cannot govern validation.

internal/apiserver/server.go[55-59]
internal/auth/middleware.go[35-55]
internal/apiserver/middleware.go[71-93]
internal/auth/jwt.go[29-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Authentication executes before the request-timeout middleware, so token validation is not constrained by the configured request deadline.

## Fix Focus Areas
- internal/apiserver/server.go[55-59]

## Recommended Fix
Register `RequestTimeout` before `authMW`, while retaining panic recovery as the outermost middleware and authentication before `RequestLogger`, so the timeout context is passed into JWT validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Token failures lose operation context ✓ Resolved 📘 Rule violation ≡ Correctness

Description

OIDCValidator.Validate returns the verifier's error directly instead of wrapping it with operation
context. Any signature, expiry, issuer, or audience rejection takes this branch, so middleware and
logs receive only the dependency's wording.

Code

internal/auth/jwt.go[53]

+		return nil, err

Evidence

Compliance rule 2788501 requires returned errors to be wrapped with fmt.Errorf and %w; the new
validation branch returns the verifier error unchanged.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/auth/jwt.go[50-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`OIDCValidator.Validate` returns token verification errors without adding operation context.

## Fix Focus Areas
- internal/auth/jwt.go[50-54]

## Recommended Fix
Replace the bare error return with `fmt.Errorf("verifying token: %w", err)` so callers retain both contextual information and the original error chain.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Auth dependencies lack group comments ✗ Dismissed 📘 Rule violation ⚙ Maintainability ⭐ New

Description

MiddlewareConfig places its two required dependencies in an uncommented field list rather than
identifying their category. Because both dependencies are mandatory and enforced by panics, a later
field addition can be misclassified as optional or configuration without a visible grouping
boundary.

Code

internal/auth/middleware.go[R14-16]

+type MiddlewareConfig struct {
+	JWTValidator JWTValidator
+	Logger       *slog.Logger

Evidence

Compliance rule 2788534 requires struct fields to be grouped by category with comment headers. The
newly added MiddlewareConfig contains required injected dependencies but has no
required-dependencies group comment.

Rule 2788534: Struct fields should be grouped by category with comments
internal/auth/middleware.go[14-16]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new authentication middleware configuration does not label its required dependency fields as a logical group.

## Fix Focus Areas
- internal/auth/middleware.go[14-16]

## Recommended Fix
Add a `// required deps (injected, never nil after construction)` header above `JWTValidator` and `Logger`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Middleware tests remain mock-only ✗ Dismissed 📘 Rule violation ▣ Testability

Description

middleware_test.go substitutes mockValidator for token validation throughout the new middleware
suite rather than exercising OIDC discovery and verification through the server. Issuer discovery,
key retrieval, signature verification, claim extraction, and middleware wiring can therefore break
together without these tests detecting the failure.

Code

internal/auth/middleware_test.go[R24-25]

+func (m *mockValidator) Validate(_ context.Context, _ string) (*auth.JWTClaims, error) {
+	return m.claims, m.err

Evidence

Compliance rule 2788542 requires new tests to favor integrated real behavior over mocked simple
flows; the suite defines a validator that returns canned claims or errors and uses it in place of
the real OIDC implementation.

Rule 2788542: Prefer integration tests over unit tests
internal/auth/middleware_test.go[18-25]
internal/auth/middleware_test.go[103-110]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new authentication middleware tests use a canned validator and do not cover the real OIDC validation path across server and middleware layers.

## Fix Focus Areas
- internal/auth/middleware_test.go[18-25]
- internal/auth/middleware_test.go[103-110]

## Recommended Fix
Add an `_integration_test.go` suite that starts a local OIDC discovery and JWKS server, signs test tokens, constructs the real OIDC validator and API server, and verifies valid and invalid requests through HTTP. Retain focused unit tests only for isolated token-header parsing and boundary cases.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 17 rules

Review mode: 🧠 Deep: This push introduces security-sensitive JWT/OIDC authentication across middleware, server wiring, configuration, API contracts, and multiple independent paths, creating a defect-dense change where redundant review is materially valuable.

Tip of the day

💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 4e74523

Results up to commit 46dd469 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Other health methods skip authentication ✗ Dismissed 🐞 Bug ≡ Correctness

Description

Middleware exempts requests solely when r.URL.Path equals healthPath and does not require the
GET method. Any unauthenticated method at that path therefore reaches downstream routing and
validation, even though only GET is declared as the public health operation.

Code

internal/auth/middleware.go[R30-32]

+			if r.URL.Path == healthPath {
+				next.ServeHTTP(w, r)
+				return

Evidence

The bypass condition checks only the URL path, whereas the authentication specification and
generated route define the exemption as GET /api/v1alpha1/health. All other paths pass through
bearer extraction and validation, confirming that omission of the method check broadens the
exemption.

internal/auth/middleware.go[29-45]
.ai/specs/environment-agent.spec.md[2125-2128]
.ai/specs/environment-agent.spec.md[2155-2160]
internal/api/server/server.gen.go[301-306]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The health-path exemption bypasses authentication for every HTTP method instead of only the public GET health operation.

## Fix Focus Areas
- internal/auth/middleware.go[29-33]

## Recommended Fix
Require both `r.Method == http.MethodGet` and `r.URL.Path == healthPath` before bypassing authentication, and add a test showing another method at the health path still requires a bearer token.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Token checks outlast request deadlines ✓ Resolved 🐞 Bug ☼ Reliability

Description

Server.Run registers authMW outside RequestTimeout, so JWTValidator.Validate receives a
context without the configured per-request deadline. When validation must wait for OIDC key
retrieval or other verifier work, protected requests can remain active beyond
AGENT_SERVER_REQUEST_TIMEOUT and consume server resources until another cancellation occurs.

Code

internal/apiserver/server.go[57]

+	r.Use(s.authMW)

Evidence

The server registers authentication before the timeout middleware, while the auth middleware
validates the token before invoking its downstream handler. RequestTimeout creates the deadline
only when its own handler is entered, and the OIDC verifier uses the context supplied by auth, so
that deadline cannot govern validation.

internal/apiserver/server.go[55-59]
internal/auth/middleware.go[35-55]
internal/apiserver/middleware.go[71-93]
internal/auth/jwt.go[29-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Authentication executes before the request-timeout middleware, so token validation is not constrained by the configured request deadline.

## Fix Focus Areas
- internal/apiserver/server.go[55-59]

## Recommended Fix
Register `RequestTimeout` before `authMW`, while retaining panic recovery as the outermost middleware and authentication before `RequestLogger`, so the timeout context is passed into JWT validation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Token failures lose operation context ✓ Resolved 📘 Rule violation ≡ Correctness

Description

OIDCValidator.Validate returns the verifier's error directly instead of wrapping it with operation
context. Any signature, expiry, issuer, or audience rejection takes this branch, so middleware and
logs receive only the dependency's wording.

Code

internal/auth/jwt.go[53]

+		return nil, err

Evidence

Compliance rule 2788501 requires returned errors to be wrapped with fmt.Errorf and %w; the new
validation branch returns the verifier error unchanged.

Rule 2788501: Wrap errors with context using fmt.Errorf and %w
internal/auth/jwt.go[50-54]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`OIDCValidator.Validate` returns token verification errors without adding operation context.

## Fix Focus Areas
- internal/auth/jwt.go[50-54]

## Recommended Fix
Replace the bare error return with `fmt.Errorf("verifying token: %w", err)` so callers retain both contextual information and the original error chain.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Middleware tests remain mock-only ✗ Dismissed 📘 Rule violation ▣ Testability

Description

middleware_test.go substitutes mockValidator for token validation throughout the new middleware
suite rather than exercising OIDC discovery and verification through the server. Issuer discovery,
key retrieval, signature verification, claim extraction, and middleware wiring can therefore break
together without these tests detecting the failure.

Code

internal/auth/middleware_test.go[R24-25]

+func (m *mockValidator) Validate(_ context.Context, _ string) (*auth.JWTClaims, error) {
+	return m.claims, m.err

Evidence

Compliance rule 2788542 requires new tests to favor integrated real behavior over mocked simple
flows; the suite defines a validator that returns canned claims or errors and uses it in place of
the real OIDC implementation.

Rule 2788542: Prefer integration tests over unit tests
internal/auth/middleware_test.go[18-25]
internal/auth/middleware_test.go[103-110]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new authentication middleware tests use a canned validator and do not cover the real OIDC validation path across server and middleware layers.

## Fix Focus Areas
- internal/auth/middleware_test.go[18-25]
- internal/auth/middleware_test.go[103-110]

## Recommended Fix
Add an `_integration_test.go` suite that starts a local OIDC discovery and JWKS server, signs test tokens, constructs the real OIDC validator and API server, and verifies valid and invalid requests through HTTP. Retain focused unit tests only for isolated token-header parsing and boundary cases.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[gabriel-farache]

@chadcrum PTAL on this one for Auth in the agent :)

[qodo-code-review]

PR Summary by Qodo

Add OIDC JWT authentication for environment agent APIs

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Protects agent API endpoints with configurable OIDC-backed JWT Bearer authentication.
• Keeps health probes unauthenticated and supports disabled authentication for compatibility.
• Propagates identity claims into request logs and returns RFC 7807 authentication errors.

Diagram

sequenceDiagram
    actor Client
    participant Router as API Router
    participant Auth as Auth Middleware
    participant OIDC as OIDC Validator
    participant Keycloak
    participant Logger as Request Logger
    participant Handler as API Handler
    Client->>Router: API request
    Router->>Auth: Apply middleware
    alt Health or auth disabled
        Auth->>Logger: Forward request
        Logger->>Handler: Invoke handler
        Handler-->>Client: API response
    else Protected endpoint
        Auth->>OIDC: Validate Bearer token
        OIDC->>Keycloak: Discover issuer and JWKS
        Keycloak-->>OIDC: Signing keys
        OIDC-->>Auth: Identity claims
        alt Invalid token
            Auth-->>Client: RFC 7807 401
        else Valid token
            Auth->>Logger: Forward with claims
            Logger->>Handler: Invoke handler
            Handler-->>Client: API response
        end
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Ingress-only authentication

• ➕ Centralizes authentication outside the agent process.
• ➕ Avoids OIDC middleware and dependencies in the application.
• ➖ Direct service access could bypass protection.
• ➖ Authenticated claims would require trusted header propagation.
• ➖ Local development and request-level audit logging become less explicit.

2. OpenAPI validator authentication hook

• ➕ Couples security enforcement directly to the published OpenAPI contract.
• ➕ Could reduce separate route-bypass logic.
• ➖ Requires adapting OIDC validation to kin-openapi callbacks.
• ➖ Makes disabled mode and identity context propagation less straightforward.
• ➖ Provides less explicit control over middleware ordering and error responses.

Recommendation: Keep the dedicated auth middleware and injected JWTValidator interface. It provides explicit ordering, testable validation boundaries, claim propagation, and controlled RFC 7807 responses while still using the standard go-oidc verifier; ingress authentication may remain an additional defense layer rather than the sole enforcement point.

Files changed (21) +905 / -67

Enhancement (6) +225 / -10

main.goConstruct and inject configured authentication middleware +21/-1

Construct and inject configured authentication middleware

• Builds either disabled or OIDC-backed authentication middleware during startup and injects it into the API server. Logs missing audience warnings and fails startup when OIDC discovery cannot initialize.

cmd/environment-agent/main.go

middleware.goAttach authenticated identity to request logs +14/-6

Attach authenticated identity to request logs

• Reads JWT claims from request context and appends subject and preferred username attributes to completed-request log entries.

internal/apiserver/middleware.go

server.goInsert authentication into the HTTP middleware chain +11/-3

Insert authentication into the HTTP middleware chain

• Accepts injectable authentication middleware and places it before request logging and OpenAPI validation. Configures the OpenAPI validator to defer authentication enforcement to the dedicated middleware.

internal/apiserver/server.go

context.goProvide request-context storage for JWT claims +17/-0

Provide request-context storage for JWT claims

• Adds typed helpers for storing and retrieving validated identity claims from request contexts.

internal/auth/context.go

jwt.goImplement OIDC JWT validation and Bearer extraction +83/-0

Implement OIDC JWT validation and Bearer extraction

• Introduces the JWTValidator abstraction and a go-oidc implementation using issuer discovery and JWKS verification. Extracts subject and preferred username claims and parses case-insensitive Bearer headers.

internal/auth/jwt.go

middleware.goEnforce JWT authentication on protected endpoints +79/-0

Enforce JWT authentication on protected endpoints

• Adds enabled and disabled authentication middleware, exempts the health path, and propagates validated claims. Authentication failures return RFC 7807 HTTP 401 responses with a WWW-Authenticate header.

internal/auth/middleware.go

Tests (7) +346 / -3

server_integration_test.goAdapt server integration setup for auth injection +1/-1

Adapt server integration setup for auth injection

• Passes nil authentication middleware so existing HTTP server integration tests retain identity behavior.

internal/apiserver/server_integration_test.go

auth_suite_test.goCreate authentication Ginkgo test suite +13/-0

Create authentication Ginkgo test suite

• Registers the new internal/auth package test suite with Ginkgo and Gomega.

internal/auth/auth_suite_test.go

jwt_test.goTest Bearer token extraction boundaries +63/-0

Test Bearer token extraction boundaries

• Covers valid headers, missing headers, incorrect schemes, empty tokens, and case-insensitive Bearer schemes.

internal/auth/jwt_test.go

middleware_test.goTest authentication middleware behavior +221/-0

Test authentication middleware behavior

• Covers health bypass, missing and invalid credentials, valid claim propagation, disabled passthrough behavior, startup warnings, and RFC 7807 error bodies.

internal/auth/middleware_test.go

config_test.goTest authentication configuration validation +46/-0

Test authentication configuration validation

• Verifies enabled authentication requires an issuer while disabled mode and an optional audience remain valid configurations.

internal/config/config_test.go

health_integration_test.goAdapt health integration server construction +1/-1

Adapt health integration server construction

• Supplies nil authentication middleware to preserve existing health integration test behavior.

internal/health/health_integration_test.go

provider_integration_test.goAdapt provider integration server construction +1/-1

Adapt provider integration server construction

• Supplies nil authentication middleware when starting the provider integration test server.

internal/provider/provider_integration_test.go

Documentation (1) +133 / -5

environment-agent.spec.mdSpecify JWT authentication requirements and acceptance criteria +133/-5

Specify JWT authentication requirements and acceptance criteria

• Defines OIDC-backed Bearer authentication, the health bypass, disabled mode, configuration, identity logging, middleware ordering, and RFC 7807 failures. Updates scope statements, configuration tables, and requirement totals.

.ai/specs/environment-agent.spec.md

Other (7) +201 / -49

2026-07-17-16-28-unit-tests.mdAdd authentication unit-test plan and traceability +124/-0

Add authentication unit-test plan and traceability

• Adds UT-AUTH scenarios for token extraction, middleware behavior, error formatting, disabled mode, and configuration validation. Maps implemented authentication tests to their acceptance criteria.

.ai/test-plans/2026-07-17-16-28-unit-tests.md

MakefileInclude auth package in unit-test target +1/-1

Include auth package in unit-test target

• Adds internal/auth to the package list executed by the test-unit target.

Makefile

openapi.yamlDeclare global JWT Bearer security +13/-4

Declare global JWT Bearer security

• Adds a global bearerAuth security scheme and exempts the health operation. Unauthorized responses now describe missing or invalid JWT Bearer tokens.

api/v1alpha1/openapi.yaml

spec.gen.goRegenerate embedded OpenAPI specification +42/-41

Regenerate embedded OpenAPI specification

• Refreshes the generated compressed specification to include Bearer security requirements and the health exemption.

api/v1alpha1/spec.gen.go

go.modAdd OIDC token-validation dependencies +3/-1

Add OIDC token-validation dependencies

• Adds coreos/go-oidc and go-jose, while upgrading golang.org/x/oauth2 for OIDC support.

go.mod

go.sumRecord OIDC dependency checksums +6/-2

Record OIDC dependency checksums

• Adds checksums for go-oidc, go-jose, and the upgraded oauth2 module.

go.sum

config.goAdd and validate authentication configuration +12/-0

Add and validate authentication configuration

• Introduces disabled, issuer URL, and JWT audience environment settings. Validation requires an issuer URL whenever authentication is enabled.

internal/config/config.go

[qodo-code-review]

Code review by qodo was updated up to the latest commit 095ed65

[gabriel-farache]

Resolved 3 stale qodo-code-review threads that were left open after the 095ed65 → 888b199 amend already fixed them:

• internal/auth/jwt.go — nil-context guard in NewOIDCValidator (covered by UT-AUTH-015)
• internal/auth/middleware.go — validator errors no longer echoed to clients (covered by UT-AUTH-041/UT-AUTH-042)
• internal/apiserver/server.go — 401 rejections now emit an audit log via auth.LogRequest (covered by UT-AUTH-100/101/102 and IT-AUTH-120/121/122)

[chadcrum]

Medium: RequestLogger records the inner response before RequestTimeout can replace it with the client-facing 503. A timed-out request can therefore produce an audit record with status 200/401/etc. while the client receives 503. Can we emit the audit record after timeout finalization, or propagate the final status to the logger? It would also be good to add an integration assertion covering both the HTTP response and audit status.

[chadcrum]

IGNORE duplicate

[chadcrum]

LGTM

[jordigilh]

Second review summary

Substantive findings are posted inline below. The PR has one existing human approval. No live Keycloak or deployed-agent evidence is available.

Verdict: Not GA-ready

[jordigilh]

AGENT_AUTH_DISABLED defaults to true, but the operator documentation does not explain the issuer, audience, Keycloak mapper, or steps required to enable authentication. Please document the production configuration and fail-safe expectations.

[gabriel-farache]

@gciavarrini should this be done in the website once you PR dcm-project/dcm-project.github.io#22 is merged? And then add a reference link to the guide in this repo README
What is done here is similar to what is done for the CP

[gciavarrini]

Yes. Better putting agent auth enablement on the website (after docs PR dcm-project/dcm-project.github.io#22) same idea as the CP guide.
No need to block this PR

[gabriel-farache]

dcm-project/dcm-project.github.io#23

[gciavarrini]

lgtm

[machacekondra]

I think this is worth of enahcement to reason about why we have chosen this type of authn.

[gabriel-farache]

I think this is worth of enahcement to reason about why we have chosen this type of authn.

@machacekondra this PR is really an shamefulness replication of what is done in the Control Plane so I guess https://github.com/dcm-project/enhancements/blob/main/enhancements/authentication/authentication.md would still apply to this.

[jordigilh]

honestly, if you're not going to use TLS I fail to see the point of integrating authN/Z at all.

[gabriel-farache]

testing purposes to avoid issue with certificates

[jordigilh]

makes no sense to me: the business logic is constrained to use HTTP to avoid testing adding a layer of complexity with TLS? Am I reading it correctly?

[gabriel-farache]

I think I replied to quickly on this one
This http client is created for the refresh of the JWKS (from your comment #35 (comment))
and regarding TLS, this will be the scheme that will determine if it is used or not (http vs https) but we do not enforce HTTPS everywhere to allow easier dev testing; a warn is clearly logged when using http and its usage is discouraged in the doc as well

[jordigilh]

My concern is self signed certs will fail unless you load the CA into the client or into the cert store.

[gabriel-farache]

yes, in PROD you must use a certificate that is known from the org CA, while in dev/test you can just start in http mode to avoid that, at least that's how I imagined things
I had my fair share of battle with this kind of stuff so I just want to let dev/test go on plain http and not to worry about the certs issues

[jordigilh]

in air gapped environments, you could end up running with a self signed cert from the org. We have such case in our own environments.

[jordigilh]

I had my fair share of battle with this kind of stuff so I just want to let dev/test go on plain http and not to worry about the certs issues

I don't compromise security for test convenience.

[gabriel-farache]

in air gapped environments,

Is this a current requirement for this milestone? As far as I am aware, it is not but I may be mistaken

I don't compromise security for test convenience.

Me neither, in this example, it fall on the admin/user to configure properly the application, a warning message is even log to reduce the human-error factor
Our main target as customers are bank at the moment, so I am pretty sure they also have internal guidelines and safeguard to avoid a so obvious mis-configuration in PROD

Again, this should evolve when maturing the application but at our current state, logging a warning is enough and changing the behaviour to reject instead of warning has little to no impact

[jordigilh]

@gabriel-farache have you tested this locally in your environment to ensure it work with keycloak?

[gabriel-farache]

@gabriel-farache have you tested this locally in your environment to ensure it work with keycloak?

@jordigilh I did it once but I did not create test for it, I have now 1343619

This mimic what is done in the control-plane repo as the code is similar

But at least now there the beginning of the start of subsystem/e2e tests in the agent and not just IT

[jordigilh]

@jordigilh I did it once but I did not create test for it, I have now 1343619

I would normally not advise to do this, but in this case since there are no e2e automation and this is green field that we're adding, I'd ask you to perform an e2e validation to ensure all the pieces work together, otherwise when QE runs this they might get unexpected surprises.

Unless auth is not required in the milestone and we can skip testing for now. In that case I'm fine

[jordigilh]

Health is left unprotected

@gabriel-farache who can reach this endpoint? I'm concerned about DDoS attacks. Perhaps for now it's not worth the pain, just raising it here for awareness.
Also, is the env agent instrumented to emit metrics at all? The fact that we only expose the health made me wonder if we have a metrics port or not that exposes metrics in prometheus format. A bit late for the milestone, but this and support for OTEL should be considered for a next release in my opinion.

[gabriel-farache]

Health is left unprotected

@gabriel-farache who can reach this endpoint? I'm concerned about DDoS attacks. Perhaps for now it's not worth the pain, just raising it here for awareness. Also, is the env agent instrumented to emit metrics at all? The fact that we only expose the health made me wonder if we have a metrics port or not that exposes metrics in prometheus format. A bit late for the milestone, but this and support for OTEL should be considered for a next release in my opinion.

@jordigilh I think it's common practice to leave the health unprotected as it is used to probe the liveliness/readiness in K8s. I think the CP is doing the same
We could put the health on a separate port so we could separate what we want to expose to the external world (the current agent's endpoints) and what we want to keep internal to the cluster (health)
I'll open an issue for follow up on this.
Currently there is no metrics either but I'll open an issue for it too

@jordigilh I did it once but I did not create test for it, I have now 1343619

I would normally not advise to do this, but in this case since there are no e2e automation and this is green field that we're adding, I'd ask you to perform an e2e validation to ensure all the pieces work together, otherwise when QE runs this they might get unexpected surprises.

Unless auth is not required in the milestone and we can skip testing for now. In that case I'm fine

WDYM? The subsystem test is spinning all real services (keycloak, the agent, nats) and validate the authN is working on the protected endpoints

[jordigilh]

@jordigilh I think it's common practice to leave the health unprotected as it is used to probe the liveliness/readiness in K8s.

Yes, but in k8s there is a virtual network that prevents this port from being accessed outside the cluster. If env agent is deployed as a quadlet, can we ensure a similar protection?

[jordigilh]

WDYM? The subsystem test is spinning all real services (keycloak, the agent, nats) and validate the authN is working on the protected endpoints

Just that: end to end validation with keycloak to ensure a successful authN.

[gabriel-farache]

@jordigilh I think it's common practice to leave the health unprotected as it is used to probe the liveliness/readiness in K8s.

If env agent is deployed as a quadlet, can we ensure a similar protection?

@jordigilh No but right now I would discard this concern as AFAIK, our target to deploy is K8s no?
It's a valid point, just not the right time to consider it IMO

WDYM? The subsystem test is spinning all real services (keycloak, the agent, nats) and validate the authN is working on the protected endpoints

Just that: end to end validation with keycloak to ensure a successful authN.

Ok, it's already there so cool :)

[jordigilh]

@jordigilh No but right now I would discard this concern as AFAIK, our target to deploy is K8s no?
It's a valid point, just not the right time to consider it IMO

yes, we can discard this, as I mentioned it's not a blocker, just raising this for awareness. This is a perfect case where narrowing down the platform would've reduced the amount of work required to get it ready. Having so many deployment platforms is a huge headache from the start.

[jordigilh]

lgtm