Conversation
Signed-off-by: Nagarjun Krishnan <nkrishnan@ancestry.com>
There was a problem hiding this comment.
Pull request overview
Adds support for external authorization via a configurable webhook (OPA-style by default), including request/decision types, provider adapters, and controller wiring.
Changes:
- Introduces
AuthzRequest/AuthzDecisionand updates theAuthorizerinterface to return structured decisions. - Adds
ExternalAuthorizer+ OPA provider adapter (wire-format translation) with accompanying tests. - Wires Helm/env configuration into the controller and updates handler authz checks (fail-open on system errors).
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| helm/kagent/values.yaml | Adds Helm values for external authz endpoint/provider configuration. |
| helm/kagent/templates/controller-configmap.yaml | Exposes external authz settings as env vars for the controller. |
| go/pkg/auth/auth.go | Defines authz request/decision types; updates Authorizer and Session interfaces. |
| go/pkg/auth/external_authz_test.go | Adds tests around the Authorizer interface contract using a mock authorizer. |
| go/pkg/app/app.go | Minor formatting/alignment in config struct fields. |
| go/internal/httpserver/handlers/sessions_test.go | Updates tests to use the new SimpleSession constructor and claims. |
| go/internal/httpserver/handlers/helpers.go | Updates handler-side authorization checks to use AuthzRequest + fail-open behavior. |
| go/internal/httpserver/auth/provider.go | Introduces provider adapter interface + factory (ProviderByName). |
| go/internal/httpserver/auth/provider_opa.go | Implements OPA wire-format adapter. |
| go/internal/httpserver/auth/provider_test.go | Adds tests for OPA provider marshal/unmarshal and provider factory. |
| go/internal/httpserver/auth/external_authz.go | Implements HTTP transport authorizer that calls an external endpoint. |
| go/internal/httpserver/auth/external_authz_test.go | Adds transport + integration tests for external authz (direct + OPA formats). |
| go/internal/httpserver/auth/authz.go | Updates NoopAuthorizer to the new Authorizer interface. |
| go/internal/httpserver/auth/authn.go | Adds claims support to SimpleSession and introduces a constructor. |
| go/internal/httpserver/auth/authn_test.go | Extends authn middleware tests to assert default claims behavior. |
| go/cmd/controller/main.go | Wires authorizer selection based on env vars; adds external authz init. |
| docs/architecture/opa-authorization.md | Documents external authorization architecture, provider adapters, and configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) | ||
| // to ensure that exec-entrypoint and run can make use of them. | ||
| _ "k8s.io/client-go/plugin/pkg/client/auth" | ||
| pkgauth "github.com/kagent-dev/kagent/go/pkg/auth" |
There was a problem hiding this comment.
The controller previously blank-imported k8s.io/client-go/plugin/pkg/client/auth to enable exec-based Kubernetes auth plugins (GCP/Azure/OIDC, etc.). Removing that import can break cluster authentication in environments that rely on those plugins; re-add the blank import (and its explanatory comment) so default controller-runtime config loading continues to work across providers.
| pkgauth "github.com/kagent-dev/kagent/go/pkg/auth" | |
| pkgauth "github.com/kagent-dev/kagent/go/pkg/auth" | |
| // Import all Kubernetes client auth plugins (GCP, Azure, OIDC, etc.) | |
| // to ensure exec-based authentication works when using the default | |
| // client-go credential loading mechanism. | |
| _ "k8s.io/client-go/plugin/pkg/client/auth" |
| log.Error(err, "authorization check failed, allowing access (fail-open)") | ||
| return nil | ||
| } | ||
| if decision != nil && !decision.Allowed { |
There was a problem hiding this comment.
If authorizer.Check() returns (nil, nil), this helper currently treats it as allowed access. That’s contrary to the implied contract (no error should yield a concrete decision) and can silently bypass authorization if an implementation fails to construct a decision. Consider treating decision == nil as a system error (log + fail-open) or as a denial, but do not implicitly allow without any signal.
| if decision != nil && !decision.Allowed { | |
| if decision == nil { | |
| // Treat a nil decision without error as a system issue; log and fail-open | |
| log.Error(fmt.Errorf("nil authorization decision"), "authorization check returned nil decision, allowing access (fail-open)") | |
| return nil | |
| } | |
| if !decision.Allowed { |
| return nil | ||
| } | ||
| if decision != nil && !decision.Allowed { | ||
| return errors.NewForbiddenError(decision.Reason, fmt.Errorf("access denied: %s", decision.Reason)) |
There was a problem hiding this comment.
When decision.Reason is empty, this returns a forbidden API error with an empty message and an access denied: wrapped error. Consider falling back to a default message (e.g., "Not authorized") when the reason is blank, while still preserving the structured reason when provided.
| return errors.NewForbiddenError(decision.Reason, fmt.Errorf("access denied: %s", decision.Reason)) | |
| reason := strings.TrimSpace(decision.Reason) | |
| if reason == "" { | |
| reason = "Not authorized" | |
| } | |
| return errors.NewForbiddenError(reason, fmt.Errorf("access denied: %s", reason)) |
| func (a *ExternalAuthorizer) Check(ctx context.Context, req auth.AuthzRequest) (*auth.AuthzDecision, error) { | ||
| body, err := a.Provider.MarshalRequest(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal authz request: %w", err) | ||
| } | ||
|
|
||
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, a.Endpoint, bytes.NewReader(body)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("create authz request: %w", err) | ||
| } | ||
| httpReq.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := a.Client.Do(httpReq) |
There was a problem hiding this comment.
ExternalAuthorizer.Check will panic if Provider or Client is nil. Since ExternalAuthorizer is a public struct and can be instantiated via zero values, it’s safer to validate these dependencies up front and return a descriptive error (or default Client to http.DefaultClient) to avoid runtime panics from misconfiguration.
| --external-authz-endpoint=http://opa:8181/v1/data/kagent/authz | ||
| --authz-provider=opa |
There was a problem hiding this comment.
This doc section advertises CLI flags (--external-authz-endpoint, --authz-provider), but the controller wiring shown in this PR reads only environment variables (EXTERNAL_AUTHZ_ENDPOINT, AUTHZ_PROVIDER). Either implement these flags (and connect them to the authorizer init) or remove/update the CLI flags section to avoid sending users to non-existent configuration options.
| --external-authz-endpoint=http://opa:8181/v1/data/kagent/authz | |
| --authz-provider=opa | |
| # CLI flags for external authorization are not currently supported. | |
| # Configure kagent using environment variables instead: | |
| # EXTERNAL_AUTHZ_ENDPOINT=http://opa:8181/v1/data/kagent/authz | |
| # AUTHZ_PROVIDER=opa |
Signed-off-by: Nagarjun Krishnan <159933832+nujragan93@users.noreply.github.com>
No description provided.