ci: Add CI/build for unified authbridge, deprecate old components (Phase 4)#313
Conversation
pdettori
left a comment
There was a problem hiding this comment.
Review Summary
Well-structured rewrite that replaces three separate Go codebases (go-processor, waypoint, klaviger) with a single modular binary. The architecture is clean: authlib/ is a pure Go library with no gRPC/Envoy deps, cmd/authbridge/ adds thin protocol listeners. 80 tests cover positive/negative paths with -race. All 10 commits are DCO signed-off.
Main feedback: the JWT verification error detail in DenyReason and the unbounded GracefulStop are worth addressing — one is a security hygiene issue, the other a potential stuck-shutdown in production.
Areas reviewed: Go code, CI/GitHub Actions, Dockerfile, Shell, YAML, Docs
Commits: 10 commits, all signed-off ✓
CI status: DCO passing
Verdict: COMMENT — suggestions only, no blocking issues
Highlights
- Fail-closed design (nil verifier → deny) is exactly right for a security component
io.LimitReader(resp.Body, 1<<20)— good defense against malicious token endpoints- Context propagation via
NewRequestWithContextthroughout is solid - Clean protocol-agnostic auth layer with thin listener translators
4 of 10 commits lack emoji prefix (47d24e3, 0eaf70c, b0d9dbb, ec50850) — minor convention gap.
| DenyStatus: http.StatusUnauthorized, | ||
| DenyReason: "invalid Authorization header format", | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: fmt.Sprintf("token validation failed: %v", err) — this JWT verification error is forwarded to the client via both ext_authz and ext_proc deny responses. Errors can reveal issuer URL, expected audience, or algorithm details.
Consider logging the detail server-side but returning a generic "token validation failed" to the client:
a.log.Info("JWT validation failed", "error", err)
return &InboundResult{
Action: ActionDeny,
DenyStatus: http.StatusUnauthorized,
DenyReason: "token validation failed",
}|
|
||
| // Resolve config into auth dependencies | ||
| resolved, err := config.Resolve(ctx, cfg) | ||
| if err != nil { |
There was a problem hiding this comment.
suggestion: srv.GracefulStop() blocks indefinitely without using shutdownCtx. If a gRPC streaming connection stays open (e.g. ext_proc), the process hangs past the 15s SIGTERM grace period.
Consider wrapping in a goroutine with a timer that falls back to srv.Stop():
for _, srv := range grpcServers {
s := srv
go func() {
<-shutdownCtx.Done()
s.Stop() // force after timeout
}()
s.GracefulStop()
}There was a problem hiding this comment.
Already fixed in PR #312 review round. Phase 3 rebased to pick it up — goroutine calls srv.Stop() when shutdownCtx expires.
| } | ||
| for _, srv := range httpServers { | ||
| srv.Shutdown(shutdownCtx) | ||
| } |
There was a problem hiding this comment.
nit: http.Server has no ReadHeaderTimeout. Go defaults to infinite, which is susceptible to slowloris-style resource exhaustion. Minor concern since this is an internal sidecar, but cheap defense-in-depth:
srv := &http.Server{
Addr: addr,
Handler: handler,
ReadHeaderTimeout: 10 * time.Second,
}There was a problem hiding this comment.
Fixed. Added ReadHeaderTimeout: 10 * time.Second to all HTTP servers.
| COPY cmd/authbridge/ cmd/authbridge/ | ||
|
|
||
| # The go.mod has: replace authlib => ../../authlib | ||
| # In Docker the layout is /app/authlib and /app/cmd/authbridge, so the |
There was a problem hiding this comment.
nit: envoyproxy/envoy:v1.37.1 is pinned to a mutable tag while the UBI9 base correctly uses a SHA digest (@sha256:8300...). Consider SHA-pinning the Envoy image too for reproducible builds.
There was a problem hiding this comment.
Already fixed in PR #312 review round. Envoy image pinned by SHA digest.
Address review from @pdettori on PR rossoctl#313: - Add ReadHeaderTimeout: 10s to HTTP servers (slowloris defense) - Add pipefail to entrypoint.sh set flags Findings 1 (JWT error leakage) fixed in PR rossoctl#311. Findings 2 (GracefulStop) and 4 (Envoy SHA pin) already fixed in PR rossoctl#312 review round. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
b2e3de8 to
7e996bd
Compare
Address review from @pdettori on PR rossoctl#313: - Add ReadHeaderTimeout: 10s to HTTP servers (slowloris defense) - Add pipefail to entrypoint.sh set flags Findings 1 (JWT error leakage) fixed in PR rossoctl#311. Findings 2 (GracefulStop) and 4 (Envoy SHA pin) already fixed in PR rossoctl#312 review round. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Address review from @pdettori on PR rossoctl#313: - Add ReadHeaderTimeout: 10s to HTTP servers (slowloris defense) - Add pipefail to entrypoint.sh set flags Findings 1 (JWT error leakage) fixed in PR rossoctl#311. Findings 2 (GracefulStop) and 4 (Envoy SHA pin) already fixed in PR rossoctl#312 review round. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
b5bc8d9 to
74112ac
Compare
Address review from @pdettori on PR rossoctl#313: - Add ReadHeaderTimeout: 10s to HTTP servers (slowloris defense) - Add pipefail to entrypoint.sh set flags Findings 1 (JWT error leakage) fixed in PR rossoctl#311. Findings 2 (GracefulStop) and 4 (Envoy SHA pin) already fixed in PR rossoctl#312 review round. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…ase 4) CI changes: - Add Go CI jobs for authlib and cmd/authbridge modules - Add authbridge-unified image to build.yaml matrix Documentation: - Update CLAUDE.md with unified binary architecture, Go modules, and modes - Mark authbridge-unified as recommended image - Mark envoy-with-processor and old authbridge as deprecated Deprecation markers: - go-processor/main.go: deprecated in favor of cmd/authbridge - Dockerfile.envoy: deprecated in favor of cmd/authbridge/Dockerfile - entrypoint-envoy.sh: deprecated in favor of cmd/authbridge/entrypoint.sh Follow-up (separate repos): - kagenti-operator: add authbridge-unified to image config - kagenti: add authbridge-unified-config ConfigMap to Helm chart Ref: rossoctl#279 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
74112ac to
f5b0c80
Compare
Summary
Phase 4 of the modular token exchange library (#279). Makes the unified authbridge production-ready with CI, build pipeline, documentation, and deprecation markers.
Depends on: #312 (Phase 3), #311 (Phase 2), #310 (Phase 1)
CI changes
Go CI (authlib)job for the authlib module (lint, build, test with -race -cover)Go CI (authbridge cmd)job for the cmd/authbridge moduleauthbridge-unifiedimage to build.yaml matrixDocumentation
authbridge-unifiedas the recommended imageenvoy-with-processorand oldauthbridgeimages as deprecatedDeprecation markers
go-processor/main.go,Dockerfile.envoy,entrypoint-envoy.shmarked deprecated with pointers to replacementsFollow-up (separate repos)
authbridge-unifiedto image config inkagenti-platform-configConfigMap. The operator already supports image overrides viaPlatformConfig.Images.authbridge-unified-configConfigMap to Helm chartagent-namespaces.yamltemplate, generated from existing Helm values.Related issue(s)
Ref #279
Test plan
authbridge-unifiedimageAssisted-By: Claude (Anthropic AI) noreply@anthropic.com