Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing validation of value field in HTTPHeader #3669

Open
kwaszczuk opened this issue Mar 9, 2025 · 3 comments
Open

Missing validation of value field in HTTPHeader #3669

kwaszczuk opened this issue Mar 9, 2025 · 3 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@kwaszczuk
Copy link

kwaszczuk commented Mar 9, 2025

What happened:
Even though the HTTPHeader API reference explicitly states: "HTTPHeader represents an HTTP Header name and value as defined by RFC 7230.", the Gateway API does not validate HTTPHeader.value field at all. This allows header values to be defined with invalid characters according to RFC 7230, such as newlines (\n) and carriage returns (\r).

This can have dramatic effects, breaking entire routing configurations in some implementations, e.g. Istio (see the example below), where including invalid header values causes the entire routing provisioning to fail.

What you expected to happen:
The Gateway API should validate HTTPHeader.value field according to RFC 7230 specification.

How to reproduce it (as minimally and precisely as possible):

  1. Create a Gateway resource my-gateway using Istio implementation:
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: my-gateway
  namespace: default
spec:
  gatewayClassName: istio
  listeners:
  - allowedRoutes:
      namespaces:
        from: Same
    name: http
    port: 80
    protocol: HTTP
  1. Create a HTTPRoute resource with a ResponseHeaderModifier with a header value containing newline or carriage return characters
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: header-test
spec:
  parentRefs:
  - name: my-gateway
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /test
    filters:
    - type: ResponseHeaderModifier
      responseHeaderModifier:
        add:
        - name: X-Test-Header
          value: "this\nis\rinvalid\r\nvalue"
  1. Apply the configuration to a cluster
  2. Observe that the configuration is accepted without validation errors, resulting in Istio's RDS (route discovery service) being completely out of order:
$ kubectl logs -n istio-system -l app=istiod --tail=100000000 -c discovery | grep "ERROR"
2025-03-09T22:56:44.871781Z     warn    delta   ADS:RDS: ACK ERROR my-gateway-istio-59fd8d6848-w9h5s.default-2141 Internal:Proto constraint validation failed (RouteConfigurationValidationError.VirtualHosts[0]: embedded message failed validation | caused by VirtualHostValidationError.Routes[0]: embedded message failed validation | caused by RouteValidationError.ResponseHeadersToAdd[0]: embedded message failed validation | caused by HeaderValueOptionValidationError.Header: embedded message failed validation | caused by HeaderValueValidationError.Value: value does not match regex pattern "^[^\x00\n\r]*$"): name: "http.80"
$ istioctl proxy-status
NAME                                          CLUSTER        CDS              LDS              EDS              RDS             ECDS        ISTIOD                      VERSION
my-gateway-istio-59fd8d6848-w9h5s.default     Kubernetes     SYNCED (42s)     SYNCED (42s)     SYNCED (44s)     ERROR (42s)     IGNORED     istiod-688bc8bb9f-dgr4t     1.25.0

Anything else we need to know?:
I believe this should be straightforward to address, so I am willing to contribute a fix.

@kwaszczuk kwaszczuk added the kind/bug Categorizes issue or PR as related to a bug. label Mar 9, 2025
@shaneutt shaneutt added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Mar 10, 2025
@shaneutt shaneutt moved this to Triage in Gateway API Pipeline Mar 10, 2025
@youngnick
Copy link
Contributor

Yes, agreed, this is a bug, we should have validation to ensure that values match what's specified.

However, the tricky part is that this is technically a breaking change, since we are tightening validation (by including some). Personally, I think this is okay in this instance because, as you say, there's no real way to use this for good - it's almost guaranteed to be broken.

@robscott @shaneutt @mlavacca thoughts?

@robscott
Copy link
Member

Thanks for catching this @kwaszczuk! I agree that we should fix this, but let's start with validation that's limited to experimental channel, and then bring it to standard channel in a follow up release as this is a breaking change like @youngnick said.

@kflynn
Copy link
Contributor

kflynn commented Mar 11, 2025

+1 to experimental-first...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Status: Triage
Development

No branches or pull requests

5 participants