feat: introspection authentication #65
Conversation
WalkthroughRemoved the external RoundTripper factory from cluster initialization, refactored the roundtripper to accept a base transport and clone requests, moved introspection-authentication into Config.Gateway, added TargetCluster.ValidateToken, changed a finalizers signature, removed CLI default init, and updated many module dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| if !rt.appCfg.Gateway.ShouldImpersonate { | ||
| rt.log.Debug().Str("path", req.URL.Path).Msg("Using bearer token authentication") | ||
|
|
||
| return rt.adminRT.RoundTrip(req) |
There was a problem hiding this comment.
We used adminRT here and it worked with any token passed since adminRT has certificates to access cluster.
There was a problem hiding this comment.
I also tried to replace adminRT with BearerRT below in the impersonation section, but I got the following error:
"level":"error","service":"crdGateway","operation":"get","group":"core.platform-mesh.io","version":"v1alpha1","kind":"AccountInfo","error":"users \"[email protected]\" is forbidden: User \"[email protected]\" cannot impersonate resource \"users\" in API group \"\" at the cluster scope: access denied\nNoOpinion"
I guess we should setup fga first, let me know if this we should replace adminRT in the impersonation branch as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/manager/roundtripper/roundtripper.go (1)
121-144: Verify discovery request path handling is exhaustive.The updated
isDiscoveryRequestlogic handles path prefixes (/services/.../clusters/...and/clusters/...) before checking discovery endpoints. The logic looks correct, but consider the following edge cases:
- Paths with trailing slashes are trimmed (line 121), which is good
- Empty paths return false (lines 122-124), which is correct
- Prefix stripping logic (lines 127-131) correctly removes 5 parts for services prefix and 3 parts for clusters prefix
However, ensure that:
- Virtual workspace paths (
/services/{service}/clusters/{workspace}/api) are correctly handled- KCP workspace paths (
/clusters/{workspace}/api) are correctly handled- All discovery endpoints (
/api,/apis,/api/v1,/apis/group,/apis/group/v1) are recognizedConsider adding test cases for virtual workspace and KCP workspace discovery paths to ensure the prefix stripping logic works correctly:
// Test cases to add to roundtripper_test.go { name: "virtual_workspace_with_trailing_components", path: "/services/myservice/clusters/workspace1/api/v1", isDiscovery: true, }, { name: "kcp_workspace_with_version", path: "/clusters/workspace1/api/v1", isDiscovery: true, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
gateway/manager/manager.go(1 hunks)gateway/manager/roundtripper/roundtripper.go(4 hunks)gateway/manager/roundtripper/roundtripper_test.go(8 hunks)gateway/manager/targetcluster/cluster.go(5 hunks)gateway/manager/targetcluster/registry.go(2 hunks)gateway/manager/targetcluster/registry_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
gateway/manager/roundtripper/roundtripper_test.go (1)
gateway/manager/roundtripper/roundtripper.go (1)
New(26-34)
gateway/manager/manager.go (1)
gateway/manager/targetcluster/registry.go (1)
NewClusterRegistry(34-43)
gateway/manager/targetcluster/cluster.go (2)
common/config/config.go (1)
Config(3-36)gateway/manager/roundtripper/roundtripper.go (2)
New(26-34)NewUnauthorizedRoundTripper(37-39)
gateway/manager/roundtripper/roundtripper.go (1)
common/config/config.go (1)
Config(3-36)
gateway/manager/targetcluster/registry.go (1)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(52-60)NewTargetCluster(63-96)
gateway/manager/targetcluster/registry_test.go (1)
gateway/manager/targetcluster/registry.go (1)
NewClusterRegistry(34-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
gateway/manager/targetcluster/registry_test.go (1)
21-21: LGTM!The test correctly uses the updated
NewClusterRegistrysignature, removing the thirdnilargument that previously represented theRoundTripperFactory.gateway/manager/manager.go (1)
25-25: LGTM!The
NewClusterRegistrycall correctly uses the simplified signature without theRoundTripperFactoryparameter.gateway/manager/roundtripper/roundtripper_test.go (3)
80-80: LGTM!All test cases correctly updated to use the new
roundtripper.Newsignature with the additionalbaseRoundTripperparameter. PassingmockAdminas bothadminRoundTripperandbaseRoundTripperis appropriate for these test scenarios.Also applies to: 265-265, 379-379, 454-454, 503-503, 534-534
514-514: LGTM!Skipping this test is appropriate since it would require mocking
baseRT, which is now an internal implementation detail.
557-563: Add assertion verifying admin Authorization header
The test removes the original Authorization header and setsImpersonate-Userbut doesn’t confirm the adminRT token is applied. Add an assertion, for example:assert.Equal(t, "Bearer <admin-token>", capturedRequest.Header.Get("Authorization"))or update the test documentation to clarify the expected authentication flow.
gateway/manager/targetcluster/registry.go (3)
27-31: LGTM!The
ClusterRegistrystruct correctly removes theroundTripperFactoryfield, aligning with the refactor that moves RoundTripper wiring into the cluster connection logic.
34-42: LGTM!The
NewClusterRegistrysignature and initialization correctly simplified by removing theRoundTripperFactoryparameter.
59-59: LGTM!The
NewTargetClustercall correctly updated to remove theroundTripperFactoryargument, consistent with the new cluster creation flow.gateway/manager/targetcluster/cluster.go (4)
18-18: LGTM!The import of the
roundtripperpackage is necessary for the new internal RoundTripper construction logic.
63-68: LGTM!The function signatures correctly updated to remove the
roundTripperFactoryparameter, aligning with the new approach where RoundTripper construction is handled internally.Also applies to: 81-81
117-126: LGTM!The new
Wrapimplementation correctly constructs the composite RoundTripper:
- Derives the base transport via
unwrapToBaseTransport- Constructs the custom RoundTripper with
adminRT,baseRT, andunauthorizedRTThis centralizes RoundTripper construction and eliminates the need for an external factory.
172-185: Verify client-go transport wrappers implement WrappedRoundTripper
Ensure every RoundTripper decorator in your client-go version (e.g. BearerAuthRoundTripper, ImpersonatingRoundTripper) defines func WrappedRoundTripper() http.RoundTripper so unwrapToBaseTransport can recurse through the chain. For vendored code, you can run:grep -R "func .*WrappedRoundTripper" -n vendor/k8s.io/client-go/transportgateway/manager/roundtripper/roundtripper.go (4)
9-9: LGTM!The import of
utilnetand addition of thebaseRTfield are necessary for the new request cloning and bearer authentication flow.Also applies to: 20-20
26-33: LGTM!The
Newconstructor correctly updated to accept and store thebaseRoundTripperparameter, enabling the bearer token authentication path to use the unwrapped base transport.
70-71: LGTM!Cloning the request before modifying headers is essential to prevent unintended side effects on the original request. This is a good security practice.
73-76: Verify non-impersonation auth behavior
Non-impersonation now wrapsbaseRTwith the user’s bearer token instead of using admin credentials. Confirm this matches the intended authentication model and add or update documentation/config comments accordingly.
…orm-mesh/kubernetes-graphql-gateway into config/IntrospectionAuthentication
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gateway/manager/roundtripper/roundtripper_test.go (1)
80-80: Add a dedicated baseRoundTripper mock.Now that
Newtakes a real base transport, these scenarios should assert that the non-impersonation path hits the base RT instead of the admin RT. PassingmockAdminin both slots masks that distinction, so a regression in wiring would still go green. Please introduce a separatemockBase(with its own expectations) or reuse an existing mock configured for the bearer path so the tests fail if the wrong transport is used.Also applies to: 265-265, 379-379, 454-454, 503-503, 530-530
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/root.go(1 hunks)common/config/config.go(2 hunks)common/config/config_test.go(4 hunks)gateway/manager/roundtripper/roundtripper.go(3 hunks)gateway/manager/roundtripper/roundtripper_test.go(7 hunks)gateway/manager/targetcluster/registry.go(3 hunks)tests/gateway_test/suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/manager/roundtripper/roundtripper.go
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/manager/targetcluster/registry.go (3)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(52-60)NewTargetCluster(63-96)common/config/config.go (1)
Config(3-36)gateway/manager/targetcluster/graphql.go (1)
IsIntrospectionQuery(81-97)
gateway/manager/roundtripper/roundtripper_test.go (1)
gateway/manager/roundtripper/roundtripper.go (1)
New(26-34)
common/config/config_test.go (1)
gateway/schema/schema.go (1)
Gateway(24-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
cmd/root.go (1)
69-69: Default relocation looks good.
gateway-introspection-authenticationaligns with the new nested config field.common/config/config_test.go (1)
16-16: Tests now target the nested introspection flag correctly.Initialization, assignment, and assertions all reference
cfg.Gateway.IntrospectionAuthentication, matching the struct changes.Also applies to: 40-42, 54-54, 68-68
tests/gateway_test/suite_test.go (1)
127-127: Suite setup tracks the relocated flag.Propagating
AuthenticateSchemaRequestsviaappCfg.Gateway.IntrospectionAuthenticationkeeps the tests aligned with the new config shape.common/config/config.go (1)
4-36: Config struct reshaping is consistent.Moving
IntrospectionAuthenticationunderGatewaywithgateway-mapstructure tags matches the new defaults and test expectations.gateway/manager/targetcluster/registry.go (3)
26-31: LGTM! ClusterRegistry simplified.The removal of the
roundTripperFactoryfield simplifies the struct and aligns with the PR objectives.
34-43: LGTM! Signature simplified.The removal of the
roundTripperFactoryparameter is consistent with the struct changes and simplifies the constructor.
59-59: LGTM! Call updated to match new signature.The call to
NewTargetClustercorrectly omits theroundTripperFactoryargument, matching the updated function signature.
…teway into config/IntrospectionAuthentication
…teway into config/IntrospectionAuthentication
…teway into config/IntrospectionAuthentication
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
gateway/manager/targetcluster/registry.go (1)
191-216: CRITICAL: Authentication logic still incorrect - requires token for all requests.The authentication logic has a critical flaw that contradicts the PR objectives and the previous review feedback.
Current behavior (lines 196-199):
- Rejects ALL requests without a token, regardless of
IntrospectionAuthenticationsetting or query typePR objective states:
"Fixed authentication check to allow proceeding without a token when IntrospectionAuthentication = false."Expected behavior (from past review and PR objectives):
- Only require token for introspection queries when
IntrospectionAuthentication=true- Allow non-introspection queries without a token
- Allow all queries without a token when
IntrospectionAuthentication=falseApply this diff to fix the authentication logic:
func (cr *ClusterRegistry) handleAuth(w http.ResponseWriter, r *http.Request, token string, cluster *TargetCluster) bool { if cr.appCfg.LocalDevelopment { return true } - if token == "" { - http.Error(w, "Authorization header is required", http.StatusUnauthorized) - return false - } - if cr.appCfg.Gateway.IntrospectionAuthentication && IsIntrospectionQuery(r) { + if token == "" { + http.Error(w, "Authorization header is required", http.StatusUnauthorized) + return false + } + valid, err := cr.validateToken(r.Context(), token, cluster) if err != nil { cr.log.Error().Err(err).Str("cluster", cluster.name).Msg("Error validating token") http.Error(w, "Token validation failed", http.StatusUnauthorized) return false } if !valid { cr.log.Debug().Str("cluster", cluster.name).Msg("Invalid token for introspection query") http.Error(w, "Invalid token", http.StatusUnauthorized) return false } } return true }
🧹 Nitpick comments (1)
gateway/manager/targetcluster/cluster.go (1)
231-256: Consider using typed error checking instead of string matching.The token validation logic uses string matching on error messages (lines 242, 246) to distinguish between 401 (unauthorized) and 403 (forbidden) errors. This is fragile and may break if error message formats change.
Consider checking the error type or status code directly:
func (tc *TargetCluster) ValidateToken(ctx context.Context, token string) (bool, error) { newCtx := context.WithValue(ctx, roundtripper.TokenKey{}, token) configMapList := &corev1.ConfigMapList{} listOpts := &client.ListOptions{ Limit: 1, } err := tc.client.List(newCtx, configMapList, listOpts) if err != nil { - errStrLower := strings.ToLower(err.Error()) - if strings.Contains(errStrLower, "unauthorized") || strings.Contains(errStrLower, "401") { + if apierrors.IsUnauthorized(err) { tc.log.Debug().Err(err).Str("cluster", tc.name).Msg("Token is invalid - unauthorized") return false, nil } - if strings.Contains(errStrLower, "forbidden") || strings.Contains(errStrLower, "403") || strings.Contains(errStrLower, "access denied") { + if apierrors.IsForbidden(err) { tc.log.Debug().Str("cluster", tc.name).Msg("Token is valid but user has no permission to list configmaps") return true, nil } tc.log.Error().Err(err).Str("cluster", tc.name).Msg("Unexpected error during token validation") return false, err } tc.log.Debug().Str("cluster", tc.name).Msg("Token validated successfully") return true, nil }You'll need to add the import:
import ( apierrors "k8s.io/apimachinery/pkg/api/errors" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
cmd/root.go(2 hunks)gateway/manager/roundtripper/roundtripper.go(4 hunks)gateway/manager/targetcluster/cluster.go(7 hunks)gateway/manager/targetcluster/registry.go(5 hunks)go.mod(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/manager/targetcluster/registry.go (3)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(54-62)NewTargetCluster(65-98)common/config/config.go (1)
Config(3-36)gateway/manager/targetcluster/graphql.go (1)
IsIntrospectionQuery(81-97)
gateway/manager/targetcluster/cluster.go (4)
common/config/config.go (1)
Config(3-36)gateway/manager/roundtripper/roundtripper.go (4)
NewBaseRoundTripper(44-53)New(28-36)NewUnauthorizedRoundTripper(39-41)TokenKey(17-17)gateway/schema/schema.go (1)
New(39-54)gateway/resolver/resolver.go (1)
New(69-75)
gateway/manager/roundtripper/roundtripper.go (1)
common/config/config.go (1)
Config(3-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / lint / lint
- GitHub Check: pipe / testSource / test
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
cmd/root.go (1)
69-69: Verify thatfalseis the appropriate default for introspection authentication.Setting
gateway-introspection-authenticationtofalseby default means GraphQL introspection queries can be performed without authentication, potentially exposing schema information. While this may be intentional for development convenience, ensure this aligns with your security requirements, especially if the gateway will be deployed in environments where schema information should be protected.go.mod (2)
27-27: Verify necessity of dual JWT libraries.The module now includes both
github.com/golang-jwt/jwt/v5(line 20) andgithub.com/lestrrat-go/jwx/v2(line 27). Clarify whether both JWT libraries are necessary or if one should replace the other to avoid maintenance overhead and potential inconsistencies in token validation logic.Also applies to: 20-20
27-27: I need to verify if there are any other advisories specific to v2.1.6 or the v2.1.x branch, and check the transitive dependencies for vulnerabilities.v2.1.6 is secure and not affected by known vulnerabilities.
The major vulnerabilities in lestrrat-go/jwx (CVE-2023-49290, CVE-2024-21664, CVE-2024-28122) were fixed in versions 2.0.18, 2.0.19, and 2.0.21 respectively. Since v2.1.6 is a later minor version in the v2 branch, it includes these security patches. Transitive dependencies including secp256k1/v4 and goccy/go-json have no publicly disclosed vulnerabilities. The addition of this dependency and its transitive dependencies introduces no known security risks.
gateway/manager/targetcluster/cluster.go (3)
191-191: LGTM!The alias
gwschemaimproves code clarity by distinguishing the gateway schema package from other potential schema packages.
4-4: LGTM!All new imports are properly used in the added/modified functionality.
Also applies to: 13-13, 20-20, 22-22
119-133: The security concern about TLS configuration bypass is valid, but the proposed fix is overcorrective and will break cluster access.The current code falls back to
http.DefaultTransportif TLS configuration fails, which may bypass CA certificate validation. However, the suggested fix returnsNewUnauthorizedRoundTripper()from the Wrap callback, which would reject all requests (discovery, admin, bearer token) with 401 status—breaking the entire cluster connection instead of safely failing.The
baseRTis only used for bearer token authentication (line 91 ofroundtripper.go), while discovery and admin requests useadminRT. Returning an unauthorized transport from the Wrap function affects all request types and is too drastic.Recommendation: Verify whether the current fallback is intentional, and if not, consider a more targeted fix—such as failing the connection during
connect()setup rather than at request time, or handling the TLS configuration failure only for the bearer token path.gateway/manager/roundtripper/roundtripper.go (3)
43-53: LGTM!The
NewBaseRoundTrippercorrectly creates a transport with TLS configuration but no authentication, which aligns with the PR objective to use baseRT (no admin credentials) for token-based authentication.
88-91: LGTM! Core authentication fix implemented correctly.The change from
adminRTtobaseRT(line 91) is the key fix mentioned in the PR objectives. Previously,adminRTworked without token validation, effectively accepting any token. Now,baseRTproperly enforces token authentication viaNewBearerAuthRoundTripper.This ensures that when
GatewayShouldImpersonate=false, a valid token is actually required and validated.Based on past review comment from vertex451.
85-85: LGTM!Using
utilnet.CloneRequestfrom the Kubernetes utility library is more maintainable than custom request cloning logic.gateway/manager/targetcluster/registry.go (2)
232-252: LGTM!The simplified
validateTokenmethod correctly delegates tocluster.ValidateToken, removing the previous complexity of constructing HTTP clients and REST configs inline. This is a cleaner separation of concerns.
24-40: LGTM!Removal of the
RoundTripperFactorypattern simplifies the code as intended in the PR objectives. The factory logic is now internalized within the cluster connection flow, making the registry's responsibilities clearer.Also applies to: 56-56
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
gateway/manager/targetcluster/cluster.go (1)
230-255: Consider edge case where valid tokens lack ConfigMap permissions.The token validation logic lists ConfigMaps to check token validity. While the implementation correctly returns
truefor 403/forbidden responses (valid token without ConfigMap list permissions), this approach may produce false negatives if a user has valid tokens for other resources but specifically lacks ConfigMap permissions.This may be acceptable depending on your RBAC setup, but consider whether a more universally accessible resource would be better for validation (e.g., listing namespaces, which most authenticated users can do, or using a SelfSubjectAccessReview).
If you prefer to use SelfSubjectAccessReview for more reliable validation:
func (tc *TargetCluster) ValidateToken(ctx context.Context, token string) (bool, error) { newCtx := context.WithValue(ctx, roundtripper.TokenKey{}, token) // Use SelfSubjectAccessReview to validate token sar := &authorizationv1.SelfSubjectAccessReview{ Spec: authorizationv1.SelfSubjectAccessReviewSpec{ ResourceAttributes: &authorizationv1.ResourceAttributes{ Verb: "list", Resource: "namespaces", }, }, } err := tc.client.Create(newCtx, sar) if err != nil { errStrLower := strings.ToLower(err.Error()) if strings.Contains(errStrLower, "unauthorized") || strings.Contains(errStrLower, "401") { tc.log.Debug().Err(err).Str("cluster", tc.name).Msg("Token is invalid - unauthorized") return false, nil } tc.log.Error().Err(err).Str("cluster", tc.name).Msg("Unexpected error during token validation") return false, err } tc.log.Debug().Str("cluster", tc.name).Msg("Token validated successfully") return true, nil }Note: You'll need to add
authorizationv1 "k8s.io/api/authorization/v1"import.gateway/manager/targetcluster/registry.go (1)
196-209: Consider checking for empty token before validation call.When
IntrospectionAuthenticationis enabled for an introspection query, the code callsvalidateTokenwithout first checking if the token is empty. WhilevalidateTokendoes handle empty tokens (lines 237-239), it would be clearer and more efficient to check upfront.Apply this diff for improved clarity:
if IsIntrospectionQuery(r) { if cr.appCfg.Gateway.IntrospectionAuthentication { + if token == "" { + cr.log.Error().Str("cluster", cluster.name).Msg("No token provided for introspection query") + http.Error(w, "Authorization header is required", http.StatusUnauthorized) + return false + } + valid, err := cr.validateToken(r.Context(), token, cluster) if err != nil { cr.log.Error().Err(err).Str("cluster", cluster.name).Msg("Error validating token")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/root.go(1 hunks)gateway/manager/roundtripper/roundtripper.go(4 hunks)gateway/manager/targetcluster/cluster.go(7 hunks)gateway/manager/targetcluster/registry.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
gateway/manager/roundtripper/roundtripper.go (1)
common/config/config.go (1)
Config(3-36)
gateway/manager/targetcluster/cluster.go (4)
common/config/config.go (1)
Config(3-36)gateway/manager/roundtripper/roundtripper.go (4)
NewBaseRoundTripper(43-52)New(27-35)NewUnauthorizedRoundTripper(38-40)TokenKey(16-16)gateway/schema/schema.go (1)
New(39-54)gateway/resolver/resolver.go (1)
New(69-75)
gateway/manager/targetcluster/registry.go (3)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(54-62)NewTargetCluster(65-98)common/config/config.go (1)
Config(3-36)gateway/manager/targetcluster/graphql.go (1)
IsIntrospectionQuery(81-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
cmd/root.go (1)
68-68: LGTM! Configuration properly relocated under gateway section.The introspection authentication setting has been correctly moved under the gateway configuration hierarchy with an appropriate default value of
false.gateway/manager/roundtripper/roundtripper.go (3)
9-10: LGTM! New imports and field support the refactored authentication flow.The
baseRTfield and supporting imports enable bearer token authentication without admin credentials, which aligns with the PR's goal of requiring valid tokens whenGatewayShouldImpersonate=false.Also applies to: 21-21
42-52: LGTM! Clean implementation of base transport creation.The function correctly creates a transport with TLS configuration only, providing the foundation for bearer token authentication without admin credentials.
84-89: LGTM! Correct implementation of request cloning and bearer authentication.Cloning the request before modifying headers is proper practice, and using
baseRT(instead ofadminRT) ensures that bearer tokens are actually validated, addressing the PR's goal of requiring valid tokens for authentication.gateway/manager/targetcluster/cluster.go (3)
83-83: LGTM! Simplified connection flow by removing factory parameter.The removal of
roundTripperFactorysimplifies the API and consolidates transport configuration within the cluster connection logic.Also applies to: 101-101
119-132: LGTM! Proper transport wiring with separated admin and base round trippers.The implementation correctly creates a base transport and wires it through the REST config wrapper, enabling proper token-based authentication alongside admin operations.
190-190: LGTM! Schema package alias improves code clarity.Using the
gwschemaalias helps disambiguate when multiple schema packages might be in scope.gateway/manager/targetcluster/registry.go (3)
24-39: LGTM! Registry simplified by removing round tripper factory.The removal of
RoundTripperFactoryfrom the registry aligns with the broader refactoring to consolidate transport configuration within individual clusters.
56-56: LGTM! Cluster instantiation updated to match new signature.
236-256: LGTM! Token validation properly delegates to cluster.The validation logic correctly delegates to the cluster's
ValidateTokenmethod and handles the results appropriately.
| return false | ||
| } | ||
|
|
||
| func (cr *ClusterRegistry) validateToken(ctx context.Context, token string, cluster *TargetCluster) (bool, error) { |
There was a problem hiding this comment.
This is moved to Cluster receiver where I use directly its connection to the cluster, not HTTP transport
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/manager/targetcluster/registry.go (1)
195-210: Consider explicit empty token check for introspection queries.When
IntrospectionAuthenticationis true and the request is an introspection query, the code callsValidateTokeneven iftokenis empty (line 197). While this likely works (assumingValidateTokenhandles empty tokens), an explicit check would make the logic clearer and provide a more specific error message.Apply this diff to add an explicit empty token check:
if IsIntrospectionQuery(r) { if cr.appCfg.Gateway.IntrospectionAuthentication { + if token == "" { + http.Error(w, "Authorization header is required for introspection queries", http.StatusUnauthorized) + return false + } + valid, err := cluster.ValidateToken(r.Context(), token) if err != nil { cr.log.Error().Err(err).Str("cluster", cluster.name).Msg("Token validation failed") http.Error(w, "Token validation failed", http.StatusUnauthorized) return false } if !valid { cr.log.Debug().Str("cluster", cluster.name).Msg("Invalid token for introspection query") http.Error(w, "Invalid token", http.StatusUnauthorized) return false } } return true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gateway/manager/roundtripper/roundtripper_test.go(8 hunks)gateway/manager/targetcluster/registry.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/manager/roundtripper/roundtripper_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/manager/targetcluster/registry.go (3)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(54-62)NewTargetCluster(65-98)common/config/config.go (1)
Config(3-36)gateway/manager/targetcluster/graphql.go (1)
IsIntrospectionQuery(81-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pipe / testSource / test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
gateway/manager/targetcluster/registry.go (5)
23-26: LGTM - Clean removal of RoundTripperFactory.The struct simplification aligns with the PR's goal to remove the factory pattern. Field ordering is clean and logical.
30-38: LGTM - Constructor simplified appropriately.The removal of the
roundTripperFactoryparameter aligns with the architectural simplification. The constructor is now cleaner and easier to maintain.
55-55: LGTM - NewTargetCluster call updated correctly.The call signature matches the updated
NewTargetClusterconstructor that no longer requires aroundTripperFactoryparameter.
168-170: LGTM - Proper ordering of context setup and authentication.Calling
SetContextsbeforehandleAuthensures the request context is properly initialized before authentication checks, which is the correct sequence.
213-218: Token validation for non-introspection queries is properly delegated to downstream layers.The verification confirms your concern is valid. The code requires a token for non-introspection queries (lines 213-216) but performs no validation within the gateway handler. Token validation occurs downstream in the roundtripper layer:
- Bearer token mode (
ShouldImpersonate=false): Token passed directly to Kubernetes cluster viatransport.NewBearerAuthRoundTripper, where the cluster validates it- Impersonation mode (
ShouldImpersonate=true): Token parsed (unverified) to extract username claim, then used withtransport.NewImpersonatingRoundTripper, which sends the impersonation request to the cluster using admin credentialsThis is the intended design: the gateway authenticates the client and delegates authorization to the Kubernetes cluster.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/manager/roundtripper/roundtripper_test.go (1)
46-54: Consider using separate mocks for adminRT and baseRT to verify correct round-tripper selection.The test case "valid_token_without_impersonation" passes
mockAdminfor bothadminRoundTripperandbaseRoundTripperparameters (line 80), which prevents verification that the implementation correctly selectsbaseRTwhenShouldImpersonate = false. Given the PR's emphasis on distinguishing these two round-trippers—specifically that non-impersonation mode should usebaseRT(without admin credentials)—using separate mocks would strengthen test coverage.Consider this approach:
{ name: "valid_token_without_impersonation", token: "valid-token", localDevelopment: false, shouldImpersonate: false, expectedStatusCode: http.StatusOK, - setupMocks: func(admin, unauthorized *mocks.MockRoundTripper) { - admin.EXPECT().RoundTrip(mock.Anything).Return(&http.Response{StatusCode: http.StatusOK}, nil) + setupMocks: func(admin, base, unauthorized *mocks.MockRoundTripper) { + base.EXPECT().RoundTrip(mock.Anything).Return(&http.Response{StatusCode: http.StatusOK}, nil) }, },And update the test function signature and instantiation accordingly:
tests := []struct { name string token string localDevelopment bool shouldImpersonate bool expectedStatusCode int - setupMocks func(*mocks.MockRoundTripper, *mocks.MockRoundTripper) + setupMocks func(admin, base, unauthorized *mocks.MockRoundTripper) }{for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { mockAdmin := &mocks.MockRoundTripper{} + mockBase := &mocks.MockRoundTripper{} mockUnauthorized := &mocks.MockRoundTripper{} - tt.setupMocks(mockAdmin, mockUnauthorized) + tt.setupMocks(mockAdmin, mockBase, mockUnauthorized) appCfg := appConfig.Config{ LocalDevelopment: tt.localDevelopment, } appCfg.Gateway.ShouldImpersonate = tt.shouldImpersonate appCfg.Gateway.UsernameClaim = "sub" - rt := roundtripper.New(testlogger.New().Logger, appCfg, mockAdmin, mockAdmin, mockUnauthorized) + rt := roundtripper.New(testlogger.New().Logger, appCfg, mockAdmin, mockBase, mockUnauthorized)Similar changes would benefit
TestRoundTripper_DiscoveryRequests,TestRoundTripper_ComprehensiveFunctionality,TestRoundTripper_KCPDiscoveryRequests, andTestRoundTripper_InvalidTokenSecurityFix.Also applies to: 80-80
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gateway/manager/roundtripper/roundtripper_test.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/manager/roundtripper/roundtripper_test.go (3)
gateway/manager/roundtripper/roundtripper.go (1)
New(27-35)gateway/manager/mocks/mock_RoundTripper.go (1)
MockRoundTripper(12-14)common/config/config.go (1)
Config(3-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / testSource / test
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
gateway/manager/roundtripper/roundtripper_test.go (2)
513-552: LGTM! Correct usage of separate mocks to verify baseRT in non-impersonation mode.This test correctly instantiates a separate
mockBase(line 518) and verifies thatbaseRTis called whenShouldImpersonate = false(line 523, 531). The test also properly validates that authorization headers are cleaned before the user token is applied, which aligns with the PR's security improvements.
554-603: LGTM! Correct usage of separate mocks to verify adminRT in impersonation mode.This test correctly uses a separate
mockBase(line 559) while verifying thatadminRTis called whenShouldImpersonate = true(line 564, 572). The test properly validates that authorization headers are cleaned and theImpersonate-Userheader is set with the correct user identity, which aligns with the PR's authentication improvements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gateway/manager/targetcluster/registry.go (1)
230-247: Consider more robust introspection query detection.The current implementation uses
strings.Containsto check for__schemaor__typeanywhere in the query string (lines 239). This approach has limitations:
- False positives: Matches these strings in comments, string literals, or field names (e.g.,
my__schema_version,"relates to __schema")- Performance: Reads and buffers the entire request body on every non-GET request
While false positives err on the side of stricter authentication (which is safer), they could cause unexpected behavior for legitimate queries.
For more accurate detection, consider parsing the GraphQL query AST to specifically identify
__schemaor__typeas field selections rather than using string matching. Libraries likegithub.com/graphql-go/graphql/language/parsercan help with this.Example approach:
import "github.com/graphql-go/graphql/language/parser" func isIntrospectionQuery(r *http.Request) bool { // Read and parse body bodyBytes, _ := io.ReadAll(r.Body) r.Body = io.NopCloser(bytes.NewBuffer(bodyBytes)) var params struct { Query string `json:"query"` } if err := json.Unmarshal(bodyBytes, ¶ms); err != nil { return false } // Parse GraphQL query doc, err := parser.Parse(parser.ParseParams{Source: params.Query}) if err != nil { return false } // Check if any selections are introspection fields // ... traverse AST looking for __schema or __type selections }Alternatively, if the current approach is sufficient for your use case, document the known limitations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
gateway/manager/targetcluster/export_test.go(2 hunks)gateway/manager/targetcluster/graphql.go(0 hunks)gateway/manager/targetcluster/registry.go(5 hunks)
💤 Files with no reviewable changes (1)
- gateway/manager/targetcluster/graphql.go
🧰 Additional context used
🧬 Code graph analysis (1)
gateway/manager/targetcluster/registry.go (2)
gateway/manager/targetcluster/cluster.go (2)
TargetCluster(54-62)NewTargetCluster(65-98)common/config/config.go (1)
Config(3-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pipe / dockerBuild / docker
- GitHub Check: pipe / testSource / test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
gateway/manager/targetcluster/export_test.go (1)
24-26: LGTM! Test helper export is appropriate.The export wrapper follows standard Go testing practices for exposing internal functions to test code.
gateway/manager/targetcluster/registry.go (1)
193-228: Verify intended behavior: introspection queries can bypass token requirement.The current logic allows introspection queries to proceed without a token when
IntrospectionAuthentication = false(line 219), while non-introspection queries always require a token (lines 222-225). This creates a potential security gap:
- An attacker could craft a query containing both introspection fields (
__schema,__type) and regular data fields- If classified as an introspection query, it would bypass the token requirement when
IntrospectionAuthentication = false- The query would still execute and could retrieve actual data without authentication
Additionally, the naive string-matching in
isIntrospectionQuery(checking for__schemaor__typeanywhere in the query string) could misclassify queries that mention these terms in comments, strings, or field names.Consider whether:
- Introspection queries should always require at least the same level of authentication as regular queries
- Or if introspection-only queries should be distinguished from mixed queries (introspection + data)
You can verify if this behavior is intentional by checking the original requirements and testing with mixed queries.
P.S. No helm charts adjustments are needed.
Testing
Tested in local-setup at 5cec80b commit 24.10 by creating an account
Changes
Summary by CodeRabbit
Refactor
CLI
Behavior
Tests
Chores