Repository navigation
Conversation
The OAuth2 client was built at package init with KESSEL_AUTH_OIDC_ISSUER passed as the token endpoint. The shared env sets that to the SSO realm URL, so token fetches could never succeed; it went unnoticed because KESSEL_INSECURE=true drops call credentials. Build one credentials object lazily, resolve the token endpoint through OIDC discovery, and fail with a clear error when issuer, client id or secret is missing. Failures are not cached so a transient discovery error is retried. The object is exposed via getBearerToken so HTTP callers behind the east-west gateway can share it with the gRPC client.
Clowder writes dependencyEndpoints.v2 on every cluster, so read RBAC only from v2 rbac/service and drop the v1 fallback. The v2 entry also says whether the endpoint sits behind the east-west gateway (authenticated) and which CA to trust. When authenticated, send an SSO service-account bearer token alongside x-rh-identity; RBAC still evaluates the identity, the gateway only checks the bearer. If no token can be obtained the manager returns 502 without calling RBAC, so it never makes an unauthenticated call to an authenticated endpoint. A provided ca_certificate is added to the system trust store. The RBAC address is now validated when the middleware is built, so a missing dependency fails at startup instead of on the first request. RBAC_ADDRESS still overrides the address for local runs.
Reviewer's GuideData structuresAdd RBAC authentication and CA-certificate fields to the shared core configuration used by the manager.
ImplementationBuild shared Kessel and RBAC OAuth2 credentials lazily through OIDC discovery, bound discovery and token requests, and pass the request context through Kessel client setup.
Attach a gateway bearer token to authenticated RBAC requests, trust the configured CA, and fail closed if token acquisition fails.
Redact Authorization during debug request dumps while retaining the real header on the outgoing request.
ConfigurationResolve RBAC only from Clowder v2 and discard its authentication metadata when RBAC_ADDRESS overrides the Clowder endpoint.
Update the application configuration to support the Clowder v2 RBAC endpoint and gateway authentication setup.
TestsTest Clowder v2 RBAC resolution, environment override handling, and preservation or removal of endpoint authentication metadata.
Verify that debug HTTP dumps redact Authorization without changing the header sent to the server.
Test Kessel client setup with shared discovered credentials, including missing issuer handling and authenticated setup.
Test authenticated and unauthenticated RBAC calls, fail-closed token errors, shared credentials, and custom CA trust.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="manager/middlewares/rbac.go" line_range="52" />
<code_context>
- client := api.Client{
+ headers := map[string]string{xRHIdentity: identity}
+ if bearer != "" {
+ headers["Authorization"] = "Bearer " + bearer
+ }
+ return &api.Client{
</code_context>
<issue_to_address>
**Bearer sent to overridden RBAC host**
When clowder marks RBAC authenticated and `RBAC_ADDRESS` points to an untrusted or plaintext destination, `initServicesFromEnv` can replace the Clowder address without changing `RbacAuthenticated`, so the middleware sends the service-account bearer to the override; an HTTP or untrusted `RBAC_ADDRESS` exposes the token to that host.
Bind authentication metadata to the selected endpoint and reject authenticated overrides that are not trusted HTTPS destinations.
</issue_to_address>
### Comment 2
<location path="manager/middlewares/rbac.go" line_range="52" />
<code_context>
- client := api.Client{
+ headers := map[string]string{xRHIdentity: identity}
+ if bearer != "" {
+ headers["Authorization"] = "Bearer " + bearer
+ }
+ return &api.Client{
</code_context>
<issue_to_address>
**Bearer tokens enter trace logs**
When trace logging is enabled and an authenticated RBAC request is made, `makeClient` enables request debugging at trace level, and `utils.CallAPI` dumps the outgoing request including headers without redaction, so every authenticated RBAC call writes the gateway bearer token to application logs.
Redact the Authorization header from request dumps, or disable request dumping for authenticated RBAC calls.
</issue_to_address>
### Comment 3
<location path="manager/middlewares/kessel.go" line_range="39" />
<code_context>
+ if cfg.KesselAuthOIDCIssuer == "" || cfg.KesselAuthClientID == "" || cfg.KesselAuthClientSecret == "" {
+ return nil, errors.New("KESSEL_AUTH_OIDC_ISSUER, KESSEL_AUTH_CLIENT_ID and KESSEL_AUTH_CLIENT_SECRET must be set")
+ }
+ discovery, err := auth.FetchOIDCDiscovery(ctx, cfg.KesselAuthOIDCIssuer, auth.FetchOIDCDiscoveryOptions{})
+ if err != nil {
+ return nil, errors.Wrap(err, "OIDC discovery failed")
</code_context>
<issue_to_address>
**Hung discovery stalls authorization**
When the OIDC discovery endpoint accepts the request but does not respond, and the incoming request has no deadline, `getCredentials` waits for `auth.FetchOIDCDiscovery` before reaching token retrieval; with no application-level deadline, a stalled OIDC endpoint leaves the RBAC-checked request waiting until its client disconnects, tying up a request handler.
Apply a bounded timeout to OIDC discovery and token acquisition so a stalled identity provider fails promptly.
</issue_to_address>
### Comment 4
<location path="manager/middlewares/rbac_auth_test.go" line_range="87-90" />
<code_context>
+ _, err := getBearerToken(t.Context())
+ require.NoError(t, err)
+ first := credentials
+ _, conn, err := setupClient(t.Context())
+ require.NoError(t, err)
+ defer conn.Close()
+ assert.Same(t, first, credentials)
+}
+
</code_context>
<issue_to_address>
**Kessel auth is not verified**
When `setupClient` stops passing its credentials to `OAuth2ClientAuthenticated` but still builds a client, when `setupClient` creates a client without attaching OAuth2 call credentials, `TestRBACAndKesselShareCredentials` still passes because `assert.Same` checks only the shared global pointer, and `TestSetupClientSecureAuth` checks only that the client is non-nil. The tests pass while secure Kessel calls are unauthenticated.
Exercise the client against a gRPC test server that requires and verifies the bearer token.
Also at `manager/middlewares/kessel_test.go:168-172`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and when Clowder marks RBAC as authenticated, the middleware now sends an SSO service-account bearer token to the configured RBAC URL; if that destination or its authentication configuration is wrong, the token may already have been disclosed, and reverting cannot undo that exposure. Token-fetch failures also fail closed and deny requests until corrected.
Blocking findings: manager/middlewares/rbac.go:52, manager/middlewares/rbac.go:52, manager/middlewares/kessel.go:39, manager/middlewares/rbac_auth_test.go:90
At trace level CallAPI dumps outgoing requests including headers, so authenticated RBAC calls would write the gateway bearer token to logs. The header is masked in place only for the dump; the request still carries the real token.
The request context passed in is a gin.Context, which is never cancelled, and discovery runs while holding credentialsMu. A stalled SSO endpoint would therefore hang every RBAC-checked request, not just one. Bound each discovery/token HTTP call to 10s.
authenticated and ca_certificate describe the Clowder v2 endpoint. If RBAC_ADDRESS points elsewhere, keeping them would send the service account bearer token to a host Clowder never vouched for. An override equal to the v2 address keeps the v2 settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2389 +/- ##
==========================================
+ Coverage 66.82% 67.09% +0.26%
==========================================
Files 157 157
Lines 9080 9133 +53
==========================================
+ Hits 6068 6128 +60
+ Misses 3010 3003 -7
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Moves the manager's RBAC lookup to Clowder v2 and adds east-west gateway bearer auth for when RBAC is reached through a ClowdAppRef. Only RBAC is migrated; vmaas, content-sources and patchman's own private endpoints are unchanged.
Kessel credentials (first commit): one lazily built
OAuth2ClientCredentials, token endpoint from OIDC discovery. PreviouslyKESSEL_AUTH_OIDC_ISSUERwas passed in as the token endpoint, which can't work with the realm URL; hidden so far becauseKESSEL_INSECURE=truedrops call credentials.rbacendpoint and noRBAC_ADDRESSauthenticated: false(today: same cluster as RBAC)authenticated: true, Kessel creds setx-rh-identityauthenticated: true, creds missingEvidence
After: new/updated tests pass locally:
Full DB-backed suite left to CI.
Merge Danger
Door: two-way
Revert restores the v1 RBAC fallback; no schema or data changes.
Blast Radius: manager
rbac/servicenorRBAC_ADDRESSis present.rbacis a required dependency, so Clowder should always write it.KESSEL_INSECURE=falseand auth enabled now performs OIDC discovery againstKESSEL_AUTH_OIDC_ISSUER. No change whereKESSEL_INSECURE=true.authenticated: true(ClowdAppRef); patchman isn't served by one today. Enabling it later needs patchman's SSO service account added to the RBAC gateway AuthorizationPolicy in app-interface.Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Migrate RBAC resolution to Clowder v2 and support authenticated gateway access while preserving secure failure behavior and existing non-RBAC service resolution.
New Features:
Bug Fixes:
Enhancements:
Tests: