Feature: allow mixed IP and UDS endpoints in backend route references#8530
Feature: allow mixed IP and UDS endpoints in backend route references#8530stekole wants to merge 6 commits intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
66020cd to
e279442
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8530 +/- ##
==========================================
- Coverage 74.41% 74.38% -0.03%
==========================================
Files 243 243
Lines 38336 38333 -3
==========================================
- Hits 28526 28514 -12
- Misses 7814 7822 +8
- Partials 1996 1997 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
5d23c9f to
32d38cb
Compare
| } | ||
| case egv1a1.KindBackend: | ||
| if err := t.validateBackendRefBackend(backendRef.BackendObjectReference, resources, backendNamespace, false); err != nil { | ||
| if err := t.validateBackendRefBackend(backendRef.BackendObjectReference, resources, backendNamespace, true); err != nil { |
There was a problem hiding this comment.
IIRC, we're find to remove the last one parameter from function validateBackendRefBackend.
There was a problem hiding this comment.
Makes sense - Thanks - Addressed.
6f00c6d to
9d4545d
Compare
|
is it possible to add an e2e test to ensure that a mixed backend worked as expected? |
|
Yes - I can write something to validate the route is accepted (not rejected as "MIXED") and traffic flows through. If we want a full HTTP on UDS and serve traffic that may require additional containers and complexity. I think ensuring the route is created is a good first step. Let me know if you'd like to see something else. We can discuss further or happy to address it in a follow-up. |
Fixes envoyproxy#8229 Signed-off-by: stekole <stefan@sandnetworks.com>
Fixes envoyproxy#8229 Signed-off-by: stekole <stefan@sandnetworks.com>
b76668c to
d648970
Compare
|
/retest |
Signed-off-by: stekole <30674956+stekole@users.noreply.github.com>
| * For security reasons, Envoy Gateway MUST reject references to a `Backend` in xRoute resources. For example, UDS and | ||
| localhost references will not be supported for xRoutes. | ||
| * For security reasons, Envoy Gateway MUST reject localhost references to a `Backend` in xRoute resources. | ||
| Unix domain socket references are supported in xRoutes, but admins must ensure proper access controls. |
There was a problem hiding this comment.
how would admins ensure proper access control if app developers can route back into the proxy ?
There was a problem hiding this comment.
Good question. I may not have all the answers but I will references a few things I know.
rbac on Backend resources - the docs already reference restricting who can create Backend CRs, consistent with guidance on CVE-2021-25740. An app developer without RBAC to create Backend resources can't reference UDS: https://github.com/envoyproxy/gateway/blob/main/site/content/en/latest/tasks/traffic/backend.md#L14-L21
UDS also requires a mounted socket in the proxy pod - UDS path in a Backend spec is a no-op unless the socketfile is actually present in the proxy pods filesystem. Mounting it requires an envoyproxy infrastructure patch, which is an admin resource that app developers shouldnt control.
ref: https://github.com/envoyproxy/gateway/blob/main/site/content/en/contributions/design/backend.md#L129-L131
Task doc: https://github.com/envoyproxy/gateway/blob/main/site/content/en/latest/tasks/traffic/backend.md#L37
There was a problem hiding this comment.
loopback routing also looks to be disabled - https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/backend.go#L198
Signed-off-by: stekole <30674956+stekole@users.noreply.github.com>
|
Checking-in if there are any outstanding concerns? |
|
@guydc can you take a look? |
|
hey @stekole will bring this up in the community meeting tomorrow to help make a decision on this feature |
|
Hi, I've changed the title to feature since it changed the route behavior to allow UDS. Calling this fix is misleading. |
|
For reference, EG also rejects localhost for dynamic resolvers because of similar concerns. |
|
I don't think that there's a significant loopback risk via the UDS endpoint, unless users patch/mutate XDS:
I also don't think that we can create a reasonable deny-list of well-known privileged sockets that should not be targeted (e.g. However, I do have a more practical concern: UDS service are often treated as-if they were "in-process", and often do not implement security measures like TLS and AuthN/Z. The common assumption is that this interface is purely local and has no network exposure. With this change, we will make it possible for app developers that have In the EG context, ext-auth, ext-proc and SDS servers are often integrated using UDS. The exposure of these auxiliary services to direct end-user calls can create availability and confidentiality concerns, such as:
This change might surprise operators, that may not realize that such existing mounted sockets may now be exposed by app developers. The backend resource was created, first and foremost, to support routing to cluster-external targets. It's not uncommon for operators to allow app developers to create Backend resources, while also using additional tools like network policies to restrict access to sensitive in-cluster resources. I prefer that this usage pattern would continue to be supported by default without the possible compromise of UDS services. So, the way I see it, we can implement this feature, but we should create some security measures:
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Thanks @guydc for the analysis. Regarding the allow-list of socket paths, I've been thinking about this and I wonder if the complexity outweighs the benefit at this stage.
The opt-in gate combined with clear documentation about the security implications gives operators the control they need without introducing API surface that I can see being hard to maintain long term. Happy to continue the discussion and adjust as needed. |
Yes, but other measures exist, like network security policies, that can block TCP traffic from reaching unwanted sensitive resources like other microservices in the cluster that are not end-user facing. In essence, I'd like to recreate that level of control also for the UDS case, albeit via EG.
IMHO, an operator that changed EnvoyProxy config to mount the sockets into pod is also well-positioned to decide which of these sockets should go into an allowlist maintained in EnvoyProxy. cc @envoyproxy/gateway-reviewers @envoyproxy/gateway-maintainers please chime in |
sgtm, what shape are you thinking @guydc |
probably something like Alternatively, the allow list could be a list of named Backend resources. But, that could be a bit problematic:
IMHO, an explicit list of paths is the best option, though it's not a very nice UX. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Backend resources with mixed IP and Unix Domain Socket (UDS) endpoints were incorrectly rejected as unsupported mixed address types. Additionally, UDS endpoints were blocked entirely in route backend references.
Which issue(s) this PR fixes:
Fixes #8229
Release Notes: Yes