This repository was archived by the owner on Sep 9, 2026. It is now read-only.
NO-ISSUE: Replace Authorino with built-in JWT authentication and OPA authorization - #685
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 22, 2026
Conversation
|
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:
WalkthroughRemoves Authorino/external auth Helm templates and interceptors, adds JWKS cache, JwtValidator, and embedded-Rego gRPC authn/authz interceptors; updates startup wiring and CA trust loading. Risk: High — security-critical changes to authentication and authorization runtime behavior. ChangesAuthentication & Authorization Architecture Replacement
Sequence DiagramsequenceDiagram
participant Client
participant GrpcAuthnInterceptor
participant JwtValidator
participant JwksCache
participant GrpcAuthzInterceptor
participant RegoPolicy
participant Handler
Client->>GrpcAuthnInterceptor: gRPC request with Authorization: Bearer {JWT}
GrpcAuthnInterceptor->>JwtValidator: Validate(bearer)
JwtValidator->>JwksCache: Get(iss, kid)
JwksCache-->>JwtValidator: RSA public key (cached)
JwtValidator-->>GrpcAuthnInterceptor: validated *jwt.Token
GrpcAuthnInterceptor->>GrpcAuthzInterceptor: forward context with token
GrpcAuthzInterceptor->>RegoPolicy: evaluate(input: token+method+metadata)
RegoPolicy-->>GrpcAuthzInterceptor: allow/deny + subject attributes
alt allow == true
GrpcAuthzInterceptor->>Handler: invoke with Subject in context
Handler-->>Client: response
else
GrpcAuthzInterceptor-->>Client: PermissionDenied / Unauthenticated gRPC error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (3)
333-339:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRestore auth before transaction management.
The unary chain now opens the database transaction before authn/authz. That means malformed or unauthenticated requests still consume database capacity before they are rejected, and it breaks the documented server bootstrap order for this command. If the authorization interceptor needs project metadata, give it a read path that doesn't depend on the request transaction instead of reordering the whole chain.
As per coding guidelines, the gRPC server uses chained interceptors in the order: panic recovery, Prometheus metrics, structured logging, authentication, database transaction management.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 333 - 339, The interceptor order currently opens a DB transaction before authentication/authorization; reorder the grpc.ChainUnaryInterceptor entries so panicInterceptor.UnaryServer, metricsInterceptor.UnaryServer, loggingInterceptor.UnaryServer are followed by authnInterceptor.UnaryServer and authzInterceptor.UnaryServer, and only then txInterceptor.UnaryServer, restoring the documented order; if authz requires project metadata, change authzInterceptor to fetch metadata via a read-only non-transactional path instead of relying on txInterceptor.Source: Coding guidelines
1170-1173:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
--ca-filehelp text.It still says the CA bundle is used for TLS connections to the external auth service, but this command now uses built-in JWKS authentication and Rego authorization. The current wording will send operators to the wrong remediation path when issuer/JWKS trust fails.
Based on PR objectives, external Authorino-based auth was removed in favor of built-in JWKS authentication and OPA authorization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 1170 - 1173, The help constant caFileFlagHelp still references "external auth service" and Authorino; update the text in the caFileFlagHelp constant to say the CA bundle is used to validate TLS when fetching JWKS/issuer metadata and for connections to OPA/Rego policy endpoints (built-in JWKS authentication and Rego authorization), so operators are guided to trust the issuer/JWKS source and policy endpoints rather than an external Authorino service; edit the constant caFileFlagHelp to reflect this new wording.
98-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently ignore
--tenancy-logic.Line 276 now always builds the default tenancy logic, but Lines 98-101 and 1180-1183 still expose the flag and advertise
guestas a valid value. Any deployment that still sets--tenancy-logic=guestwill change authorization behavior with no warning. Either reject non-defaultvalues or deprecate this flag as ignored, like the removed auth flags.Also applies to: 269-280, 1180-1183
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 98 - 101, The CLI currently accepts a --tenancy-logic flag (runner.args.tenancyLogic) but always uses the default implementation, silently ignoring other values (e.g., "guest"); add explicit validation after flag parsing to either reject non-"default" values with a clear error (returning a non-zero exit and message like "--tenancy-logic other-than-default is unsupported") or mark the flag deprecated by returning an error/warning when any non-"default" value is provided; also update the flag registration (tenancy-logic help text) to reflect that only "default" is supported or that the flag is deprecated so callers see it in help output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/grpc_authn_interceptor.go`:
- Around line 689-699: The discovery handler currently accepts discoDoc.JwksUri
verbatim and stores it in issuerInfo.jwksUrl, allowing non-HTTPS schemes; update
this code to parse discoDoc.JwksUri (e.g., via url.Parse) and verify that the
scheme is "https" before assigning to issuerInfo.jwksUrl, returning an error if
not https; also apply the same scheme validation to the JWKS fetch path that
uses issuerInfo.jwksUrl (the code that retrieves the key set) so any discovered
or fetched JWKS URL is rejected unless its parsed URL.Scheme == "https".
- Around line 567-583: The cache uses only kid and is shared across issuers
causing key collisions; change caching and lookup to be issuer-scoped by
composing a cache key from issuer+kid (e.g., issuer+"|"+kid) or store keys in a
per-issuer map and select the appropriate issuer keyset before verification;
update uses of i.keys.Load, i.keys.Store and the refresh path (i.refreshKeys) so
refreshes populate issuer-scoped entries and lookups use the issuer+kid
composite key (also apply the same fix to the similar logic around the other
lookup block that mirrors this code).
- Around line 567-587: The current key lookup (i.keys.Load(kid)) returns cached
keys indefinitely and only adds/updates entries in refreshKeys, which lets
revoked JWKS keys remain trusted; change refreshKeys(ctx) to build a new fresh
key set (not mutate the existing map), then atomically swap i.keys to the new
set (e.g., replace the sync.Map or map pointer) so deleted keys disappear;
additionally add a TTL check in the verification path (where i.keys.Load(kid) is
used) to trigger refreshKeys when the cached set is older than the TTL even on
cache hits, and ensure refreshKeys returns an error on refresh failure without
leaving a stale pointer in place.
- Around line 486-490: The log call using i.logger.ErrorContext currently emits
the raw bearer token (variable bearer); remove the plaintext token from logs and
instead log a safe surrogate (e.g., token length, a masked substring, or a short
hash) while still recording the error (err) and context (ctx). Update the
i.logger.ErrorContext invocation that currently passes slog.String("!token",
bearer) to pass a non-sensitive field such as slog.Int("token_len", len(bearer))
or slog.String("token_sig", maskedSig) or slog.String("token_hash", shortHash)
and keep slog.Any("error", err) so parsing errors are retained without exposing
credentials.
In `@internal/auth/grpc_authz_interceptor_metadata_test.go`:
- Around line 53-64: The tests currently ignore the error returned by
interceptor.UnaryServer and don't assert the handler ran; update each
UnaryServer invocation (e.g., the calls to interceptor.UnaryServer in the
Projects/Get/Delete/Update cases) to capture the returned error into a variable,
add Expect(err).ToNot(HaveOccurred()) after the call, and instrument the handler
closure with a local handled boolean that the closure sets true when executed
and assert Expect(handled).To(BeTrue()) on the success path; apply the same
pattern to the other occurrences mentioned (lines covering the other UnaryServer
calls at the other test cases).
In `@internal/auth/grpc_authz_interceptor.go`:
- Around line 273-277: The debug logs currently dump full OPA input/result maps
via i.logger.DebugContext (the calls that pass slog.Any("input", input) and the
similar result logging), which risks leaking identity/PII; replace those calls
so they no longer serialize whole maps and instead log only bounded derived
fields (e.g., RPC method, allow/deny boolean from result, subject username or id
extracted from the input claims, and an object or tenant id if present). Locate
the DebugContext calls that reference the variables input and result and remove
slog.Any("input", input)/slog.Any("result", result); extract and log only
specific fields (method, allow/deny, subject id/email, object/tenant id) with
explicit slog.String/slog.Bool entries and omit any tokens, full claim sets, or
unbounded maps.
- Around line 250-257: authorizeWithoutToken currently grants Guest purely from
the regex whitelist (isAnonymousMethod) and skips evaluating the Rego policy
(data.authz); change it so anonymous access is decided by the Rego policy: when
no token, call the existing Rego policy evaluator (the same path that checks
data.authz elsewhere in this package) with the method and subject set to Guest
(or equivalent anonymous principal), and only when the policy allows set result
= ContextWithSubject(ctx, Guest); otherwise return the Unauthenticated
grpcstatus error. Remove or keep isAnonymousMethod only as an optimization that
still triggers a policy check, and ensure the decision uses the policy
evaluation result rather than the regex alone.
In `@internal/auth/policies/authz.rego`:
- Around line 167-256: The policy never reads the metadata injected by
internal/auth/grpc_authz_interceptor.go (input.context.context_extensions) so
Projects/Get/Delete/Update remain unreachable for non-admins; fix by adding
explicit allow rules for the Projects methods (e.g., grpc_method in
{"/osac.public.v1.Projects/Get","/osac.public.v1.Projects/Delete","/osac.public.v1.Projects/Update"})
that consume the injected metadata (read the tenant/name from
input.context.context_extensions, validate against the caller's
visibility/permissions like has_client_permissions or a new
has_project_permissions predicate) and return allow when the metadata-backed
checks pass, or if you prefer revert the change remove the metadata injection in
grpc_authz_interceptor.go until the policy is updated.
---
Outside diff comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 333-339: The interceptor order currently opens a DB transaction
before authentication/authorization; reorder the grpc.ChainUnaryInterceptor
entries so panicInterceptor.UnaryServer, metricsInterceptor.UnaryServer,
loggingInterceptor.UnaryServer are followed by authnInterceptor.UnaryServer and
authzInterceptor.UnaryServer, and only then txInterceptor.UnaryServer, restoring
the documented order; if authz requires project metadata, change
authzInterceptor to fetch metadata via a read-only non-transactional path
instead of relying on txInterceptor.
- Around line 1170-1173: The help constant caFileFlagHelp still references
"external auth service" and Authorino; update the text in the caFileFlagHelp
constant to say the CA bundle is used to validate TLS when fetching JWKS/issuer
metadata and for connections to OPA/Rego policy endpoints (built-in JWKS
authentication and Rego authorization), so operators are guided to trust the
issuer/JWKS source and policy endpoints rather than an external Authorino
service; edit the constant caFileFlagHelp to reflect this new wording.
- Around line 98-101: The CLI currently accepts a --tenancy-logic flag
(runner.args.tenancyLogic) but always uses the default implementation, silently
ignoring other values (e.g., "guest"); add explicit validation after flag
parsing to either reject non-"default" values with a clear error (returning a
non-zero exit and message like "--tenancy-logic other-than-default is
unsupported") or mark the flag deprecated by returning an error/warning when any
non-"default" value is provided; also update the flag registration
(tenancy-logic help text) to reflect that only "default" is supported or that
the flag is deprecated so callers see it in help output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8f327bdf-4371-44ab-8e39-df611c9a0a6e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
charts/service/templates/authorino/authorino.yamlcharts/service/templates/authorino/certificate.yamlcharts/service/templates/grpc-server/authconfig.yamlgo.modinternal/auth/auth_context.gointernal/auth/auth_headers.gointernal/auth/auth_rules_test.gointernal/auth/auth_script_token_source_test.gointernal/auth/auth_static_token_source_test.gointernal/auth/grpc_authn_interceptor.gointernal/auth/grpc_authn_interceptor_test.gointernal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_metadata_test.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_metadata_test.gointernal/auth/grpc_external_auth_interceptor_test.gointernal/auth/grpc_guest_auth_interceptor.gointernal/auth/grpc_guest_auth_interceptor_test.gointernal/auth/policies/authz.regointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/kubernetes/files/kubernetes_files.gointernal/network/cert_pool.gointernal/testing/tokens.goit/it_multitenancy_test.go
💤 Files with no reviewable changes (9)
- charts/service/templates/authorino/certificate.yaml
- internal/auth/grpc_guest_auth_interceptor_test.go
- charts/service/templates/authorino/authorino.yaml
- internal/auth/auth_rules_test.go
- internal/auth/grpc_guest_auth_interceptor.go
- charts/service/templates/grpc-server/authconfig.yaml
- internal/auth/grpc_external_auth_interceptor_test.go
- internal/auth/grpc_external_auth_interceptor.go
- internal/auth/grpc_external_auth_interceptor_metadata_test.go
jhernand
force-pushed
the
simplify_auth
branch
2 times, most recently
from
June 12, 2026 16:49
c09e10b to
91823e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (1)
269-281: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueLow-risk: Dead code path from ignored
tenancyLogicflag.The log message at line 273 still logs
c.args.tenancyLogic, but the switch logic is removed and only default tenancy is created. This creates operator confusion—logs suggest the flag was honored when it's actually ignored.Risk: Low — no security impact, but operators may misdiagnose configuration issues.
Suggested fix
// Create the tenancy logic: c.logger.InfoContext( ctx, "Creating tenancy logic", - slog.String("type", c.args.tenancyLogic), + slog.String("type", "default"), ) var tenancyLogic auth.TenancyLogic🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 269 - 281, The log claims it’s creating the tenancy logic for c.args.tenancyLogic but the code always builds the default tenancy via auth.NewDefaultTenancyLogic and ignores the flag; update the behavior so the log reflects reality: either (A) honor the flag by adding the switch/selection that sets tenancyLogic based on c.args.tenancyLogic, or (B) remove/adjust the c.args.tenancyLogic field from the c.logger.InfoContext call and log that the default tenancy is being created (use tenancyLogic or a literal like "default" in the c.logger.InfoContext call). Modify the code around NewDefaultTenancyLogic, the tenancyLogic variable, and the c.logger.InfoContext invocation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/grpc_authn_interceptor_test.go`:
- Around line 311-332: The test name says it should accept a token without the
'typ' claim but it currently uses MakeTokenString which always sets 'typ';
replace the MakeTokenString call with the token helper that builds tokens from
explicit claims (e.g., the MakeTokenWithClaims/MakeTokenFromClaims helper in
internal/testing/tokens.go) and pass a claims map that omits the "typ" key so
the created token has no 'typ' claim; keep the rest of the test
(metadata.Authorization, interceptor.UnaryServer, noopHandler) unchanged.
In `@internal/auth/grpc_authn_interceptor.go`:
- Around line 647-665: The refreshKeys implementation currently replaces the
entire keysCache and uses a hardcoded 1*time.Minute throttle; change it to honor
the configured TTL (use i.keysTTL or SetKeysTTL) for throttling and to perform
per-issuer updates: call loadKeys into newCache as now but do not wholesale swap
i.keysCache — instead iterate newCache and for each issuer key that successfully
loaded, update i.keysCache.Store(issuer, value), and for issuers that failed
keep the existing i.keysCache entry untouched; only update i.lastRefresh when
the configured TTL has elapsed and/or when at least one issuer was successfully
refreshed; keep holding keysCacheLock while swapping/updating to preserve
concurrency safety and reference functions/vars refreshKeys, loadKeys,
i.keysCache, newCache, i.lastRefresh and the configured TTL variable (i.keysTTL
/ SetKeysTTL).
- Around line 287-293: The http.Client created for JWKS fetching lacks timeouts
and does not respect caller context; update the creation of httpClient in
grpc_authn_interceptor.go to (1) configure Transport.DialContext (or Dialer) and
ResponseHeaderTimeout/IdleConnTimeout and set http.Client.Timeout to reasonable
values, and (2) ensure JWKS fetch calls use context-aware requests (e.g., use
http.NewRequestWithContext or pass the incoming context down) so that functions
handling cache misses/refresh (the JWKS refresh path) honor cancellation from
the gRPC context; reference the httpClient variable, tls.Config with b.caPool,
and the JWKS refresh call sites to wire the context and timeouts.
In `@internal/auth/policies/authz.rego`:
- Around line 82-89: The Rego rules use redundant existence checks after
assignments: remove the extra boolean checks so the rule bodies rely on the
left-hand-side assignments to fail when the input path is undefined;
specifically update the subject_tenants rules (remove the trailing
`input.auth.identity.organization` and `input.auth.identity.organizations`
conditions and keep only `input.auth.identity.authnMethod == "jwt"` plus the
assignment) and likewise remove the separate `input.auth.identity.realm_access`
/ `.roles` existence checks and rely on the assignment to `realm_access`/`roles`
to determine presence.
- Around line 82-95: The two subject_tenants rules can both produce values when
a JWT has both input.auth.identity.organization and
input.auth.identity.organizations, causing OPA "complete rules" errors; make the
branches mutually exclusive by adding negative guards so only one rule can match
(e.g., in the rule that uses input.auth.identity.organization add "not
input.auth.identity.organizations", and in the rule that uses
input.auth.identity.organizations add "not input.auth.identity.organization"),
or alternatively decide a precedence (choose organization over organizations or
vice versa) and implement that precedence via guard checks; keep the
subject_tenants = subject_groups fallback as-is for when neither claim exists
and ensure you reference the existing symbols subject_tenants,
input.auth.identity.organization, input.auth.identity.organizations,
subject_groups, and input.auth.identity.authnMethod when editing the rules.
---
Outside diff comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 269-281: The log claims it’s creating the tenancy logic for
c.args.tenancyLogic but the code always builds the default tenancy via
auth.NewDefaultTenancyLogic and ignores the flag; update the behavior so the log
reflects reality: either (A) honor the flag by adding the switch/selection that
sets tenancyLogic based on c.args.tenancyLogic, or (B) remove/adjust the
c.args.tenancyLogic field from the c.logger.InfoContext call and log that the
default tenancy is being created (use tenancyLogic or a literal like "default"
in the c.logger.InfoContext call). Modify the code around
NewDefaultTenancyLogic, the tenancyLogic variable, and the c.logger.InfoContext
invocation accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7bda577b-e96b-4ff8-8477-f85d32e1a053
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
charts/service/templates/authorino/authorino.yamlcharts/service/templates/authorino/certificate.yamlcharts/service/templates/grpc-server/authconfig.yamlgo.modinternal/auth/auth_context.gointernal/auth/auth_headers.gointernal/auth/auth_rules_test.gointernal/auth/auth_script_token_source_test.gointernal/auth/auth_static_token_source_test.gointernal/auth/grpc_authn_interceptor.gointernal/auth/grpc_authn_interceptor_test.gointernal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_metadata_test.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_metadata_test.gointernal/auth/grpc_external_auth_interceptor_test.gointernal/auth/grpc_guest_auth_interceptor.gointernal/auth/grpc_guest_auth_interceptor_test.gointernal/auth/policies/authz.regointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/kubernetes/files/kubernetes_files.gointernal/network/cert_pool.gointernal/testing/tokens.goit/it_multitenancy_test.go
💤 Files with no reviewable changes (9)
- charts/service/templates/authorino/certificate.yaml
- charts/service/templates/authorino/authorino.yaml
- internal/auth/grpc_guest_auth_interceptor_test.go
- charts/service/templates/grpc-server/authconfig.yaml
- internal/auth/grpc_external_auth_interceptor_test.go
- internal/auth/auth_rules_test.go
- internal/auth/grpc_external_auth_interceptor.go
- internal/auth/grpc_external_auth_interceptor_metadata_test.go
- internal/auth/grpc_guest_auth_interceptor.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (1)
333-340:⚠️ Potential issue | 🟠 MajorMajor: Move authn/authz ahead of txInterceptor to prevent DB transaction start on unauthenticated/denied requests (internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go:333-340, internal/database/database_tx_interceptor.go:77-90)
Current unary chain starts a transaction (
i.manager.Begin(ctx)) before authn/authz run, so even requests that will be rejected still consume DB transaction/pool capacity—expanding the DoS surface on a security-critical path.grpc.ChainUnaryInterceptor( panicInterceptor.UnaryServer, metricsInterceptor.UnaryServer, loggingInterceptor.UnaryServer, txInterceptor.UnaryServer, authnInterceptor.UnaryServer, authzInterceptor.UnaryServer, ),Reorder so authentication happens before database transaction management (at least move
authnInterceptorbeforetxInterceptor; moveauthzInterceptorbeforetxInterceptorunless the specific authz path truly requires DB-backed metadata to be inside the transaction).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 333 - 340, The unary interceptor chain currently calls txInterceptor.UnaryServer before authnInterceptor.UnaryServer and authzInterceptor.UnaryServer, which starts a DB transaction (i.manager.Begin) for requests that may be rejected; reorder the interceptors in the grpc.ChainUnaryInterceptor call so authnInterceptor.UnaryServer (and authzInterceptor.UnaryServer, unless that authorization requires DB metadata inside the transaction) appear before txInterceptor.UnaryServer to ensure authentication/authorization run prior to starting a DB transaction.Source: Coding guidelines
♻️ Duplicate comments (3)
internal/auth/grpc_authn_interceptor_test.go (1)
311-332:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not exercise the missing-
typscenario.The test name says "Accepts token without 'typ' claim", but Line 318 uses
MakeTokenStringwhich always adds atypclaim (perinternal/testing/tokens.go). The test passes a normal Bearer token withtyp: "Bearer"instead of a token lacking thetypclaim entirely.Suggested fix
- token := MakeTokenString(issuerUrl, "Bearer", time.Minute) + token := MakeTokenObject(jwt.MapClaims{ + "iss": issuerUrl, + "sub": "user-123", + }).Raw🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/grpc_authn_interceptor_test.go` around lines 311 - 332, The test name claims to verify behavior for a token missing the 'typ' claim but currently uses MakeTokenString (which always sets typ), so update the test to create a JWT that truly lacks the 'typ' claim: replace the call to MakeTokenString(issuerUrl, "Bearer", time.Minute) with a token constructed without the typ field (either by adding a new helper like MakeTokenWithoutTyp(issuerUrl, time.Minute) or by building the raw claims map and signing it with the same test signer), then place that token into metadata and call interceptor.UnaryServer (same as existing) to assert no error; keep references to NewGrpcAuthnInterceptor, MakeTokenString (for context), interceptor.UnaryServer, and noopHandler so reviewers can locate the change.internal/auth/grpc_authn_interceptor.go (2)
647-711:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRefresh can evict working issuer keys on transient issuer failure.
loadKeyslogs per-issuer errors but always returnsnil(Line 711), sorefreshKeysalways swaps innewCacheeven when some issuers failed to load. After a TTL-triggered refresh, a brief outage for one issuer deletes that issuer's previously working keys and causes valid tokens from that issuer to be rejected until the next successful refresh.Additionally, the hardcoded 1-minute throttle (Line 650) ignores the configured
keysTTL(which can be set viaSetKeysTTL), so TTL values below a minute are not honored as advertised.Severity: Valid tokens can be rejected during transient issuer outages; medium-to-high risk depending on issuer reliability.
Recommended fix
Keep the previous cache entry for each issuer unless that issuer's refresh succeeds:
func (i *GrpcAuthnInterceptor) refreshKeys(ctx context.Context) error { i.keysCacheLock.Lock() defer i.keysCacheLock.Unlock() - if time.Since(i.lastRefresh) > 1*time.Minute { + if i.keysTTL > 0 && time.Since(i.lastRefresh) > i.keysTTL { newCache := &sync.Map{} err := i.loadKeys(ctx, newCache) if err != nil { return err } - i.keysCache.Range(func(key, value any) bool { - i.keysCache.Delete(key) - return true - }) + // Only replace keys for issuers that successfully loaded newCache.Range(func(key, value any) bool { i.keysCache.Store(key, value) return true }) i.lastRefresh = time.Now() } return nil }Then update
loadKeysto track per-issuer success and only store keys incachefor successful issuers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/grpc_authn_interceptor.go` around lines 647 - 711, refreshKeys currently uses a hardcoded 1-minute throttle and blindly replaces the live i.keysCache with newCache even if some issuers failed to load, which can evict working keys; change the time check to use the configured i.keysTTL (set via SetKeysTTL) instead of 1*time.Minute, and make loadKeys track per-issuer success (e.g., return a map of issuer id -> whether loadJwksUrl succeeded, or populate only successful entries in the provided cache). Then in refreshKeys, after loadKeys returns, merge only the successful issuer entries from newCache into i.keysCache (leaving existing entries for issuers that failed), updating i.lastRefresh only if at least one issuer succeeded. Use the existing functions/fields discoverJwksUrl, loadJwksUrl, loadKeys, refreshKeys, i.keysCache, i.lastRefresh, and i.keysTTL when implementing this behavior.
287-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeouts and TLS MinVersion to the HTTP client.
The HTTP client lacks timeouts, creating an availability risk. On cache miss (Line 626), JWKS refresh is synchronous; a slow or unresponsive issuer endpoint can hang authentication indefinitely when the gRPC context has no deadline. Additionally, the TLS configuration does not specify
MinVersion, which defaults to TLS 1.2 for clients—acceptable but not ideal for new code.Severity: Availability degradation under slow/failing issuer endpoints; medium risk.
Suggested fix
httpClient := &http.Client{ + Timeout: 30 * time.Second, Transport: &http.Transport{ + ResponseHeaderTimeout: 30 * time.Second, + TLSHandshakeTimeout: 10 * time.Second, TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, RootCAs: b.caPool, }, }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/grpc_authn_interceptor.go` around lines 287 - 293, The HTTP client created in grpc_authn_interceptor.go lacks request timeouts and a TLS MinVersion, which can hang JWKS refreshes on cache miss; modify the httpClient instantiation (the httpClient variable/Transport TLSClientConfig) to set a sensible client-level Timeout (e.g., 5–15s), configure Transport timeouts such as TLSHandshakeTimeout and IdleConnTimeout, and set TLSClientConfig.MinVersion (e.g., tls.VersionTLS12 or tls.VersionTLS13); additionally ensure the JWKS refresh code path that uses this client performs requests with the caller gRPC context (use req.WithContext(ctx) or pass the context into the HTTP call) so refreshes honor request deadlines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/grpc_authz_interceptor_test.go`:
- Around line 490-604: The test suite currently only covers tenant scope from
the "organization" claim; add tests to exercise the JWT fallback/precedence
logic by creating new cases using createKeycloakUserToken + ContextWithToken and
invoking interceptor.UnaryServer (same pattern as existing tests) that assert
SubjectFromContext and permission behavior: (1) a token with no
"organization"/"organizations" but with "groups" yields the tenant(s) from
groups and authorizes/denies accordingly, (2) a token whose "groups" include the
reserved admin role name exercises tenant-admin behavior (expect tenant-admin
privileges on public user management RPCs), and (3) a token carrying both
"organization" and "organizations" verifies the claim-precedence path (ensure
the resulting Subject.Tenants matches the intended precedence and permissions).
Ensure each case asserts handled vs denied via grpcstatus/grpccodes and uses the
same UnaryServer call sites like "/osac.public.v1.Users/Create" or
"/osac.private.v1.Hubs/Create" to validate authorization outcomes.
In `@internal/auth/policies/authz.rego`:
- Around line 73-75: The current JWT fallback uses subject_groups =
input.auth.identity.groups and then treats those same groups as subject_tenants
and as the global admin signal (is_admin), which allows a groups claim like
"admins" to escalate tenant membership into cluster-wide admin; fix this by
separating tenant membership from admin detection: stop using
input.auth.identity.groups directly as subject_tenants for JWTs and instead read
tenant membership from an explicit tenant/organization claim (e.g.,
input.auth.identity.organizations or input.auth.identity.organization) or only
from a namespaced tenant claim, and derive is_admin only from a dedicated admin
claim/role (e.g., input.auth.identity.roles or input.auth.identity.is_admin) or
from a reserved admin group name/prefix that cannot overlap with tenant names;
additionally, remove or narrow the groups fallback on privileged JWT paths
(those that rely on organization/organizations) so the groups fallback is not
applied to compute subject_tenants or grant ["*"] tenants; update the related
rules that reference subject_groups, subject_tenants and is_admin (including the
other occurrences noted around the other blocks) to use the new claims/filters.
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 275-281: The code always constructs auth.NewDefaultTenancyLogic()
(via NewDefaultTenancyLogic().SetLogger(c.logger).Build()) making the CLI flag
--tenancy-logic=guest a silent no-op; update runner.run to respect the CLI
value: read the tenancy flag (the variable backing the flag), switch on its
value and either construct the corresponding logic (e.g.,
auth.NewGuestTenancyLogic() when "guest") or return a clear error/usage message
for unsupported values (reject/deprecate non-default values), and mirror the
same fix where the same pattern occurs (the NewDefaultTenancyLogic construction
at the other location referenced) so the flag is honored consistently. Ensure to
surface a helpful error if an unknown tenancy string is provided.
- Line 76: Fix three issues: stop discarding errors from flags.MarkDeprecated
for "grpc-authn-type" and "grpc-authn-external-address" by checking/returning
the error instead of using `_ = ...`; honor the parsed tenancy flag by wiring
c.args.tenancyLogic into the server build (replace the hardcoded
auth.NewDefaultTenancyLogic() call with logic that selects tenancy
implementation based on c.args.tenancyLogic, including support for the "guest"
value, and update tenancyLogicFlagHelp to reflect actual supported values); and
reorder middleware in the grpc.ChainUnaryInterceptor call so authn/authz
interceptors (authnInterceptor.UnaryServer and authzInterceptor.UnaryServer) run
before txInterceptor.UnaryServer to ensure authentication/authorization occurs
before starting DB transactions. Ensure each change references the existing
symbols: flags.MarkDeprecated, c.args.tenancyLogic, auth.NewDefaultTenancyLogic,
tenancyLogicFlagHelp, grpc.ChainUnaryInterceptor, txInterceptor.UnaryServer,
authnInterceptor.UnaryServer, and authzInterceptor.UnaryServer.
---
Outside diff comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 333-340: The unary interceptor chain currently calls
txInterceptor.UnaryServer before authnInterceptor.UnaryServer and
authzInterceptor.UnaryServer, which starts a DB transaction (i.manager.Begin)
for requests that may be rejected; reorder the interceptors in the
grpc.ChainUnaryInterceptor call so authnInterceptor.UnaryServer (and
authzInterceptor.UnaryServer, unless that authorization requires DB metadata
inside the transaction) appear before txInterceptor.UnaryServer to ensure
authentication/authorization run prior to starting a DB transaction.
---
Duplicate comments:
In `@internal/auth/grpc_authn_interceptor_test.go`:
- Around line 311-332: The test name claims to verify behavior for a token
missing the 'typ' claim but currently uses MakeTokenString (which always sets
typ), so update the test to create a JWT that truly lacks the 'typ' claim:
replace the call to MakeTokenString(issuerUrl, "Bearer", time.Minute) with a
token constructed without the typ field (either by adding a new helper like
MakeTokenWithoutTyp(issuerUrl, time.Minute) or by building the raw claims map
and signing it with the same test signer), then place that token into metadata
and call interceptor.UnaryServer (same as existing) to assert no error; keep
references to NewGrpcAuthnInterceptor, MakeTokenString (for context),
interceptor.UnaryServer, and noopHandler so reviewers can locate the change.
In `@internal/auth/grpc_authn_interceptor.go`:
- Around line 647-711: refreshKeys currently uses a hardcoded 1-minute throttle
and blindly replaces the live i.keysCache with newCache even if some issuers
failed to load, which can evict working keys; change the time check to use the
configured i.keysTTL (set via SetKeysTTL) instead of 1*time.Minute, and make
loadKeys track per-issuer success (e.g., return a map of issuer id -> whether
loadJwksUrl succeeded, or populate only successful entries in the provided
cache). Then in refreshKeys, after loadKeys returns, merge only the successful
issuer entries from newCache into i.keysCache (leaving existing entries for
issuers that failed), updating i.lastRefresh only if at least one issuer
succeeded. Use the existing functions/fields discoverJwksUrl, loadJwksUrl,
loadKeys, refreshKeys, i.keysCache, i.lastRefresh, and i.keysTTL when
implementing this behavior.
- Around line 287-293: The HTTP client created in grpc_authn_interceptor.go
lacks request timeouts and a TLS MinVersion, which can hang JWKS refreshes on
cache miss; modify the httpClient instantiation (the httpClient
variable/Transport TLSClientConfig) to set a sensible client-level Timeout
(e.g., 5–15s), configure Transport timeouts such as TLSHandshakeTimeout and
IdleConnTimeout, and set TLSClientConfig.MinVersion (e.g., tls.VersionTLS12 or
tls.VersionTLS13); additionally ensure the JWKS refresh code path that uses this
client performs requests with the caller gRPC context (use req.WithContext(ctx)
or pass the context into the HTTP call) so refreshes honor request deadlines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 6a44252c-6d1e-47d0-b5e3-3f69cf692ca6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
charts/service/templates/authorino/authorino.yamlcharts/service/templates/authorino/certificate.yamlcharts/service/templates/grpc-server/authconfig.yamlgo.modinternal/auth/auth_context.gointernal/auth/auth_headers.gointernal/auth/auth_rules_test.gointernal/auth/auth_script_token_source_test.gointernal/auth/auth_static_token_source_test.gointernal/auth/grpc_authn_interceptor.gointernal/auth/grpc_authn_interceptor_test.gointernal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_metadata_test.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_metadata_test.gointernal/auth/grpc_external_auth_interceptor_test.gointernal/auth/grpc_guest_auth_interceptor.gointernal/auth/grpc_guest_auth_interceptor_test.gointernal/auth/policies/authz.regointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/kubernetes/files/kubernetes_files.gointernal/network/cert_pool.gointernal/testing/tokens.goit/it_multitenancy_test.go
💤 Files with no reviewable changes (9)
- charts/service/templates/grpc-server/authconfig.yaml
- charts/service/templates/authorino/authorino.yaml
- internal/auth/grpc_guest_auth_interceptor_test.go
- internal/auth/grpc_guest_auth_interceptor.go
- internal/auth/auth_rules_test.go
- internal/auth/grpc_external_auth_interceptor.go
- internal/auth/grpc_external_auth_interceptor_metadata_test.go
- internal/auth/grpc_external_auth_interceptor_test.go
- charts/service/templates/authorino/certificate.yaml
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go (1)
1186-1189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
--ca-filehelp text to match the current auth architecture.Line 1188 still says the CA bundle is used for “external auth service”, but this command now performs in-process JWT/JWKS auth. This is operator-facing and currently misleading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go` around lines 1186 - 1189, Update the operator-facing help constant caFileFlagHelp to remove the incorrect reference to an “external auth service” and instead describe that the CA bundle is used for in-process JWT/JWKS verification (trusted CA certificates in PEM format for validating JWT issuer keys), so change the text in the caFileFlagHelp constant to clearly state it provides trusted CA certificates for in-process JWT/JWKS auth and TLS verification as applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/auth_jwks_cache.go`:
- Around line 549-555: The JWKS body is being read with io.ReadAll(reader)
without a size limit; change the read to use a bounded reader (e.g., wrap reader
with io.LimitReader using a defined constant like maxJWKSSizeBytes) and read
from that instead, returning a clear error if the payload exceeds the limit;
update the json.Unmarshal call to use the bounded data and keep the
jwksCacheKeySetData handling unchanged.
- Around line 341-352: The goroutine spawns a background JWKS refresh using
context.WithoutCancel (refreshCtx) which can block indefinitely; change this to
derive a cancellable context with a bounded timeout (e.g. ctxWithTimeout,
created via context.WithTimeout(ctx, <reasonableDuration>)) before launching the
goroutine, pass ctxWithTimeout into i.refresh(…) and into i.logger.ErrorContext,
and ensure you call cancel() (defer cancel()) inside the goroutine so the
outbound HTTP call in refresh and any resources are always bounded by
timeout/cancellation rather than detached.
- Around line 232-244: The code currently trusts the issuer string returned by
tokenObject.Claims.GetIssuer() (used to populate issuersInfo and later for
authenticated JWKS/network calls); validate and canonicalize that issuer before
storing or using it: parse the issuer with net/url, require an https scheme,
strip/deny any userinfo, and ensure the host (and optional port) matches an
allow-list of expected Kubernetes issuer hosts or a configured trusted issuer
pattern (or compare against the cluster-known issuer value) and reject anything
else with an error; only after these checks assign issuerUrl into
jwksCacheIssuerInfo and into issuersInfo so untrusted/tampered token values
cannot redirect authenticated discovery.
In `@internal/auth/policies/authz.rego`:
- Around line 136-154: is_client currently becomes true when admin roles are
false even if there is no authenticated identity; change the is_client rule to
require a present, non-empty authenticated identity before classifying as a
client (e.g. add checks that input.auth.identity is defined/non-empty and that
input.auth.authnMethod is defined/non-empty or not an anonymous/none value);
keep the existing role negations (not is_admin, not is_tenant_admin, not
is_tenant_idp_manager) but add the identity/authnMethod guards so unauthorized
requests without authentication cannot satisfy is_client (no changes needed to
has_client_permissions beyond inheriting the corrected is_client behavior).
---
Outside diff comments:
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 1186-1189: Update the operator-facing help constant caFileFlagHelp
to remove the incorrect reference to an “external auth service” and instead
describe that the CA bundle is used for in-process JWT/JWKS verification
(trusted CA certificates in PEM format for validating JWT issuer keys), so
change the text in the caFileFlagHelp constant to clearly state it provides
trusted CA certificates for in-process JWT/JWKS auth and TLS verification as
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: cf6f9c07-1bcc-468e-a042-799156d51887
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
charts/service/templates/authorino/authorino.yamlcharts/service/templates/authorino/certificate.yamlcharts/service/templates/grpc-server/authconfig.yamlgo.modinternal/auth/auth_context.gointernal/auth/auth_headers.gointernal/auth/auth_jwks_cache.gointernal/auth/auth_jwks_cache_mock.gointernal/auth/auth_jwks_cache_test.gointernal/auth/auth_jwt_validator.gointernal/auth/auth_jwt_validator_mock.gointernal/auth/auth_jwt_validator_test.gointernal/auth/auth_rules_test.gointernal/auth/auth_script_token_source_test.gointernal/auth/auth_static_token_source_test.gointernal/auth/grpc_authn_interceptor.gointernal/auth/grpc_authn_interceptor_test.gointernal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_metadata_test.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_metadata_test.gointernal/auth/grpc_external_auth_interceptor_test.gointernal/auth/grpc_guest_auth_interceptor.gointernal/auth/grpc_guest_auth_interceptor_test.gointernal/auth/policies/authz.regointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/kubernetes/files/kubernetes_files.gointernal/network/cert_pool.gointernal/testing/testing_server.gointernal/testing/tokens.goit/it_multitenancy_test.go
💤 Files with no reviewable changes (9)
- charts/service/templates/authorino/certificate.yaml
- internal/auth/grpc_external_auth_interceptor_metadata_test.go
- charts/service/templates/authorino/authorino.yaml
- internal/auth/grpc_guest_auth_interceptor.go
- internal/auth/grpc_external_auth_interceptor_test.go
- charts/service/templates/grpc-server/authconfig.yaml
- internal/auth/grpc_guest_auth_interceptor_test.go
- internal/auth/auth_rules_test.go
- internal/auth/grpc_external_auth_interceptor.go
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/testing/tokens.go (1)
65-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
life == 0does not actually produce a non-expiring token (Severity: Minor, auth-test reliability impact).Line 69 only skips writing
exp, butMakeTokenObjectpreloads default claims that already includeexp, so zero-life tokens still expire quickly. That contradicts the function contract and can silently invalidate expiry-sensitive tests.Suggested fix
func MakeTokenString(iss, typ string, life time.Duration) string { claims := jwt.MapClaims{} claims["iss"] = iss claims["typ"] = typ - if life != 0 { + if life == 0 { + claims["exp"] = nil + } else { claims["exp"] = time.Now().Add(life).Unix() } token := MakeTokenObject(claims) return token.Raw }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testing/tokens.go` around lines 65 - 71, MakeTokenString is intended to create a non-expiring token when life == 0, but MakeTokenObject preloads default claims (including exp) so skipping setting claims["exp"] isn't enough; update MakeTokenString to explicitly remove any preloaded "exp" claim when life == 0 (or alternatively call MakeTokenObject without defaults and only set claims you want). Concretely: in MakeTokenString, after obtaining the base claims from MakeTokenObject (or where claims are combined), if life == 0 delete claims["exp"] so no expiry claim is present; otherwise set claims["exp"] = time.Now().Add(life).Unix(). This ensures the zero-life code path truly produces a non-expiring token.
♻️ Duplicate comments (3)
internal/auth/auth_jwks_cache.go (3)
549-555:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor availability risk: unbounded JWKS body read allows memory exhaustion.
io.ReadAll(reader)on remote data has no size cap. A malicious or buggy endpoint can force excessive memory usage.Suggested fix
- jsonData, err := io.ReadAll(reader) + const maxJWKSBytes = 1 << 20 // 1 MiB + limited := io.LimitReader(reader, maxJWKSBytes+1) + jsonData, err := io.ReadAll(limited) if err != nil { return } + if len(jsonData) > maxJWKSBytes { + err = fmt.Errorf("JSON web key set is too large") + return + } var setData jwksCacheKeySetData err = json.Unmarshal(jsonData, &setData)As per coding guidelines, external input sizes should be defensively bounded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/auth_jwks_cache.go` around lines 549 - 555, The code currently uses io.ReadAll(reader) when reading JWKS bodies (producing jsonData) which can lead to unbounded memory use; change this to read with an explicit cap (for example wrap reader with io.LimitReader or use io.CopyN) and define a sane constant like maxJWKSBodySize (e.g. a few MB) to enforce the limit, then handle the case where the body exceeds the limit by returning an error; keep the subsequent json.Unmarshal into jwksCacheKeySetData unchanged but ensure you propagate/return the read/limit error instead of silently returning on failure.Source: Coding guidelines
341-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor availability risk: detached background refresh has no timeout and can leave stuck goroutines.
The refresh goroutine uses
context.WithoutCancel(ctx)and can block indefinitely on slow/unresponsive endpoints.Suggested fix
- refreshCtx := context.WithoutCancel(ctx) + refreshCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second) go func() { + defer cancel() err := i.refresh(refreshCtx, issuerInfo) if err != nil { i.logger.ErrorContext(As per coding guidelines, external network work should be bounded with cancellation/timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/auth_jwks_cache.go` around lines 341 - 352, The background refresh goroutine currently uses context.WithoutCancel(ctx) and can block indefinitely; change it to derive a bounded context (e.g., context.WithTimeout or a cancellable context with a configured timeout) and pass that to i.refresh so external network calls are time-limited; ensure you call the cancel function (defer cancel()) inside the goroutine so the context is released, and keep the same logging via i.logger.ErrorContext (use the bounded ctx when logging) while preserving issuerInfo and issuerUrl usage.Source: Coding guidelines
232-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMajor security risk: unvalidated Kubernetes
issis promoted to trusted issuer and used for authenticated outbound calls.
issis extracted from an unverified token and directly trusted. If that file is tampered, discovery/JWKS requests (with bearer token) can be redirected to an attacker-controlled endpoint.Suggested fix
var issuerUrl string issuerUrl, err = tokenObject.Claims.GetIssuer() if err != nil { err = fmt.Errorf( "failed to get the 'iss' claim from Kubernetes service account token: %w", err, ) return } + issuerUrl = strings.TrimSpace(issuerUrl) + parsedIssuer, parseErr := url.Parse(issuerUrl) + if parseErr != nil { + err = fmt.Errorf("invalid Kubernetes issuer URL '%s': %w", issuerUrl, parseErr) + return + } + if parsedIssuer.Scheme != "https" || parsedIssuer.Host == "" || parsedIssuer.User != nil { + err = fmt.Errorf("invalid Kubernetes issuer URL '%s'", issuerUrl) + return + } + issuerUrl = parsedIssuer.String() issuersInfo[issuerUrl] = &jwksCacheIssuerInfo{ issuerUrl: issuerUrl, tokenFile: tokenFile, }As per coding guidelines, inputs at trust boundaries must be validated with allow-list style checks before privileged use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/auth_jwks_cache.go` around lines 232 - 244, The code currently promotes an unverified issuer string from tokenObject.Claims.GetIssuer() into issuersInfo (stored in jwksCacheIssuerInfo) and uses it for outbound discovery/JWKS calls; instead, validate the issuer before trusting it—either verify the token signature and claims using the cluster's known Kubernetes CA/issuer or check the extracted issuerUrl against a configured allow-list/expected prefix/regex of permitted issuers (e.g., the kube API server issuer hostnames or an explicit config list) and reject or ignore tokens whose issuerUrl fails validation; update the logic around tokenObject.Claims.GetIssuer(), the population of issuersInfo[issuerUrl], and any code that reads jwksCacheIssuerInfo.tokenFile to only proceed after issuerUrl passes this allow-list/signature validation, returning a clear error when validation fails.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/auth_jwt_validator_test.go`:
- Around line 352-417: The test enforces a brittle 10x median-speedup gate which
causes CI flakiness; modify the assertion in the It("Is at least an order of
magnitude faster with caching") case so the required speedup is configurable and
far less strict by default: introduce a lookup (e.g. from env var
JWT_CACHE_EXPECTED_SPEEDUP parsed as float, defaulting to 2.0) and replace the
hard BeNumerically("<", statsWithout.DurationFor(StatMedian)/10) check with
BeNumerically("<", statsWithout.DurationFor(StatMedian)/expectedSpeedup). Update
the Expect line that references statsWith.DurationFor(StatMedian) accordingly
and keep all other experiment setup (NewJwtValidator, SetCacheEnabled,
RankStats, experimentWith/experimentWithout) unchanged.
In `@internal/auth/auth_jwt_validator.go`:
- Around line 138-147: JwtValidatorBuilder.Build() currently omits audience
validation, allowing tokens for other audiences to be accepted; add an
expected-audience check by extending the JwtValidatorBuilder (e.g., add a field
like expectedAudience or audiences), pass that into Build, and include
jwt.WithAudience(expectedAudience) (or jwt.WithAudience(...) for multiple
audiences) in parserOptions; also update validateClaims() to explicitly verify
the "aud" claim matches the expected audience(s) (in addition to the existing
typ/sub checks) and make the gRPC auth interceptor only store the token in
context after validateClaims confirms the audience, so tokens issued for other
audiences are rejected before reaching authorization logic.
In `@internal/auth/grpc_authn_interceptor_test.go`:
- Around line 78-371: Tests only cover UnaryServer; add equivalent StreamServer
tests to exercise the same authentication behaviors: create new It blocks that
mirror the unary cases using NewGrpcAuthnInterceptor(), the same helper
functions (MakeTokenString, MakeTokenObject, TokenFromContext) and the mock
JwtValidator to assert StreamServer rejects missing Authorization on private
methods, rejects bad auth scheme, returns validator errors as gRPC status,
rejects multiple authorization headers, allows anonymous methods when
configured, and stores validated token in the stream context on success; call
the interceptor.StreamServer method with appropriate grpc.StreamServerInfo
(FullMethod set to "/my_package/MyMethod" or other patterns) and a small fake
stream handler that inspects ctx for TokenFromContext and sets a handled flag,
using metadata.NewIncomingContext to inject headers exactly as done in the unary
tests so behavior is consistent.
In `@internal/auth/grpc_authz_interceptor.go`:
- Around line 193-205: The stream interceptor path isn’t covered: add a test
that invokes GrpcAuthzInterceptor.StreamServer (using a minimal mock
grpc.ServerStream) to exercise authorize, then call grpcAuthzStream.Context()
and assert the returned context contains the authorized Subject (the same
context authorize returned); if the test fails, fix grpcAuthzStream.Context() to
return the wrapped context set in StreamServer (ensure grpcAuthzStream has a
field like context and its Context() method returns that field rather than
delegating to the underlying stream). Ensure the test uses the interceptor's
authorize behavior and the StreamServer/handler flow so regressions in streaming
context propagation are caught.
In `@internal/auth/policies/authz.rego`:
- Around line 77-95: The current rule sets subject_tenants to subject_groups for
JWT users lacking organization/organizations claims, which lets JWTs without
explicit tenant claims inherit tenant-management scope; update the logic in the
subject_tenants rule(s) (the clauses using input.auth.identity.authnMethod ==
"jwt", input.auth.identity.organization, input.auth.identity.organizations, and
subject_groups) so that for JWTs you do NOT fall back to subject_groups when the
token might be used for tenant-management roles (e.g., tenant-admin or
tenant-idp-manager); specifically, require explicit organization/organizations
claims to populate subject_tenants for JWT users and make the fallback to
subject_groups either removed for JWT authnMethod or conditioned to only apply
when the evaluated role is not a tenant-management role. Ensure this change is
applied to the duplicate block(s) handling JWT fallback so both occurrences are
consistent.
In `@internal/cmd/service/start/grpcserver/start_grpc_server_cmd.go`:
- Around line 177-183: Update the help text for the --ca-file flag
(variable/name: caFileFlagHelp) to reflect the new JWKS/OIDC trust usage instead
of the removed external-auth service: describe that the provided CA files are
used to establish trust for issuer discovery and JWKS fetching (OIDC/JWKS
verification) by the auth.NewJwksCache path and related TLS calls, and remove
references to the external-auth service; ensure the wording clearly states these
CA files are appended to the system/kubernetes pools and used for TLS when
retrieving OIDC issuer metadata and JWKS.
---
Outside diff comments:
In `@internal/testing/tokens.go`:
- Around line 65-71: MakeTokenString is intended to create a non-expiring token
when life == 0, but MakeTokenObject preloads default claims (including exp) so
skipping setting claims["exp"] isn't enough; update MakeTokenString to
explicitly remove any preloaded "exp" claim when life == 0 (or alternatively
call MakeTokenObject without defaults and only set claims you want). Concretely:
in MakeTokenString, after obtaining the base claims from MakeTokenObject (or
where claims are combined), if life == 0 delete claims["exp"] so no expiry claim
is present; otherwise set claims["exp"] = time.Now().Add(life).Unix(). This
ensures the zero-life code path truly produces a non-expiring token.
---
Duplicate comments:
In `@internal/auth/auth_jwks_cache.go`:
- Around line 549-555: The code currently uses io.ReadAll(reader) when reading
JWKS bodies (producing jsonData) which can lead to unbounded memory use; change
this to read with an explicit cap (for example wrap reader with io.LimitReader
or use io.CopyN) and define a sane constant like maxJWKSBodySize (e.g. a few MB)
to enforce the limit, then handle the case where the body exceeds the limit by
returning an error; keep the subsequent json.Unmarshal into jwksCacheKeySetData
unchanged but ensure you propagate/return the read/limit error instead of
silently returning on failure.
- Around line 341-352: The background refresh goroutine currently uses
context.WithoutCancel(ctx) and can block indefinitely; change it to derive a
bounded context (e.g., context.WithTimeout or a cancellable context with a
configured timeout) and pass that to i.refresh so external network calls are
time-limited; ensure you call the cancel function (defer cancel()) inside the
goroutine so the context is released, and keep the same logging via
i.logger.ErrorContext (use the bounded ctx when logging) while preserving
issuerInfo and issuerUrl usage.
- Around line 232-244: The code currently promotes an unverified issuer string
from tokenObject.Claims.GetIssuer() into issuersInfo (stored in
jwksCacheIssuerInfo) and uses it for outbound discovery/JWKS calls; instead,
validate the issuer before trusting it—either verify the token signature and
claims using the cluster's known Kubernetes CA/issuer or check the extracted
issuerUrl against a configured allow-list/expected prefix/regex of permitted
issuers (e.g., the kube API server issuer hostnames or an explicit config list)
and reject or ignore tokens whose issuerUrl fails validation; update the logic
around tokenObject.Claims.GetIssuer(), the population of issuersInfo[issuerUrl],
and any code that reads jwksCacheIssuerInfo.tokenFile to only proceed after
issuerUrl passes this allow-list/signature validation, returning a clear error
when validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f725d1bb-db42-4396-aef8-33d5d782abc6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
charts/service/templates/authorino/authorino.yamlcharts/service/templates/authorino/certificate.yamlcharts/service/templates/grpc-server/authconfig.yamlgo.modinternal/auth/auth_context.gointernal/auth/auth_headers.gointernal/auth/auth_jwks_cache.gointernal/auth/auth_jwks_cache_mock.gointernal/auth/auth_jwks_cache_test.gointernal/auth/auth_jwt_validator.gointernal/auth/auth_jwt_validator_mock.gointernal/auth/auth_jwt_validator_test.gointernal/auth/auth_rules_test.gointernal/auth/auth_script_token_source_test.gointernal/auth/auth_static_token_source_test.gointernal/auth/grpc_authn_interceptor.gointernal/auth/grpc_authn_interceptor_test.gointernal/auth/grpc_authz_interceptor.gointernal/auth/grpc_authz_interceptor_metadata_test.gointernal/auth/grpc_authz_interceptor_test.gointernal/auth/grpc_external_auth_interceptor.gointernal/auth/grpc_external_auth_interceptor_metadata_test.gointernal/auth/grpc_external_auth_interceptor_test.gointernal/auth/grpc_guest_auth_interceptor.gointernal/auth/grpc_guest_auth_interceptor_test.gointernal/auth/policies/authz.regointernal/cmd/service/probe/grpcserver/probe_grpc_server_cmd.gointernal/cmd/service/start/consoleproxy/start_console_proxy_cmd.gointernal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/kubernetes/files/kubernetes_files.gointernal/network/cert_pool.gointernal/testing/testing_server.gointernal/testing/tokens.goit/it_multitenancy_test.go
💤 Files with no reviewable changes (9)
- internal/auth/grpc_guest_auth_interceptor.go
- internal/auth/auth_rules_test.go
- charts/service/templates/grpc-server/authconfig.yaml
- charts/service/templates/authorino/certificate.yaml
- internal/auth/grpc_external_auth_interceptor_metadata_test.go
- internal/auth/grpc_external_auth_interceptor.go
- internal/auth/grpc_guest_auth_interceptor_test.go
- internal/auth/grpc_external_auth_interceptor_test.go
- charts/service/templates/authorino/authorino.yaml
tzvatot
suggested changes
Jun 14, 2026
| "/osac.public.v1.Clusters/List", | ||
| "/osac.public.v1.Clusters/Update", | ||
| "/osac.public.v1.ClusterCatalogItems/Get", | ||
| "/osac.public.v1.ClusterCatalogItems/List", |
Contributor
There was a problem hiding this comment.
🟡 ComputeInstanceCatalogItems missing from client permissions
The policy includes ClusterCatalogItems/Get and List for clients but ComputeInstanceCatalogItems/Get and List are missing. Clients won't be able to browse VM catalog items.
This was also missing from the old Helm authconfig, so it's a pre-existing gap - but since you're rewriting the policy from scratch, this is the right time to fix it. Add:
"/osac.public.v1.ComputeInstanceCatalogItems/Get",
"/osac.public.v1.ComputeInstanceCatalogItems/List",
| jwtValidator, err := auth.NewJwtValidator(). | ||
| SetLogger(c.logger). | ||
| SetJwksCache(jwksCache). | ||
| SetExpirationLeeway(10 * time.Second). |
Contributor
There was a problem hiding this comment.
💡 10s expiration leeway is generous
Industry standard is typically 0-5s. With 10s, an expired token can still authenticate for 10 seconds after expiry. Not a security hole, but worth considering tightening to 5s unless there's a specific clock skew concern.
jhernand
changed the title
|
@jhernand: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
CrystalChun
reviewed
Jun 15, 2026
| result, err = i.withoutHeader(ctx, method) | ||
| case 1: | ||
| header := values[0] | ||
| result, err = i.withHeader(ctx, header) |
Contributor
There was a problem hiding this comment.
[Can be addressed in a later patch]
Should method always be taken into consideration? For example, what if a user uses an expired token, but is calling an anonymous method such as Capabilities?
Contributor
Author
There was a problem hiding this comment.
What are you suggesting? That a user with a broken/expired/invalid token should be able to call the anonymous methods? We can certainly do that.
Contributor
There was a problem hiding this comment.
Yes that's what I mean. Do you think it should be allowed? I don't imagine it's likely to happen often so I'm ok if we don't allow it and leave this as is
Contributor
Author
There was a problem hiding this comment.
Actually it used to be like that, I will restore it.
CrystalChun
reviewed
Jun 15, 2026
Comment on lines
+646
to
+648
| result = &rsa.PublicKey{ | ||
| N: new(big.Int).SetBytes(nb), | ||
| E: int(new(big.Int).SetBytes(eb).Int64()), |
Contributor
There was a problem hiding this comment.
[Can be addressed in a later patch]
Low risk of happening, but potential overflow can cause malformed entries
Suggested change
| result = &rsa.PublicKey{ | |
| N: new(big.Int).SetBytes(nb), | |
| E: int(new(big.Int).SetBytes(eb).Int64()), | |
| eBig := new(big.Int).SetBytes(eb) | |
| if !eBig.IsInt64() { | |
| err = errors.New("RSA exponent is too large") | |
| return | |
| } | |
| e := eBig.Int64() | |
| if e < 3 || e&1 == 0 { | |
| err = errors.New("RSA exponent is invalid") | |
| return | |
| } | |
| result = &rsa.PublicKey{N: new(big.Int).SetBytes(nb), E: int(e)} |
Contributor
Author
There was a problem hiding this comment.
Makes sense, will do.
Contributor
|
/approve |
Contributor
Author
|
/retest |
Contributor
Author
|
@CrystalChun would be nice if you can re-approve this. |
Contributor
Author
|
/retest |
Contributor
|
/lgtm |
…authorization The main motivation for this change is to stop using Authorino for authentication and authorization. Instead the service now validates JWT tokens directly using a JWKS authentication interceptor and evaluates the authorization policy using the OPA Go library. The Rego policy that was previously defined inside the Authorino `AuthConfig` custom resource is now embedded in the binary with `//go:embed`. This prevents users from copying and modifying the policies, and also simplifies the deployment by removing the dependency on Authorino, its operator, and the corresponding `cert-manager` certificates. The new `GrpcAuthnInterceptor` discovers JWKS endpoints via OpenID Connect discovery for each trusted issuer and validates bearer tokens extracted from the `authorization` header. It supports both Keycloak JWT tokens and Kubernetes service account tokens. The validated token is stored in the context for the downstream authorization interceptor. The new `GrpcAuthzInterceptor` constructs an input document compatible with the structure that Authorino used and evaluates the embedded Rego policy using a `rego.PreparedEvalQuery`. On success it builds a `Subject` containing the user and tenant list and stores it in the context, replacing the role previously played by the `X-Subject` response header that Authorino produced. The `--grpc-authn-type` and `--grpc-authn-external-address` flags are deprecated and ignored. The guest auth interceptor and the external auth interceptor are removed together with their tests. The Helm templates for Authorino, its TLS certificate, and the `AuthConfig` are also removed. On the dependency side, `envoyproxy/go-control-plane` and its transitive dependencies are dropped from `go.mod`. The Kustomize manifests are intentionally left unchanged to avoid breaking the CI tests that still use them. In that environment Authorino will continue to run but will not be used by the service. It will be removed in a future change. Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit Jun 22, 2026
195495f
into
osac-project:main
21 of 22 checks passed
3 tasks
|
New changes are detected. LGTM label has been removed. |
|
@jhernand: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This was referenced Jun 25, 2026
Merged
4 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
Summary
interceptors that validate JWT tokens via JWKS discovery and evaluate an embedded Rego
policy using the OPA Go library.
//go:embedso that users cannot copyand modify it, and simplify deployment by removing the dependency on Authorino, its
operator, and the corresponding
cert-managercertificates.Authorino. Deprecate the
--grpc-authn-typeand--grpc-authn-external-addressflags.Drop the
envoyproxy/go-control-planedependency.The Kustomize manifests are intentionally left unchanged to avoid breaking the CI
integration tests. Authorino will still run in that environment but will not be used by
the service. It will be removed in a future change.
Test plan
ginkgo run -r internal)ginkgo run it)Summary by CodeRabbit
New Features
Removed
Improved