diff --git a/api/v1beta3/auth_config_types.go b/api/v1beta3/auth_config_types.go index c2499cd63..f1991f248 100644 --- a/api/v1beta3/auth_config_types.go +++ b/api/v1beta3/auth_config_types.go @@ -396,6 +396,15 @@ type JwtAuthenticationSpec struct { // +optional // +kubebuilder:validation:Minimum:=0 Timeout *int `json:"timeout,omitempty"` + + // Maximum number of bytes to read from OIDC discovery and JWKS HTTP response bodies. + // When set, response body readers will be limited to this size, preventing + // unbounded memory consumption from unexpectedly large responses. + // If the response exceeds this limit, the truncation will cause a decode error. + // If omitted, no limit is applied. + // +optional + // +kubebuilder:validation:Minimum:=1 + MaxResponseBytes *int64 `json:"maxResponseBytes,omitempty"` } // Settings to perform the OAuth2 token introspection request. @@ -420,6 +429,15 @@ type OAuth2TokenIntrospectionSpec struct { // +optional // +kubebuilder:validation:Minimum:=0 Timeout *int `json:"timeout,omitempty"` + + // Maximum number of bytes to read from the token introspection HTTP response body. + // When set, the response body reader will be limited to this size, preventing + // unbounded memory consumption from unexpectedly large responses. + // If the response is JSON and exceeds this limit, the truncation will cause a decode error. + // If omitted, no limit is applied. + // +optional + // +kubebuilder:validation:Minimum:=1 + MaxResponseBytes *int64 `json:"maxResponseBytes,omitempty"` } // Parameters of the Kubernetes TokenReview request @@ -600,6 +618,15 @@ type HttpEndpointSpec struct { // +optional // +kubebuilder:validation:Minimum:=0 Timeout *int `json:"timeout,omitempty"` + + // Maximum number of bytes to read from the HTTP response body. + // When set, the response body reader will be limited to this size, preventing + // unbounded memory consumption from unexpectedly large responses. + // If the response is JSON and exceeds this limit, the truncation will cause a decode error. + // If omitted, no limit is applied. + // +optional + // +kubebuilder:validation:Minimum:=1 + MaxResponseBytes *int64 `json:"maxResponseBytes,omitempty"` } // +kubebuilder:validation:Enum:=GET;POST;PUT;PATCH;DELETE;HEAD;OPTIONS;CONNECT;TRACE @@ -666,6 +693,15 @@ type UserInfoMetadataSpec struct { // +optional // +kubebuilder:validation:Minimum:=0 Timeout *int `json:"timeout,omitempty"` + + // Maximum number of bytes to read from the UserInfo HTTP response body. + // When set, the response body reader will be limited to this size, preventing + // unbounded memory consumption from unexpectedly large responses. + // If the response exceeds this limit, the truncation will cause a decode error. + // If omitted, no limit is applied. + // +optional + // +kubebuilder:validation:Minimum:=1 + MaxResponseBytes *int64 `json:"maxResponseBytes,omitempty"` } // Settings of the User-Managed Access (UMA) source of resource data. @@ -686,6 +722,15 @@ type UmaMetadataSpec struct { // +optional // +kubebuilder:validation:Minimum:=0 Timeout *int `json:"timeout,omitempty"` + + // Maximum number of bytes to read from UMA HTTP response bodies (discovery, PAT, resource queries). + // When set, response body readers will be limited to this size, preventing + // unbounded memory consumption from unexpectedly large responses. + // If the response exceeds this limit, the truncation will cause a decode error. + // If omitted, no limit is applied. + // +optional + // +kubebuilder:validation:Minimum:=1 + MaxResponseBytes *int64 `json:"maxResponseBytes,omitempty"` } // +kubebuilder:validation:XValidation:rule="has(self.patternMatching) ? !(has(self.opa) || has(self.kubernetesSubjectAccessReview) || has(self.spicedb)) : has(self.opa) ? !(has(self.kubernetesSubjectAccessReview) || has(self.spicedb)) : has(self.kubernetesSubjectAccessReview) != has(self.spicedb)",message="Use exactly one of: patternMatching, opa, kubernetesSubjectAccessReview, spicedb" diff --git a/api/v1beta3/zz_generated.deepcopy.go b/api/v1beta3/zz_generated.deepcopy.go index abae85c9a..c88bd3683 100644 --- a/api/v1beta3/zz_generated.deepcopy.go +++ b/api/v1beta3/zz_generated.deepcopy.go @@ -722,6 +722,11 @@ func (in *HttpEndpointSpec) DeepCopyInto(out *HttpEndpointSpec) { *out = new(int) **out = **in } + if in.MaxResponseBytes != nil { + in, out := &in.MaxResponseBytes, &out.MaxResponseBytes + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HttpEndpointSpec. @@ -764,6 +769,11 @@ func (in *JwtAuthenticationSpec) DeepCopyInto(out *JwtAuthenticationSpec) { *out = new(int) **out = **in } + if in.MaxResponseBytes != nil { + in, out := &in.MaxResponseBytes, &out.MaxResponseBytes + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JwtAuthenticationSpec. @@ -996,6 +1006,11 @@ func (in *OAuth2TokenIntrospectionSpec) DeepCopyInto(out *OAuth2TokenIntrospecti *out = new(int) **out = **in } + if in.MaxResponseBytes != nil { + in, out := &in.MaxResponseBytes, &out.MaxResponseBytes + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OAuth2TokenIntrospectionSpec. @@ -1296,6 +1311,11 @@ func (in *UmaMetadataSpec) DeepCopyInto(out *UmaMetadataSpec) { *out = new(int) **out = **in } + if in.MaxResponseBytes != nil { + in, out := &in.MaxResponseBytes, &out.MaxResponseBytes + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UmaMetadataSpec. @@ -1332,6 +1352,11 @@ func (in *UserInfoMetadataSpec) DeepCopyInto(out *UserInfoMetadataSpec) { *out = new(int) **out = **in } + if in.MaxResponseBytes != nil { + in, out := &in.MaxResponseBytes, &out.MaxResponseBytes + *out = new(int64) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserInfoMetadataSpec. diff --git a/controllers/auth_config_controller.go b/controllers/auth_config_controller.go index 888f9ebf8..604aae1d3 100644 --- a/controllers/auth_config_controller.go +++ b/controllers/auth_config_controller.go @@ -375,15 +375,22 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf authCred, ) oauth2.Timeout = oauth2Identity.Timeout + if oauth2Identity.MaxResponseBytes != nil { + oauth2.MaxResponseBytes = *oauth2Identity.MaxResponseBytes + } translatedIdentity.OAuth2 = oauth2 // oidc case api.JwtAuthentication: var jwtVerifier identity_evaluators.JWTVerifier + var jwtMaxResponseBytes int64 + if identity.Jwt.MaxResponseBytes != nil { + jwtMaxResponseBytes = *identity.Jwt.MaxResponseBytes + } if identity.Jwt.IssuerUrl != "" { - jwtVerifier = identity_evaluators.NewOIDCProviderVerifier(ctx, identity.Jwt.IssuerUrl, identity.Jwt.TTL, identity.Jwt.Timeout) + jwtVerifier = identity_evaluators.NewOIDCProviderVerifier(ctx, identity.Jwt.IssuerUrl, identity.Jwt.TTL, identity.Jwt.Timeout, jwtMaxResponseBytes) } else if identity.Jwt.JwksUrl != "" { - jwtVerifier = identity_evaluators.NewJwksVerifier(ctx, identity.Jwt.JwksUrl, identity.Jwt.Timeout) + jwtVerifier = identity_evaluators.NewJwksVerifier(ctx, identity.Jwt.JwksUrl, identity.Jwt.Timeout, jwtMaxResponseBytes) } else { return nil, fmt.Errorf("missing issuerUrl or jwksUrl for JWT authentication method") // should never happen if properly validated at the API level } @@ -499,17 +506,22 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf return nil, err // TODO: Review this error, perhaps we don't need to return an error, just reenqueue. } + var umaMaxResponseBytes int64 + if metadata.Uma.MaxResponseBytes != nil { + umaMaxResponseBytes = *metadata.Uma.MaxResponseBytes + } if uma, err := metadata_evaluators.NewUMAMetadata( ctx, metadata.Uma.Endpoint, string(secret.Data["clientID"]), string(secret.Data["clientSecret"]), + metadata.Uma.Timeout, + umaMaxResponseBytes, ); err != nil { span.RecordError(err) span.SetStatus(codes.Error, "failed to create UMA metadata evaluator") return nil, err } else { - uma.Timeout = metadata.Uma.Timeout translatedMetadata.UMA = uma } @@ -527,6 +539,9 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf } userInfo := metadata_evaluators.NewUserInfo(openIdConfigStore, metadata.UserInfo.UserInfoUrl) userInfo.Timeout = metadata.UserInfo.Timeout + if metadata.UserInfo.MaxResponseBytes != nil { + userInfo.MaxResponseBytes = *metadata.UserInfo.MaxResponseBytes + } translatedMetadata.UserInfo = userInfo // generic http @@ -609,12 +624,18 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf sharedSecret = string(secret.Data[externalRegistry.SharedSecret.Key]) } + var opaMaxResponseBytes int64 + if externalRegistry.MaxResponseBytes != nil { + opaMaxResponseBytes = *externalRegistry.MaxResponseBytes + } + externalSource = &authorization_evaluators.OPAExternalSource{ - Endpoint: externalRegistry.Url, - SharedSecret: sharedSecret, - AuthCredentials: newAuthCredential(externalRegistry.Credentials), - TTL: externalRegistry.TTL, - Timeout: externalRegistry.Timeout, + Endpoint: externalRegistry.Url, + SharedSecret: sharedSecret, + AuthCredentials: newAuthCredential(externalRegistry.Credentials), + TTL: externalRegistry.TTL, + Timeout: externalRegistry.Timeout, + MaxResponseBytes: opaMaxResponseBytes, } } @@ -1221,6 +1242,11 @@ func (r *AuthConfigReconciler) buildGenericHttpEvaluator(ctx context.Context, ht } } + var maxResponseBytes int64 + if http.MaxResponseBytes != nil { + maxResponseBytes = *http.MaxResponseBytes + } + ev := &metadata_evaluators.GenericHttp{ Endpoint: http.Url, DynamicEndpoint: dynamicEndpoint, @@ -1233,6 +1259,7 @@ func (r *AuthConfigReconciler) buildGenericHttpEvaluator(ctx context.Context, ht OAuth2: oauth2ClientCredentialsConfig, OAuth2TokenForceFetch: oauth2TokenForceFetch, Timeout: http.Timeout, + MaxResponseBytes: maxResponseBytes, } if sharedSecret != "" || oauth2ClientCredentialsConfig != nil { diff --git a/docs/features.md b/docs/features.md index d99c185ef..a78e8e757 100644 --- a/docs/features.md +++ b/docs/features.md @@ -421,6 +421,35 @@ In both cases, the location where the secret (long-lived or OAuth2 access token) Custom headers can be set with the `headers` field. Nevertheless, headers such as `Content-Type` and `Authorization` (or eventual custom header used for carrying the authentication secret, set instead via the `credentials` option) will be superseded by the respective values defined for the fields `contentType` and `sharedSecretRef`. +#### Limiting response size (`maxResponseBytes`) + +To protect Authorino from memory exhaustion caused by unexpectedly large HTTP responses from external services, set the `maxResponseBytes` field to cap the number of bytes read from the response body. + +```yaml +spec: + metadata: + my-metadata: + http: + url: https://external-service/metadata + maxResponseBytes: 65536 # 64 KiB +``` + +The same field is available in: +- `spec.callbacks.*.http` — HTTP callbacks +- `spec.authorization.*.opa.externalPolicy` — external OPA policy bundles +- `spec.authentication.*.jwt` — OIDC discovery and JWKS fetching +- `spec.authentication.*.oauth2Introspection` — OAuth2 token introspection +- `spec.metadata.*.userInfo` — OIDC UserInfo +- `spec.metadata.*.uma` — UMA resource registry (applies to discovery, PAT requests, and resource queries) + +> **Important:** When a response exceeds the limit, the body is truncated to the specified size. For `application/json` responses, this truncation produces malformed JSON that can cause a decode error. As a result, the evaluator will fail and the corresponding piece of the auth pipeline will not be available. +> +> Depending on how downstream authorization policies handle missing metadata, the outcome can vary: +> - **Policies that deny access when metadata is absent**: the request will be denied. This is the safer default. +> - **Policies that fall back to granting access on the absence of metadata**: truncation may cause the policy to inadvertently allow a request that should have been denied. Review your authorization policies to ensure they do not assume missing metadata means "allowed." +> +> When `maxResponseBytes` is omitted, no limit is applied and the full response body is read. + ### OIDC UserInfo ([`metadata.userInfo`](https://pkg.go.dev/github.com/kuadrant/authorino/api/v1beta3?utm_source=gopls#UserInfoMetadataSpec)) Online fetching of OpenID Connect (OIDC) UserInfo data (phase ii of the Authorino [Auth Pipeline](./architecture.md#the-auth-pipeline-aka-enforcing-protection-in-request-time)), associated with an OIDC identity source configured and resolved in phase (i). diff --git a/install/crd/authorino.kuadrant.io_authconfigs.yaml b/install/crd/authorino.kuadrant.io_authconfigs.yaml index b0b449f10..948e05f7a 100644 --- a/install/crd/authorino.kuadrant.io_authconfigs.yaml +++ b/install/crd/authorino.kuadrant.io_authconfigs.yaml @@ -257,6 +257,16 @@ spec: The JSON Web Keys (JWK) obtained from this endpoint are automatically cached and the caching updated whenever the kid of a JWT does not match any of the cached JWKs (https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys). One of: jwksUrl, issuerUrl type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from OIDC discovery and JWKS HTTP response bodies. + When set, response body readers will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for OIDC discovery and JWK fetching HTTP requests, in milliseconds. @@ -319,6 +329,16 @@ spec: IMPORTANT: Ensure this URL points to a trusted OAuth2 server. If this value can be influenced by user input, you may be vulnerable to Server-Side Request Forgery (SSRF) attacks. type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from the token introspection HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for the token introspection HTTP request, in milliseconds. @@ -907,6 +927,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -1472,6 +1502,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -1836,6 +1876,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -2001,6 +2051,16 @@ spec: IMPORTANT: Ensure this URL points to a trusted UMA server. If this value can be influenced by user input, you may be vulnerable to Server-Side Request Forgery (SSRF) attacks. type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from UMA HTTP response bodies (discovery, PAT, resource queries). + When set, response body readers will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for UMA HTTP requests (discovery, PAT, resource queries), in milliseconds. @@ -2021,6 +2081,16 @@ spec: Name of an OIDC JWT authentication rule whose obtained configuration includes an "userinfo_endpoint" claim. One of: identitySource, userInfoUrl type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from the UserInfo HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for the UserInfo HTTP request, in milliseconds. diff --git a/install/manifests.yaml b/install/manifests.yaml index 07722f270..a3d599471 100644 --- a/install/manifests.yaml +++ b/install/manifests.yaml @@ -292,6 +292,16 @@ spec: The JSON Web Keys (JWK) obtained from this endpoint are automatically cached and the caching updated whenever the kid of a JWT does not match any of the cached JWKs (https://openid.net/specs/openid-connect-core-1_0.html#RotateSigKeys). One of: jwksUrl, issuerUrl type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from OIDC discovery and JWKS HTTP response bodies. + When set, response body readers will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for OIDC discovery and JWK fetching HTTP requests, in milliseconds. @@ -354,6 +364,16 @@ spec: IMPORTANT: Ensure this URL points to a trusted OAuth2 server. If this value can be influenced by user input, you may be vulnerable to Server-Side Request Forgery (SSRF) attacks. type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from the token introspection HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for the token introspection HTTP request, in milliseconds. @@ -966,6 +986,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -1579,6 +1609,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -1943,6 +1983,16 @@ spec: type: object description: Custom headers in the HTTP request. type: object + maxResponseBytes: + description: |- + Maximum number of bytes to read from the HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response is JSON and exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer method: default: GET description: |- @@ -2108,6 +2158,16 @@ spec: IMPORTANT: Ensure this URL points to a trusted UMA server. If this value can be influenced by user input, you may be vulnerable to Server-Side Request Forgery (SSRF) attacks. type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from UMA HTTP response bodies (discovery, PAT, resource queries). + When set, response body readers will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for UMA HTTP requests (discovery, PAT, resource queries), in milliseconds. @@ -2128,6 +2188,16 @@ spec: Name of an OIDC JWT authentication rule whose obtained configuration includes an "userinfo_endpoint" claim. One of: identitySource, userInfoUrl type: string + maxResponseBytes: + description: |- + Maximum number of bytes to read from the UserInfo HTTP response body. + When set, the response body reader will be limited to this size, preventing + unbounded memory consumption from unexpectedly large responses. + If the response exceeds this limit, the truncation will cause a decode error. + If omitted, no limit is applied. + format: int64 + minimum: 1 + type: integer timeout: description: |- Timeout for the UserInfo HTTP request, in milliseconds. diff --git a/pkg/evaluators/authorization/opa.go b/pkg/evaluators/authorization/opa.go index 3f3f0597c..3862ef9b8 100644 --- a/pkg/evaluators/authorization/opa.go +++ b/pkg/evaluators/authorization/opa.go @@ -229,9 +229,10 @@ type OPAExternalSource struct { Endpoint string SharedSecret string auth.AuthCredentials - TTL int - Timeout *int - refresher workers.Worker + TTL int + Timeout *int + MaxResponseBytes int64 + refresher workers.Worker } func (ext *OPAExternalSource) downloadRegoDataFromUrl(ctx context.Context) (string, error) { @@ -256,18 +257,27 @@ func (ext *OPAExternalSource) downloadRegoDataFromUrl(ctx context.Context) (stri // Use the caller's context for tracing (so traces are linked), but the request uses Background context otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) - if resp, err := httputil.NewClient(ext.Timeout).Do(req); err != nil { + if resp, err := httputil.NewClient(httputil.WithTimeout(ext.Timeout)).Do(req); err != nil { return "", fmt.Errorf("failed to fetch Rego config: %v", err) } else { defer func(Body io.ReadCloser) { _ = Body.Close() }(resp.Body) - body, err := io.ReadAll(resp.Body) + var bodyReader io.Reader = resp.Body + if ext.MaxResponseBytes > 0 { + bodyReader = io.LimitReader(resp.Body, ext.MaxResponseBytes) + } + + body, err := io.ReadAll(bodyReader) if err != nil { return "", fmt.Errorf("unable to read response body: %v", err) } + if ext.MaxResponseBytes > 0 && int64(len(body)) >= ext.MaxResponseBytes { + return "", fmt.Errorf("response body truncated at %d bytes; refusing to compile a potentially incomplete policy", ext.MaxResponseBytes) + } + if resp.StatusCode != http.StatusOK { return "", fmt.Errorf("%s: %s", resp.Status, body) } diff --git a/pkg/evaluators/authorization/opa_test.go b/pkg/evaluators/authorization/opa_test.go index a4bed8049..bc5969f38 100644 --- a/pkg/evaluators/authorization/opa_test.go +++ b/pkg/evaluators/authorization/opa_test.go @@ -242,6 +242,93 @@ func TestOPANonBooleanAllowed(t *testing.T) { assert.ErrorContains(t, err, "Unauthorized") } +func TestOPAExternalUrlMaxResponseBytes(t *testing.T) { + t.Run("within limit", func(t *testing.T) { + extHttpMetadataServer := httptest.NewHttpServerMock(opaExtHttpServerMockAddr, map[string]httptest.HttpServerMockResponseFunc{ + "/rego": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: opaInlineRegoV1DataMock} + }, + }) + defer extHttpMetadataServer.Close() + + externalSource := &OPAExternalSource{ + Endpoint: "http://" + opaExtHttpServerMockAddr + "/rego", + AuthCredentials: auth.NewAuthCredential("", ""), + MaxResponseBytes: int64(len(opaInlineRegoV1DataMock) + 100), + } + + opa, err := NewOPAAuthorization("test-opa-maxbytes-ok", "", externalSource, false, opaParser.RegoV1, 0, context.TODO()) + + assert.NilError(t, err) + assertOPAAuthorization(t, opa) + }) + + t.Run("exceeds limit truncates and causes error", func(t *testing.T) { + extHttpMetadataServer := httptest.NewHttpServerMock(opaExtHttpServerMockAddr, map[string]httptest.HttpServerMockResponseFunc{ + "/rego": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: opaInlineRegoV1DataMock} + }, + }) + defer extHttpMetadataServer.Close() + + externalSource := &OPAExternalSource{ + Endpoint: "http://" + opaExtHttpServerMockAddr + "/rego", + AuthCredentials: auth.NewAuthCredential("", ""), + MaxResponseBytes: 10, + } + + opa, err := NewOPAAuthorization("test-opa-maxbytes-truncated", "", externalSource, false, opaParser.RegoV1, 0, context.TODO()) + + assert.ErrorContains(t, err, "response body truncated") + assert.Assert(t, opa == nil) + }) + + t.Run("truncated valid rego prefix is rejected", func(t *testing.T) { + // The full policy has two rules; truncating to the first rule yields valid Rego + // that compiles fine but silently drops the deny logic. The truncation check must + // catch this before compilation. + fullPolicy := `allow := true +deny := true` + extHttpMetadataServer := httptest.NewHttpServerMock(opaExtHttpServerMockAddr, map[string]httptest.HttpServerMockResponseFunc{ + "/rego": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: fullPolicy} + }, + }) + defer extHttpMetadataServer.Close() + + externalSource := &OPAExternalSource{ + Endpoint: "http://" + opaExtHttpServerMockAddr + "/rego", + AuthCredentials: auth.NewAuthCredential("", ""), + MaxResponseBytes: int64(len("allow := true\n")), + } + + opa, err := NewOPAAuthorization("test-opa-maxbytes-valid-prefix", "", externalSource, false, opaParser.RegoV1, 0, context.TODO()) + + assert.ErrorContains(t, err, "response body truncated") + assert.Assert(t, opa == nil) + }) + + t.Run("zero means no limit", func(t *testing.T) { + extHttpMetadataServer := httptest.NewHttpServerMock(opaExtHttpServerMockAddr, map[string]httptest.HttpServerMockResponseFunc{ + "/rego": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: opaInlineRegoV1DataMock} + }, + }) + defer extHttpMetadataServer.Close() + + externalSource := &OPAExternalSource{ + Endpoint: "http://" + opaExtHttpServerMockAddr + "/rego", + AuthCredentials: auth.NewAuthCredential("", ""), + MaxResponseBytes: 0, + } + + opa, err := NewOPAAuthorization("test-opa-maxbytes-nolimit", "", externalSource, false, opaParser.RegoV1, 0, context.TODO()) + + assert.NilError(t, err) + assertOPAAuthorization(t, opa) + }) +} + func assertOPAAuthorization(t *testing.T, opa *OPA) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/pkg/evaluators/identity/jwt.go b/pkg/evaluators/identity/jwt.go index d4384c6f8..c28952d15 100644 --- a/pkg/evaluators/identity/jwt.go +++ b/pkg/evaluators/identity/jwt.go @@ -92,18 +92,24 @@ type JWTVerifier interface { } type oidcProviderVerifier struct { - issuerUrl string - timeout *int + issuerUrl string + timeout *int + maxResponseBytes int64 mu sync.RWMutex provider *oidc.Provider refresher workers.Worker } -func NewOIDCProviderVerifier(ctx gocontext.Context, issuerUrl string, ttl int, timeout *int) JWTVerifier { +func NewOIDCProviderVerifier(ctx gocontext.Context, issuerUrl string, ttl int, timeout *int, maxResponseBytes ...int64) JWTVerifier { + var maxBytes int64 + if len(maxResponseBytes) > 0 { + maxBytes = maxResponseBytes[0] + } v := &oidcProviderVerifier{ - issuerUrl: issuerUrl, - timeout: timeout, + issuerUrl: issuerUrl, + timeout: timeout, + maxResponseBytes: maxBytes, } ctxWithLogger := log.IntoContext(ctx, log.FromContext(ctx).WithName("jwt")) v.getOpenIdProvider(ctxWithLogger, false) @@ -170,10 +176,10 @@ func (v *oidcProviderVerifier) getOpenIdProvider(ctx gocontext.Context, force bo defer v.mu.Unlock() if v.provider == nil || force { - // Create HTTP client with timeout and trace propagation. + // Create HTTP client with timeout, trace propagation, and optional response body size limit. // Use Background context for request lifecycle (to avoid cancellation from reconciliation), // but propagate trace context from caller's ctx for observability. - httpClient := httputil.NewClientWithTracing(ctx, v.timeout) + httpClient := httputil.NewClient(httputil.WithTimeout(v.timeout), httputil.WithTracing(ctx), httputil.WithMaxResponseBytes(v.maxResponseBytes)) discoveryCtx := oidc.ClientContext(gocontext.Background(), httpClient) if provider, err := oidc.NewProvider(discoveryCtx, v.issuerUrl); err != nil { @@ -203,11 +209,15 @@ type jwksVerifier struct { jwks oidc.KeySet } -func NewJwksVerifier(ctx gocontext.Context, jwksUrl string, timeout *int) JWTVerifier { - // Create HTTP client with timeout and trace propagation. +func NewJwksVerifier(ctx gocontext.Context, jwksUrl string, timeout *int, maxResponseBytes ...int64) JWTVerifier { + // Create HTTP client with timeout, trace propagation, and optional response body size limit. // Use Background context for request lifecycle (to avoid cancellation from reconciliation), // but propagate trace context from caller's ctx for observability. - httpClient := httputil.NewClientWithTracing(ctx, timeout) + var maxBytes int64 + if len(maxResponseBytes) > 0 { + maxBytes = maxResponseBytes[0] + } + httpClient := httputil.NewClient(httputil.WithTimeout(timeout), httputil.WithTracing(ctx), httputil.WithMaxResponseBytes(maxBytes)) jwkCtx := oidc.ClientContext(gocontext.Background(), httpClient) return &jwksVerifier{ diff --git a/pkg/evaluators/identity/oauth2.go b/pkg/evaluators/identity/oauth2.go index d25348731..9b18d3300 100644 --- a/pkg/evaluators/identity/oauth2.go +++ b/pkg/evaluators/identity/oauth2.go @@ -26,6 +26,7 @@ type OAuth2 struct { ClientID string `yaml:"clientId"` ClientSecret string `yaml:"clientSecret"` Timeout *int + MaxResponseBytes int64 } func NewOAuth2Identity(tokenIntrospectionUrl string, tokenTypeHint string, clientID string, clientSecret string, creds auth.AuthCredentials) *OAuth2 { @@ -77,7 +78,7 @@ func (oauth *OAuth2) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (in otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) - resp, err := httputil.NewClient(oauth.Timeout).Do(req) + resp, err := httputil.NewClient(httputil.WithTimeout(oauth.Timeout)).Do(req) if err != nil { return nil, err } @@ -85,6 +86,11 @@ func (oauth *OAuth2) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (in _ = Body.Close() }(resp.Body) + var bodyReader io.Reader = resp.Body + if oauth.MaxResponseBytes > 0 { + bodyReader = io.LimitReader(resp.Body, oauth.MaxResponseBytes) + } + // a non-200 response (e.g. invalid client credentials) is not a valid introspection // result and may not carry the RFC 7662 "active" field; treat it as an error instead // of attempting to parse it as a successful introspection response. @@ -92,14 +98,14 @@ func (oauth *OAuth2) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (in // the X-Ext-Auth-Reason header) and logged; the response body may contain data from the // auth server and is only logged at debug level. if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) + body, _ := io.ReadAll(bodyReader) log.FromContext(ctx).WithName("oauth2").V(1).Info("token introspection request failed", "status", resp.Status, "response", string(body)) return nil, fmt.Errorf("token introspection request failed: %s", resp.Status) } // parse the response var claims map[string]interface{} - if err := json.NewDecoder(resp.Body).Decode(&claims); err != nil { + if err := json.NewDecoder(bodyReader).Decode(&claims); err != nil { return nil, err } else { active, ok := claims["active"].(bool) diff --git a/pkg/evaluators/identity/oauth2_test.go b/pkg/evaluators/identity/oauth2_test.go index d1e385fed..01198dd30 100644 --- a/pkg/evaluators/identity/oauth2_test.go +++ b/pkg/evaluators/identity/oauth2_test.go @@ -3,6 +3,7 @@ package identity import ( "context" "fmt" + "strings" "testing" mock_auth "github.com/kuadrant/authorino/pkg/auth/mocks" @@ -76,6 +77,53 @@ func TestOAuth2Call(t *testing.T) { } } +func TestOAuth2MaxResponseBytes(t *testing.T) { + largeClaims := `{ "active": true, "sub": "user123", "extra": "` + strings.Repeat("x", 1000) + `" }` + + authServer := httptest.NewHttpServerMock(oauthServerHost, map[string]httptest.HttpServerMockResponseFunc{ + "/introspect-large": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: largeClaims} + }, + }) + defer authServer.Close() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + authCredMock := mock_auth.NewMockAuthCredentials(ctrl) + authCredMock.EXPECT().GetCredentialsFromAuthReq(gomock.Any()).Return("oauth-opaque-token", nil).AnyTimes() + + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetHttp().Return(nil).AnyTimes() + + ctx := context.Background() + + t.Run("within limit", func(t *testing.T) { + oauthEvaluator := NewOAuth2Identity(fmt.Sprintf("http://%v/introspect-large", oauthServerHost), "access_token", "client-id", "client-secret", authCredMock) + oauthEvaluator.MaxResponseBytes = int64(len(largeClaims) + 100) + obj, err := oauthEvaluator.Call(pipelineMock, ctx) + assert.NilError(t, err) + claims := obj.(map[string]interface{}) + assert.Assert(t, claims["active"]) + }) + + t.Run("exceeds limit causes decode error", func(t *testing.T) { + oauthEvaluator := NewOAuth2Identity(fmt.Sprintf("http://%v/introspect-large", oauthServerHost), "access_token", "client-id", "client-secret", authCredMock) + oauthEvaluator.MaxResponseBytes = 50 + _, err := oauthEvaluator.Call(pipelineMock, ctx) + assert.Assert(t, err != nil) + }) + + t.Run("zero means no limit", func(t *testing.T) { + oauthEvaluator := NewOAuth2Identity(fmt.Sprintf("http://%v/introspect-large", oauthServerHost), "access_token", "client-id", "client-secret", authCredMock) + oauthEvaluator.MaxResponseBytes = 0 + obj, err := oauthEvaluator.Call(pipelineMock, ctx) + assert.NilError(t, err) + claims := obj.(map[string]interface{}) + assert.Assert(t, claims["active"]) + }) +} + func TestDefaultTokenTypeHint(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/pkg/evaluators/metadata/generic_http.go b/pkg/evaluators/metadata/generic_http.go index a8f860d9e..50bb76ad7 100644 --- a/pkg/evaluators/metadata/generic_http.go +++ b/pkg/evaluators/metadata/generic_http.go @@ -34,6 +34,7 @@ type GenericHttp struct { OAuth2 *oauth2.ClientCredentials OAuth2TokenForceFetch bool Timeout *int + MaxResponseBytes int64 auth.AuthCredentials } @@ -59,7 +60,7 @@ func (h *GenericHttp) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (i return nil, err } - resp, err := httputil.NewClient(h.Timeout).Do(req) + resp, err := httputil.NewClient(httputil.WithTimeout(h.Timeout)).Do(req) if err != nil { return nil, err } @@ -67,9 +68,14 @@ func (h *GenericHttp) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (i _ = Body.Close() }(resp.Body) + var bodyReader io.Reader = resp.Body + if h.MaxResponseBytes > 0 { + bodyReader = io.LimitReader(resp.Body, h.MaxResponseBytes) + } + // parse the response as json if strings.Contains(strings.Join(resp.Header["Content-Type"], ";"), "application/json") { - decoder := gojson.NewDecoder(resp.Body) + decoder := gojson.NewDecoder(bodyReader) var elements []map[string]interface{} @@ -94,10 +100,7 @@ func (h *GenericHttp) Call(pipeline auth.AuthPipeline, ctx gocontext.Context) (i } // parse the response as text - defer func(Body io.ReadCloser) { - _ = Body.Close() - }(resp.Body) - str, err := io.ReadAll(resp.Body) + str, err := io.ReadAll(bodyReader) if err != nil { return nil, err } diff --git a/pkg/evaluators/metadata/generic_http_test.go b/pkg/evaluators/metadata/generic_http_test.go index b3e42efe8..a756e114c 100644 --- a/pkg/evaluators/metadata/generic_http_test.go +++ b/pkg/evaluators/metadata/generic_http_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" gohttptest "net/http/httptest" + "strings" "testing" mock_auth "github.com/kuadrant/authorino/pkg/auth/mocks" @@ -435,6 +436,113 @@ func TestWithOAuth2AuthenticationWithoutTokenCache(t *testing.T) { assert.Equal(t, objJSON["foo"], "bar") } +func TestGenericHttpMaxResponseBytesJSON(t *testing.T) { + largeJSON := `{"key":"` + strings.Repeat("x", 1000) + `"}` + + extHttpMetadataServer := httptest.NewHttpServerMock(testHttpMetadataServerHost, map[string]httptest.HttpServerMockResponseFunc{ + "/metadata": httptest.NewHttpServerMockResponseFuncJSON(largeJSON), + }) + defer extHttpMetadataServer.Close() + + ctx := context.TODO() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + endpoint := "http://" + testHttpMetadataServerHost + "/metadata" + + t.Run("within limit", func(t *testing.T) { + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetAuthorizationJSON().Return(genericHttpAuthDataMock()) + + metadata := &GenericHttp{ + Endpoint: endpoint, + Method: "GET", + MaxResponseBytes: int64(len(largeJSON) + 100), + } + + obj, err := metadata.Call(pipelineMock, ctx) + assert.NilError(t, err) + objJSON := obj.(map[string]interface{}) + assert.Equal(t, len(objJSON["key"].(string)), 1000) + }) + + t.Run("exceeds limit truncates and causes decode error", func(t *testing.T) { + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetAuthorizationJSON().Return(genericHttpAuthDataMock()) + + metadata := &GenericHttp{ + Endpoint: endpoint, + Method: "GET", + MaxResponseBytes: 10, + } + + _, err := metadata.Call(pipelineMock, ctx) + assert.Assert(t, err != nil, "expected error due to truncated JSON") + }) + + t.Run("zero means no limit", func(t *testing.T) { + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetAuthorizationJSON().Return(genericHttpAuthDataMock()) + + metadata := &GenericHttp{ + Endpoint: endpoint, + Method: "GET", + MaxResponseBytes: 0, + } + + obj, err := metadata.Call(pipelineMock, ctx) + assert.NilError(t, err) + objJSON := obj.(map[string]interface{}) + assert.Equal(t, len(objJSON["key"].(string)), 1000) + }) +} + +func TestGenericHttpMaxResponseBytesPlainText(t *testing.T) { + largeBody := strings.Repeat("A", 500) + + extHttpMetadataServer := httptest.NewHttpServerMock(testHttpMetadataServerHost, map[string]httptest.HttpServerMockResponseFunc{ + "/metadata": httptest.NewHttpServerMockResponseFuncPlain(largeBody), + }) + defer extHttpMetadataServer.Close() + + ctx := context.TODO() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + endpoint := "http://" + testHttpMetadataServerHost + "/metadata" + + t.Run("within limit", func(t *testing.T) { + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetAuthorizationJSON().Return(genericHttpAuthDataMock()) + + metadata := &GenericHttp{ + Endpoint: endpoint, + Method: "GET", + MaxResponseBytes: 1000, + } + + obj, err := metadata.Call(pipelineMock, ctx) + assert.NilError(t, err) + assert.Equal(t, obj, largeBody) + }) + + t.Run("exceeds limit truncates response", func(t *testing.T) { + pipelineMock := mock_auth.NewMockAuthPipeline(ctrl) + pipelineMock.EXPECT().GetAuthorizationJSON().Return(genericHttpAuthDataMock()) + + metadata := &GenericHttp{ + Endpoint: endpoint, + Method: "GET", + MaxResponseBytes: 100, + } + + obj, err := metadata.Call(pipelineMock, ctx) + assert.NilError(t, err) + assert.Equal(t, len(obj.(string)), 100) + assert.Equal(t, obj, strings.Repeat("A", 100)) + }) +} + func genericHttpAuthDataMock() string { type mockIdentityObject struct { User string `json:"user"` diff --git a/pkg/evaluators/metadata/uma.go b/pkg/evaluators/metadata/uma.go index c37f97405..931257200 100644 --- a/pkg/evaluators/metadata/uma.go +++ b/pkg/evaluators/metadata/uma.go @@ -38,19 +38,19 @@ func (provider *Provider) GetTokenURL() string { return provider.tokenURL } -func (provider *Provider) GetResourcesByURI(uri string, pat PAT, ctx gocontext.Context, timeout *int) ([]interface{}, error) { +func (provider *Provider) GetResourcesByURI(uri string, pat PAT, ctx gocontext.Context, timeout *int, maxResponseBytes int64) ([]interface{}, error) { if err := context.CheckContext(ctx); err != nil { return nil, err } - resourceIDs, err := provider.queryResourcesByURI(uri, pat, ctx, timeout) + resourceIDs, err := provider.queryResourcesByURI(uri, pat, ctx, timeout, maxResponseBytes) if err != nil { return nil, err } - return provider.getResourcesByIDs(resourceIDs, pat, ctx, timeout) + return provider.getResourcesByIDs(resourceIDs, pat, ctx, timeout, maxResponseBytes) } -func (provider *Provider) queryResourcesByURI(uri string, pat PAT, ctx gocontext.Context, timeout *int) ([]string, error) { +func (provider *Provider) queryResourcesByURI(uri string, pat PAT, ctx gocontext.Context, timeout *int, maxResponseBytes int64) ([]string, error) { if err := context.CheckContext(ctx); err != nil { return nil, err } @@ -61,14 +61,14 @@ func (provider *Provider) queryResourcesByURI(uri string, pat PAT, ctx gocontext log.FromContext(ctx).V(1).Info("querying resources by uri", "url", queryResourcesURL.String()) var resourceIDs []string - if err := pat.Get(queryResourcesURL.String(), ctx, &resourceIDs, timeout); err != nil { + if err := pat.Get(queryResourcesURL.String(), ctx, &resourceIDs, timeout, maxResponseBytes); err != nil { return nil, err } else { return resourceIDs, nil } } -func (provider *Provider) getResourcesByIDs(resourceIDs []string, pat PAT, ctx gocontext.Context, timeout *int) ([]interface{}, error) { +func (provider *Provider) getResourcesByIDs(resourceIDs []string, pat PAT, ctx gocontext.Context, timeout *int, maxResponseBytes int64) ([]interface{}, error) { if err := context.CheckContext(ctx); err != nil { return nil, err } @@ -82,7 +82,7 @@ func (provider *Provider) getResourcesByIDs(resourceIDs []string, pat PAT, ctx g go func(id string) { defer waitGroup.Done() - if data, err := provider.getResourceByID(id, pat, ctx, timeout); err == nil { + if data, err := provider.getResourceByID(id, pat, ctx, timeout, maxResponseBytes); err == nil { buf <- data } }(resourceID) @@ -98,7 +98,7 @@ func (provider *Provider) getResourcesByIDs(resourceIDs []string, pat PAT, ctx g return resourceData, nil } -func (provider *Provider) getResourceByID(resourceID string, pat PAT, ctx gocontext.Context, timeout *int) (interface{}, error) { +func (provider *Provider) getResourceByID(resourceID string, pat PAT, ctx gocontext.Context, timeout *int, maxResponseBytes int64) (interface{}, error) { if err := context.CheckContext(ctx); err != nil { return nil, err } @@ -109,7 +109,7 @@ func (provider *Provider) getResourceByID(resourceID string, pat PAT, ctx gocont log.FromContext(ctx).V(1).Info("getting resource data", "url", resourceURL.String()) var data interface{} - if err := pat.Get(resourceURL.String(), ctx, &data, timeout); err != nil { + if err := pat.Get(resourceURL.String(), ctx, &data, timeout, maxResponseBytes); err != nil { return nil, err } return data, nil @@ -123,7 +123,7 @@ func (pat *PAT) String() string { return pat.AccessToken } -func (pat *PAT) Get(rawurl string, ctx gocontext.Context, v interface{}, timeout *int) error { +func (pat *PAT) Get(rawurl string, ctx gocontext.Context, v interface{}, timeout *int, maxResponseBytes ...int64) error { if err := context.CheckContext(ctx); err != nil { return err } @@ -138,7 +138,7 @@ func (pat *PAT) Get(rawurl string, ctx gocontext.Context, v interface{}, timeout otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) // get the response - resp, err := httputil.NewClient(timeout).Do(req) + resp, err := httputil.NewClient(httputil.WithTimeout(timeout)).Do(req) if err != nil { return err } @@ -146,14 +146,16 @@ func (pat *PAT) Get(rawurl string, ctx gocontext.Context, v interface{}, timeout _ = body.Close() }(resp.Body) - return json.UnmashalJSONResponse(resp, &v, nil) + return json.UnmashalJSONResponse(resp, &v, nil, maxResponseBytes...) } -func NewUMAMetadata(ctx gocontext.Context, endpoint string, clientID string, clientSecret string) (*UMA, error) { +func NewUMAMetadata(ctx gocontext.Context, endpoint string, clientID string, clientSecret string, timeout *int, maxResponseBytes int64) (*UMA, error) { uma := &UMA{ - Endpoint: endpoint, - ClientID: clientID, - ClientSecret: clientSecret, + Endpoint: endpoint, + ClientID: clientID, + ClientSecret: clientSecret, + Timeout: timeout, + MaxResponseBytes: maxResponseBytes, } if err := uma.discover(ctx); err != nil { return nil, err @@ -163,10 +165,11 @@ func NewUMAMetadata(ctx gocontext.Context, endpoint string, clientID string, cli } type UMA struct { - Endpoint string `yaml:"endpoint,omitempty"` - ClientID string `yaml:"client_id"` - ClientSecret string `yaml:"client_secret"` - Timeout *int + Endpoint string `yaml:"endpoint,omitempty"` + ClientID string `yaml:"client_id"` + ClientSecret string `yaml:"client_secret"` + Timeout *int + MaxResponseBytes int64 provider *Provider } @@ -185,7 +188,7 @@ func (uma *UMA) discover(ctx gocontext.Context) error { otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) - if resp, err := httputil.NewClient(uma.Timeout).Do(req); err != nil { + if resp, err := httputil.NewClient(httputil.WithTimeout(uma.Timeout)).Do(req); err != nil { return fmt.Errorf("failed to fetch uma config: %v", err) } else { defer func(body io.ReadCloser) { @@ -194,7 +197,7 @@ func (uma *UMA) discover(ctx gocontext.Context) error { var p providerJSON var rawClaims []byte - if err = json.UnmashalJSONResponse(resp, &p, &rawClaims); err != nil { + if err = json.UnmashalJSONResponse(resp, &p, &rawClaims, uma.MaxResponseBytes); err != nil { return fmt.Errorf("failed to decode uma provider discovery object: %v", err) } @@ -225,7 +228,7 @@ func (uma *UMA) Call(pipeline auth.AuthPipeline, parentCtx gocontext.Context) (i // get resource data uri := pipeline.GetHttp().GetPath() - resourceData, err := uma.provider.GetResourcesByURI(uri, pat, ctx, uma.Timeout) + resourceData, err := uma.provider.GetResourcesByURI(uri, pat, ctx, uma.Timeout, uma.MaxResponseBytes) if err != nil { return nil, err @@ -262,7 +265,7 @@ func (uma *UMA) requestPAT(ctx gocontext.Context, pat *PAT) error { otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) // get the response - resp, err := httputil.NewClient(uma.Timeout).Do(req) + resp, err := httputil.NewClient(httputil.WithTimeout(uma.Timeout)).Do(req) if err != nil { return err } @@ -271,7 +274,7 @@ func (uma *UMA) requestPAT(ctx gocontext.Context, pat *PAT) error { }(resp.Body) // parse the pat - if err := json.UnmashalJSONResponse(resp, pat, nil); err != nil { + if err := json.UnmashalJSONResponse(resp, pat, nil, uma.MaxResponseBytes); err != nil { return fmt.Errorf("failed to decode uma pat: %v", err) } diff --git a/pkg/evaluators/metadata/uma_test.go b/pkg/evaluators/metadata/uma_test.go index 38878f134..9590e717c 100644 --- a/pkg/evaluators/metadata/uma_test.go +++ b/pkg/evaluators/metadata/uma_test.go @@ -37,7 +37,7 @@ func TestNewUMAMetadata(t *testing.T) { }) defer httpServer.Close() - uma, err := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret") + uma, err := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret", nil, 0) assert.NilError(t, err) assert.Equal(t, umaIssuer, uma.provider.issuer) @@ -49,7 +49,7 @@ func TestUMAMetadataFailToDecodeConfig(t *testing.T) { }) defer httpServer.Close() - uma, err := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret") + uma, err := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret", nil, 0) assert.ErrorContains(t, err, "failed to decode uma provider discovery object") assert.Check(t, uma == nil) @@ -78,7 +78,7 @@ func TestUMACall(t *testing.T) { request := &envoy_auth.AttributeContext_HttpRequest{Path: "/someresource"} pipelineMock.EXPECT().GetHttp().Return(request) - uma, _ := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret") + uma, _ := NewUMAMetadata(context.TODO(), umaIssuer, "client-id", "client-secret", nil, 0) obj, err := uma.Call(pipelineMock, context.TODO()) @@ -86,3 +86,47 @@ func TestUMACall(t *testing.T) { assert.Equal(t, "["+resourceData+"]", string(data)) assert.NilError(t, err) } + +func TestUMAMaxResponseBytesDiscovery(t *testing.T) { + httpServer := httptest.NewHttpServerMock(umaServerHost, map[string]httptest.HttpServerMockResponseFunc{ + "/uma/.well-known/uma2-configuration": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: umaWellKnownConfig} + }, + }) + defer httpServer.Close() + + t.Run("within limit", func(t *testing.T) { + uma := &UMA{ + Endpoint: umaIssuer, + ClientID: "client-id", + ClientSecret: "client-secret", + MaxResponseBytes: int64(len(umaWellKnownConfig) + 100), + } + err := uma.discover(context.TODO()) + assert.NilError(t, err) + assert.Equal(t, umaIssuer, uma.provider.issuer) + }) + + t.Run("exceeds limit causes decode error", func(t *testing.T) { + uma := &UMA{ + Endpoint: umaIssuer, + ClientID: "client-id", + ClientSecret: "client-secret", + MaxResponseBytes: 20, + } + err := uma.discover(context.TODO()) + assert.ErrorContains(t, err, "failed to decode uma provider discovery object") + }) + + t.Run("zero means no limit", func(t *testing.T) { + uma := &UMA{ + Endpoint: umaIssuer, + ClientID: "client-id", + ClientSecret: "client-secret", + MaxResponseBytes: 0, + } + err := uma.discover(context.TODO()) + assert.NilError(t, err) + assert.Equal(t, umaIssuer, uma.provider.issuer) + }) +} diff --git a/pkg/evaluators/metadata/user_info.go b/pkg/evaluators/metadata/user_info.go index 1e2ab7c12..14ba2a1ed 100644 --- a/pkg/evaluators/metadata/user_info.go +++ b/pkg/evaluators/metadata/user_info.go @@ -19,6 +19,7 @@ type UserInfo struct { OpenIdConfig auth.OpenIdConfigStore UserInfoEndpoint string Timeout *int + MaxResponseBytes int64 } func NewUserInfo(openIdConfigStore auth.OpenIdConfigStore, userInfoEndpoint string) *UserInfo { @@ -71,10 +72,10 @@ func (u *UserInfo) Call(pipeline auth.AuthPipeline, parentCtx gocontext.Context) } // fetch user info - return fetchUserInfo(userInfoEndpoint, accessToken, u.Timeout, ctx) + return fetchUserInfo(userInfoEndpoint, accessToken, u.Timeout, u.MaxResponseBytes, ctx) } -func fetchUserInfo(userInfoEndpoint string, accessToken string, timeout *int, ctx gocontext.Context) (interface{}, error) { +func fetchUserInfo(userInfoEndpoint string, accessToken string, timeout *int, maxResponseBytes int64, ctx gocontext.Context) (interface{}, error) { if err := context.CheckContext(ctx); err != nil { return nil, err } @@ -89,7 +90,7 @@ func fetchUserInfo(userInfoEndpoint string, accessToken string, timeout *int, ct otel.GetTextMapPropagator().Inject(ctx, otel_propagation.HeaderCarrier(req.Header)) - resp, err := httputil.NewClient(timeout).Do(req) + resp, err := httputil.NewClient(httputil.WithTimeout(timeout)).Do(req) if err != nil { return nil, err } @@ -97,9 +98,14 @@ func fetchUserInfo(userInfoEndpoint string, accessToken string, timeout *int, ct _ = Body.Close() }(resp.Body) + var bodyReader io.Reader = resp.Body + if maxResponseBytes > 0 { + bodyReader = io.LimitReader(resp.Body, maxResponseBytes) + } + // parse the response var claims map[string]interface{} - err = json.NewDecoder(resp.Body).Decode(&claims) + err = json.NewDecoder(bodyReader).Decode(&claims) if err != nil { return nil, err } diff --git a/pkg/evaluators/metadata/user_info_test.go b/pkg/evaluators/metadata/user_info_test.go index 46d5cce8e..881b7f011 100644 --- a/pkg/evaluators/metadata/user_info_test.go +++ b/pkg/evaluators/metadata/user_info_test.go @@ -16,8 +16,9 @@ import ( ) const ( - authServerHost string = "127.0.0.1:9002" - userInfoClaims string = `{ "sub": "831707be-ef07-4d63-b427-4216309e9897" }` + authServerHost string = "127.0.0.1:9002" + userInfoClaims string = `{ "sub": "831707be-ef07-4d63-b427-4216309e9897" }` + largeUserInfoClaims string = `{ "sub": "831707be-ef07-4d63-b427-4216309e9897", "extra": "` + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" + `" }` ) var wellKnownOIDCConfig string = fmt.Sprintf(`{ @@ -61,6 +62,9 @@ func TestMain(m *testing.M) { "/userinfo": func() httptest.HttpServerMockResponse { return httptest.HttpServerMockResponse{Status: 200, Body: userInfoClaims} }, + "/userinfo-large": func() httptest.HttpServerMockResponse { + return httptest.HttpServerMockResponse{Status: 200, Body: largeUserInfoClaims} + }, }) defer authServer.Close() os.Exit(m.Run()) @@ -114,3 +118,27 @@ func TestUserInfoMissingOIDCConfig(t *testing.T) { _, err := ta.userInfo.Call(ta.pipelineMock, ta.ctx) assert.Error(t, err, "missing openid connect configuration") } + +func TestFetchUserInfoMaxResponseBytes(t *testing.T) { + ctx := context.TODO() + endpoint := fmt.Sprintf("http://%s/userinfo-large", authServerHost) + + t.Run("within limit", func(t *testing.T) { + obj, err := fetchUserInfo(endpoint, "token", nil, int64(len(largeUserInfoClaims)+100), ctx) + assert.NilError(t, err) + claims := obj.(map[string]interface{}) + assert.Equal(t, "831707be-ef07-4d63-b427-4216309e9897", claims["sub"]) + }) + + t.Run("exceeds limit causes decode error", func(t *testing.T) { + _, err := fetchUserInfo(endpoint, "token", nil, 30, ctx) + assert.Assert(t, err != nil) + }) + + t.Run("zero means no limit", func(t *testing.T) { + obj, err := fetchUserInfo(endpoint, "token", nil, 0, ctx) + assert.NilError(t, err) + claims := obj.(map[string]interface{}) + assert.Equal(t, "831707be-ef07-4d63-b427-4216309e9897", claims["sub"]) + }) +} diff --git a/pkg/http/request.go b/pkg/http/request.go index 9cfb171c8..74437ab49 100644 --- a/pkg/http/request.go +++ b/pkg/http/request.go @@ -173,42 +173,84 @@ func (t *tracingRoundTripper) RoundTrip(req *http.Request) (*http.Response, erro return t.base.RoundTrip(req) } -// NewClient creates an HTTP client with the specified timeout. -// If timeoutMs is nil or negative, defaults to 5000ms (5 seconds). -// If timeoutMs is 0, no timeout is set (matching Go's http.Client convention). -// If timeoutMs is positive, uses that value as the timeout in milliseconds. -func NewClient(timeoutMs *int) *http.Client { - timeout := 5000 * time.Millisecond // default +type clientOptions struct { + timeoutMs *int + tracingCtx context.Context + maxResponseBytes int64 +} + +// Option configures an HTTP client created by NewClient. +type Option func(*clientOptions) + +// WithTimeout sets the client timeout in milliseconds. +// If nil or negative, defaults to 5000ms. If 0, no timeout is set. +func WithTimeout(timeoutMs *int) Option { + return func(o *clientOptions) { o.timeoutMs = timeoutMs } +} + +// WithTracing enables OpenTelemetry trace propagation on every outbound request. +func WithTracing(ctx context.Context) Option { + return func(o *clientOptions) { o.tracingCtx = ctx } +} + +// WithMaxResponseBytes limits every response body to the given number of bytes +// at the transport level, preventing unbounded memory consumption. +func WithMaxResponseBytes(n int64) Option { + return func(o *clientOptions) { o.maxResponseBytes = n } +} - if timeoutMs != nil && *timeoutMs >= 0 { - timeout = time.Duration(*timeoutMs) * time.Millisecond +// NewClient creates an HTTP client with the given options. +// Without options the client uses the default 5 000 ms timeout. +func NewClient(opts ...Option) *http.Client { + o := &clientOptions{} + for _, fn := range opts { + fn(o) } - return &http.Client{ - Timeout: timeout, + timeout := 5000 * time.Millisecond + if o.timeoutMs != nil && *o.timeoutMs >= 0 { + timeout = time.Duration(*o.timeoutMs) * time.Millisecond } -} -// NewClientWithTracing creates an HTTP client with the specified timeout and trace propagation. -// The trace context from ctx will be injected into all outbound HTTP requests made by this client. -// This is useful for instrumenting HTTP clients used by third-party libraries that create requests -// internally (e.g., go-oidc for OIDC discovery and JWK fetching). -// -// The ctx parameter is used only for trace propagation, not for request cancellation. -// Callers should use context.Background() or a non-cancellable context for the HTTP request lifecycle. -func NewClientWithTracing(ctx context.Context, timeoutMs *int) *http.Client { - baseClient := NewClient(timeoutMs) + client := &http.Client{Timeout: timeout} - // Wrap the transport with trace injection - baseTransport := baseClient.Transport - if baseTransport == nil { - baseTransport = http.DefaultTransport + if o.tracingCtx != nil || o.maxResponseBytes > 0 { + transport := http.DefaultTransport + if o.tracingCtx != nil { + transport = &tracingRoundTripper{base: transport, ctx: o.tracingCtx} + } + if o.maxResponseBytes > 0 { + transport = &maxResponseBytesRoundTripper{base: transport, maxBytes: o.maxResponseBytes} + } + client.Transport = transport } - baseClient.Transport = &tracingRoundTripper{ - base: baseTransport, - ctx: ctx, + return client +} + +// maxResponseBytesRoundTripper wraps an http.RoundTripper and limits the size of response bodies. +// This prevents unbounded memory consumption when the HTTP client is used by third-party libraries +// whose response body reads cannot be controlled directly. +type maxResponseBytesRoundTripper struct { + base http.RoundTripper + maxBytes int64 +} + +func (rt *maxResponseBytesRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := rt.base.RoundTrip(req) + if err != nil { + return nil, err } + resp.Body = &limitedReadCloser{ + Reader: io.LimitReader(resp.Body, rt.maxBytes), + Closer: resp.Body, + } + return resp, nil +} - return baseClient +// limitedReadCloser combines a limited io.Reader with the original io.Closer, +// so that reads are bounded while the underlying body is still properly closed. +type limitedReadCloser struct { + io.Reader + io.Closer } diff --git a/pkg/http/request_test.go b/pkg/http/request_test.go index f566ac144..44b2ce1e5 100644 --- a/pkg/http/request_test.go +++ b/pkg/http/request_test.go @@ -2,6 +2,7 @@ package http import ( "context" + "io" "net/http" "net/http/httptest" "strings" @@ -332,7 +333,7 @@ func TestNewClient(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - client := NewClient(tt.timeout) + client := NewClient(WithTimeout(tt.timeout)) if client == nil { t.Error("NewClient() returned nil") return @@ -628,7 +629,7 @@ func TestNewRequestWithCredentials(t *testing.T) { } } -func TestNewClientWithTracing(t *testing.T) { +func TestNewClientWithTracingOption(t *testing.T) { // Set up a real tracer and propagator for this test tp := sdktrace.NewTracerProvider() tracer := tp.Tracer("test") @@ -653,7 +654,7 @@ func TestNewClientWithTracing(t *testing.T) { // Create a client with tracing timeout := 5000 - client := NewClientWithTracing(ctx, &timeout) + client := NewClient(WithTimeout(&timeout), WithTracing(ctx)) // Make a request using this client req, err := http.NewRequest("GET", server.URL, nil) @@ -686,7 +687,7 @@ func TestNewClientWithTracing(t *testing.T) { } } -func TestNewClientWithTracing_TimeoutConfiguration(t *testing.T) { +func TestNewClientWithTracingOption_TimeoutConfiguration(t *testing.T) { ctx := context.Background() tests := []struct { @@ -713,7 +714,7 @@ func TestNewClientWithTracing_TimeoutConfiguration(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - client := NewClientWithTracing(ctx, tt.timeoutMs) + client := NewClient(WithTimeout(tt.timeoutMs), WithTracing(ctx)) gotMs := int(client.Timeout.Milliseconds()) if gotMs != tt.wantMs { @@ -732,6 +733,104 @@ func TestNewClientWithTracing_TimeoutConfiguration(t *testing.T) { } } +func TestMaxResponseBytesRoundTripper(t *testing.T) { + t.Run("limits response body", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("this is a long response body that exceeds the limit")) + })) + defer server.Close() + + client := NewClient(WithTracing(context.Background()), WithMaxResponseBytes(10)) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + if err != nil { + t.Fatalf("unexpected error creating request: %v", err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("unexpected read error: %v", err) + } + if len(body) != 10 { + t.Errorf("expected body length 10, got %d", len(body)) + } + if string(body) != "this is a " { + t.Errorf("expected truncated body %q, got %q", "this is a ", string(body)) + } + }) + + t.Run("zero means no limit", func(t *testing.T) { + fullBody := strings.Repeat("x", 10000) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(fullBody)) + })) + defer server.Close() + + client := NewClient(WithTracing(context.Background()), WithMaxResponseBytes(0)) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + if err != nil { + t.Fatalf("unexpected error creating request: %v", err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("unexpected read error: %v", err) + } + if len(body) != 10000 { + t.Errorf("expected full body length 10000, got %d", len(body)) + } + }) + + t.Run("transport wraps tracing", func(t *testing.T) { + client := NewClient(WithTracing(context.Background()), WithMaxResponseBytes(100)) + rt, ok := client.Transport.(*maxResponseBytesRoundTripper) + if !ok { + t.Fatal("expected Transport to be *maxResponseBytesRoundTripper") + } + if _, ok := rt.base.(*tracingRoundTripper); !ok { + t.Error("expected inner transport to be *tracingRoundTripper") + } + }) + + t.Run("limits without tracing", func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("this is a long response body that exceeds the limit")) + })) + defer server.Close() + + client := NewClient(WithMaxResponseBytes(10)) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, server.URL, nil) + if err != nil { + t.Fatalf("unexpected error creating request: %v", err) + } + resp, err := client.Do(req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("unexpected read error: %v", err) + } + if len(body) != 10 { + t.Errorf("expected body length 10, got %d", len(body)) + } + }) +} + func ptr(i int) *int { return &i } diff --git a/pkg/json/json.go b/pkg/json/json.go index 320621a9a..fdc2ecc6f 100644 --- a/pkg/json/json.go +++ b/pkg/json/json.go @@ -66,11 +66,18 @@ func (v *JSONValue) IsTemplate() bool { return len(curlyBracesForModifiersRegex.FindAllStringSubmatch(v.Pattern, -1)) != len(allCurlyBracesRegex.FindAllStringSubmatch(v.Pattern, -1)) } -// UnmashalJSONResponse unmarshalls a generic HTTP response body into a JSON structure -// Pass optionally a pointer to a byte array to get the raw body of the response object written back -func UnmashalJSONResponse(resp *http.Response, v interface{}, b *[]byte) error { +// UnmashalJSONResponse unmarshalls a generic HTTP response body into a JSON structure. +// Pass optionally a pointer to a byte array to get the raw body of the response object written back. +// When maxResponseBytes > 0, the response body reader is limited to that size to prevent +// unbounded memory consumption from large responses. +func UnmashalJSONResponse(resp *http.Response, v interface{}, b *[]byte, maxResponseBytes ...int64) error { + var bodyReader io.Reader = resp.Body + if len(maxResponseBytes) > 0 && maxResponseBytes[0] > 0 { + bodyReader = io.LimitReader(resp.Body, maxResponseBytes[0]) + } + // read response body - body, err := io.ReadAll(resp.Body) + body, err := io.ReadAll(bodyReader) if err != nil { return fmt.Errorf("unable to read response body: %v", err) } diff --git a/pkg/oauth2/client_credentials.go b/pkg/oauth2/client_credentials.go index 21211c1b5..6a4f8523c 100644 --- a/pkg/oauth2/client_credentials.go +++ b/pkg/oauth2/client_credentials.go @@ -47,7 +47,7 @@ func (c *ClientCredentials) ClientCredentialsToken(ctx context.Context, force bo // Inject custom HTTP client with timeout into context // The oauth2 library will use this client for token requests - httpClient := httputil.NewClient(c.Timeout) + httpClient := httputil.NewClient(httputil.WithTimeout(c.Timeout)) ctx = context.WithValue(ctx, gooauth2.HTTPClient, httpClient) token, err := c.Token(ctx)