envoyproxy / envoyproxy/gateway
SecurityPolicy: Gateway-targeted authorization is attached to L4 listeners without validation
- Dominant language
- Go
- Stars
- 3k
- Forks
- 864
- Avg merge
- 2d 2h
- Merged PRs (30d)
- 140
Description
*Description*:
A `SecurityPolicy` that targets a `Gateway`, a `ListenerSet`, or one of their listeners is never checked against the L4 rules. `validateSecurityPolicyForL4` only runs when the policy targets a `TCPRoute` or a `UDPRoute`. For every other target the whole `authorization` object is attached to the TCP and UDP listeners of that Gateway as-is.
The L4 matcher only reads `principal.clientCIDRs`. Everything else in a rule is thrown away, and that changes what the rule means:
| rule as written | what the L4 listener gets |
|---|---|
| `Allow` if `operation.methods: [GET]` **and** `clientCIDRs: 10.0.0.0/8` | `Allow` from `10.0.0.0/8`. The method check is gone, so the rule allows more than it says. |
| `Allow` if `headers: x-trusted=yes`, no `clientCIDRs` | The rule disappears. Traffic falls through to `defaultAction`. |
The first one is the problem. Dropping a check from an authorization rule opens up access instead of restricting it. The second one changes the result in whichever direction `defaultAction` points.
In both cases the policy reports `Accepted=True` and says nothing about it.
*Repro*:
Apply a Gateway with an HTTP listener and a UDP listener, a `UDPRoute` on the UDP listener, and this policy:
```yaml
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
name: sp-gw-mixed
spec:
targetRefs:
- group: gateway.networking.k8s.io
kind: Gateway
name: gw-mixed
authorization:
defaultAction: Deny
rules:
- action: Allow
name: allow-get-from-corp
operation:
methods: ["GET"]
principal:
clientCIDRs:
- 10.0.0.0/8
- action: Allow
name: allow-by-header
principal:
headers:
- name: x-trusted
values: ["yes"]
```
`egctl x translate --from gateway-api --to xds` gives a `udp_proxy` matcher with one entry. Its only check is `10.0.0.0/8`. The `operation` and the header rule are both missing.
The same thing happens when the policy names the UDP listener directly with `sectionName: udp`.
*Cause*:
- The validator dispatch in `internal/gatewayapi/securitypolicy.go` picks `validateSecurityPolicyForL4` only for `KindTCPRoute` and `KindUDPRoute`. A Gateway target goes to `validateSecurityPolicy`, which allows HTTP-only fields.
- `translateSecurityPolicyForListeners` then copies the whole `ir.Authorization` onto every matching route in `x.TCP` and `x.UDP`.
*Affected*:
- `TCPRoute`. This has been the behaviour since Gateway-targeted authorization for TCP listeners was added.
- `UDPRoute`. Same code path, as of #9833.
Policies that target a route are handled correctly. #9833 also made the L4 validator reject `operation`, which it had been missing.
*What should happen*:
There are three kinds of target, and they do not all want the same answer.
1. The policy names an L4 listener, for example `Gateway` plus `sectionName` on a UDP listener. The user clearly meant that listener, so HTTP-only fields there are simply wrong. Reject the policy, the same as a route target already does.
2. The policy targets a whole Gateway or ListenerSet that has both HTTP and L4 listeners. Do not reject it. Rejecting would take working JWT or OIDC auth away from the HTTP listeners in order to fix the L4 listeners, and it would break existing users whose Gateway-wide policy happens to sit on a Gateway that also has a TCP listener. Gateway-wide policies are meant to cover several protocols. `cors` is already ignored on TCP listeners for the same reason.
3. The policy targets a Gateway whose listeners are all L4. Nothing can ever honour it, so rejecting costs nothing.
One rule covers all three and keeps today's behaviour for route targets:
> Reject the policy if it cannot take effect on any listener it targets. Otherwise apply it where it can work, drop the rules that do not fully fit on the L4 listeners, and say so in the status.
Dropping the whole rule instead of just the parts that do not fit is what removes the over-broad access. An over-broad rule becomes no rule, which under a `Deny` default fails closed.
*Status reporting*:
Put a short note in the `Accepted` condition message. A separate `Warning` condition is more than this needs, since the policy really is accepted for the listeners it can serve.
Two things to know before writing it:
- `SetAcceptedForPolicyAncestor` in `internal/gatewayapi/status/policy.go` hardcodes `"Policy has been accepted."` and takes no message argument. It also returns early if an `Accepted` condition is already set for that ancestor, so the message has to be written when `Accepted` is first set, not appended afterwards. Either add a variant that takes a message, or call `SetConditionForPolicyAncestor` directly.
- A policy that targets a whole Gateway gets a single ancestorRef with no `sectionName`. There is no per-listener ancestor to attach a per-listener message to, so the message has to name the skipped listeners itself.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in internal/gatewayapi/securitypolicy.go and trace validateSecurityPolicyForL4 and translateSecurityPolicyForListeners, then inspect status handling in internal/gatewayapi/status/policy.go. Reproduce the behavior with the provided Gateway, UDPRoute, and SecurityPolicy using egctl x translate --from gateway-api --to xds. Done means incompatible L4 rules are dropped or the policy is rejected when no target listener can apply it, with the Accepted message explaining skipped listeners.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- api, authorization, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100