OCPBUGS-99480: Harden Gateway API docs against route hijacking - #117235
OCPBUGS-99480: Harden Gateway API docs against route hijacking#117235jab-rh wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jab-rh: This pull request references Jira Issue OCPBUGS-99480, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
openshift-ci
Bot
added
the
size/L
Replace from: All in customer-facing Gateway examples with trusted namespace selectors, require listener hostnames, and warn that unrestricted route attachment can enable traffic or hostname hijacking. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@jab-rh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| * You have access to the cluster as a user with the `cluster-admin` role. | ||
| * You have installed the {oc-first}. | ||
| * You have an existing `GatewayClass` resource, such as `openshift-default`. | ||
| * You have an existing `GatewayClass` custom resource, such as `openshift-default`. |
There was a problem hiding this comment.
hum, I think the "custom" word here may be unnecessary, let me explain why: in Kubernetes, a resource means "an instance of a specific type".
So this is similar to say "a Route resource, or a FeatureGate resource" and so one (there are pedantic differences here which I don't think will add anything to my review)
Anyway, IMO we can remove the "custom" word
There was a problem hiding this comment.
@rikatz, interesting; I'd always considered any resource that is implemented via the CRD mechanism to be a CR, and thus custom resource, and this usage is what we've historically had throughout the OCP documentation. So that's why I changed it, even though I could have left it alone; I couldn't help it.
| .Procedure | ||
|
|
||
| . Create a YAML file, such as `hello-gateway.yaml`, that defines your `Gateway` object without the addresses field: | ||
| . Create a YAML file, such as `hello-gateway.yaml`, that defines your `Gateway` object. The `spec.addresses` field is omitted from this configuration to ensure automatic assignment. |
There was a problem hiding this comment.
actually, we don't explicitly support address assignment on Gateway API (we don't run conformance tests for it nowadays) so this later claim of "ensure automatic assignment" is not quite true right now. It can (and will) change in a close future, but today it is what it is
| * `spec.listeners.hostname`: Replace `<cluster_domain>` with your actual cluster ingress domain (for example, `example.com`). | ||
| * `listeners.hostname`: Specifies which hostnames can attach to this listener. Set a hostname so that only routes under that domain can receive traffic. | ||
| * `allowedRoutes.namespaces.from`: Allows route attachment only from namespaces that have the `shared-gateway-access: "true"` label. | ||
| * `spec.gatewayClassName`: The controller that provisions the address and populates the `status.addresses` field. |
There was a problem hiding this comment.
I think the comment here is out of alignment with others. On the others above, you use a verb like "Specifies, Allows" but here you just say "the controller that provisions...". Maybe we should align the format here (and on the spec.listeners.hostname as well).
There was a problem hiding this comment.
maybe a "what and how" phrasing would be better? like you did on metadata.name and listeners.hostname which btw I just figured out you are duplicating the meaning (line 45 and line 46)
There was a problem hiding this comment.
@rikatz, agreed. I was just too exhausted today to properly parallelize it here. I'll have to find some way to apply consistency.
| + | ||
| -- | ||
| The following example demonstrates a `Gateway` CR with two listeners, one for HTTP and one for HTTPS. For detailed descriptions of the listener fields, see xref:gateway-listener-configuration-reference_{context}[]. | ||
| The following example demonstrates a `Gateway` CR with two listeners, one for HTTP and one for HTTPS. For detailed descriptions of the listener fields, see "Gateway listener configuration reference". |
There was a problem hiding this comment.
I don't really know how this works, but is this new reference link right?
There was a problem hiding this comment.
@rikatz, so because reasons we have very specific usage limitations on xref in modules, and this is the most straightforward way of maintaining compliance with those. I can also create a new Additional resources section with just this link there, but the content is further down the page, so it seemed possibly excessive to do this.
| ---- | ||
| -- | ||
|
|
||
| . Apply the `Gateway` CR by running the following command: |
There was a problem hiding this comment.
should you mention additionally somewhere here that with the configuration above, just HTTPRoutes that exists on namespaces with the label env=dev will be allowed?
| * You must include application hostnames and a `backendRef` rule that points to your backend service. | ||
| * If your `HTTPRoute` and `Gateway` CRs are deployed in different namespaces, the `Gateway` CR listener must be configured to allow cross-namespace routes. You must set `allowedRoutes.namespaces.from: All` in the `Gateway` CR. | ||
| * If your `HTTPRoute` and `Gateway` CRs are deployed in different namespaces, the `Gateway` CR listener must allow routes from the `HTTPRoute` namespace. You must configure the `spec.listeners.allowedRoutes.namespaces` field in the `Gateway` CR and specify a selector for trusted namespaces. | ||
| * The listener `hostname` on the parent `Gateway` CR must cover the hostnames in your `HTTPRoute` CR. For example, a listener hostname of `*.gwapi.apps.example.com` accepts an `HTTPRoute` hostname such as `app.gwapi.apps.example.com`, but rejects hostnames outside that domain. |
There was a problem hiding this comment.
this is more a recommendation (should) than an enforcement (must)
fwiw here and on every other place of this document, the hostname definition on the Gateway and on the xRoutes is less about permission and more about routing and union definition.
For example, if a Gateway listener has no hostname definition, but a route does and attaches to that listener (eg.: www.some.tld), every call to www.some.tld will be directed to that route.
If a Gateway defines a hostname (like *.some.tld) and the route doesn't but attach to that Gateway, and no other route defines *.some.tld every traffic from *.some.tld will be directed to that route.
If you need some better definition for this hostname intersection, I recommend taking a look at https://gateway-api.sigs.k8s.io/docs/concepts/hostnames/
|
|
||
| == Benefits | ||
| [id="gateway-api-benefits_{context}"] | ||
| == Gateway API Benefits |
There was a problem hiding this comment.
I have no idea of the process here, but is there a reason to change this section and the section below to add "Gateway API" given this is already a Gateway API page?
There was a problem hiding this comment.
@rikatz, institutional memory. I've always done my sections this way for the OpenShift networking content on the SDN side. Or at least I think I did. I'll have to go back and see what I've done historically.
|
|
||
| `listeners.hostname`:: | ||
| Defines the hostnames that the listener matches for incoming requests. For example, it can match only requests for hostnames ending in `<example_domain.tld>` (such as `www.<example_domain.tld>`). If no hostname is specified, the gateway routes any traffic that can attach to it. | ||
| Defines the hostnames that the listener matches for incoming requests. For example, it can match only requests for hostnames ending in `<example_domain.tld>` (such as `www.<example_domain.tld>` or `*.gwapi.<example_domain.tld>`). Always set a hostname on each listener. If no hostname is specified, any route that can attach to the listener can claim arbitrary hostnames, which can allow hostname or domain hijacking. |
There was a problem hiding this comment.
unless the user has a good reason to not set a hostname, like if they want to define a catch-all route.
Anyway, on managed cloud environments (AWS, GCP, Azure), not setting a hostname on the listener will cause a DNSRecord to not be created (not a problem/bug, but the Gateway will not have an automatic DNS entry created)
4 participants
Replace from: All in customer-facing Gateway examples with trusted namespace selectors, require listener hostnames, and warn that unrestricted route attachment can enable traffic or hostname hijacking.
Version(s): 4.20+ TBD
Issue:
https://redhat.atlassian.net/browse/OCPBUGS-99480
Link to docs preview:
QE review:
Additional information: