Adding GEP-709: Cross namespace references from Routes #711

robscott · 2021-07-10T00:55:26Z

What type of PR is this?
/kind gep

What this PR does / why we need it:
This adds GEP #709 as the culmination of discussion around ReferencePolicy, cross namespace forwarding, and route inclusion.

Does this PR introduce a user-facing change?:

NONE

/cc @youngnick @stevesloka @hbagdi @jpeach @danehans

k8s-ci-robot · 2021-07-10T00:55:30Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: stevesloka.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind gep

What this PR does / why we need it:
This adds GEP #709 as the culmination of discussion around ReferencePolicy, cross namespace forwarding, and route inclusion.

Does this PR introduce a user-facing change?:
NONE
/cc @youngnick @stevesloka @hbagdi @jpeach @danehans

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 kubernetes/test-infra repository.

k8s-ci-robot · 2021-07-10T00:55:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

robscott · 2021-07-12T20:35:25Z

https://deploy-preview-711--kubernetes-sigs-gateway-api.netlify.app/geps/gep-709/

site-src/geps/gep-709.md

hbagdi · 2021-07-12T20:44:55Z

site-src/geps/gep-709.md

+  name: bar
+  namespace: bar
+spec:
+  from:


I recommend adding a spec.rules[], with each rule having a from and to.
That allows for defining multiple policies in a single resource.

I think due to the additive nature of this API I'd rather have multiple resources than one large resource, but open to ideas. I'm concerned that a list of lists would become hard to manage.

I think it's better to encourage people to have more, smaller objects rather than less, larger objects as well.

I note that the from and two stanzas are slices here though, but we don't have any discussion on how they should be combined. I'll add more here on the structs themselves.

site-src/geps/gep-709.md

hbagdi · 2021-07-12T21:04:17Z

site-src/geps/gep-709.md

+
+* Conceptually similar to NetworkPolicy.
+* A separate resource enables admins to restrict who can allow cross namespace
+  references.


No action required.
Do we need any specific commentary on our Security model that might be useful in future when we look back on this GEP? I don't think so. Food for thought for you and others.

If we do update anything, it's probably only to point out something like "the purpose of this is to allow the person who controls the referent object the requirement to accept that reference, explicitly, with the intent of making cross-namespace references more secure by default."

howardjohn · 2021-07-12T21:54:22Z

site-src/geps/gep-709.md

+}
+
+// ReferencePolicyTo describes trusted kinds.
+type ReferencePolicyTo struct {


Having a policy per-service will likely be a bottleneck in some scenarios. For example, if we have an egress proxy, and have an external-namespace. I probably just want to say from: egress-gateway, to: any service in external-namespace. I am not sure that is re-presentable here, without 1 ReferencePolicy per service?

Similarly would be from: anywhere

Both to and from structs don't have names. This just represents trust from resources of type a in namespace foo to resources of type b within the same namespace as the ReferencePolicy. So in this case I think the to: any service in external-namespace is quite straightforward.

On the other hand from: anywhere is intentionally not allowed. I'm not entirely convinced it's a use case we want to encourage, but if it were, I think using a namespace selector instead of a string would be the appropriate way to do it. That does feel like a potential extension point, but I'm pretty hesitant to start with it unless there are compelling use cases.

Yeah, I think one of the key parts about this proposal is that it allows access from a Group/Kind and namespace to a Group/Kind and namespace (where the to namespace is always "the namespace the object is in".) I think that if we want to make it more flexible than "named namespace", it should be a label selector across namespaces only.

As it stands, we can do "Allow from a group of things in a list of namespaces" using these structs, since from is a list.

howardjohn · 2021-07-12T21:55:48Z

site-src/geps/gep-709.md

+### Benefits
+
+* Conceptually similar to NetworkPolicy.
+* A separate resource enables admins to restrict who can allow cross namespace


Can you give an example of when this would actually be useful? I am having a hard time thinking of a scenario where I would want to disallow a namespace from exposing themselves to other namespaces

I think this is conceptually similar to organizations who want to restrict the namespaces that can create Services of type LB and use quotas to achieve that. It's possible that you want users to be able to create Services without allowing them to expose them externally.

The use case for not allowing things to expose themselves to other namespaces is any cluster where a namespace admin for one namespace is not totally trusted. In that case, allowing inbound references is a critical step in privilege escalation (another reason to only allow this interaction in one direction).

For example, I've worked before on CI clusters, where each namespace ran a step in a CI pipeline. Now, CI pipelines are essentaily remote-code-execution-as-a-service, so those workloads are very untrusted. Would someone here be able to create a Service record? Possibly, and if so, it's almost certainly good to ensure that they cannot create cross-namespace references to their namespace.

This is a little contrived, I know, but I think the principle of "relatively untrusted namespace owner" is a pretty sound usecase for this overall.

Hmm, I am not sure I understand. Can you let me know where I am going wrong? This is my understanding:

Lets say we have a namespace ingress, which contains a Gateway. We have a namespace untrusted, which has a Service.

I think the statement here is we are trying to avoid letting untrusted expose their service through ingress' LoadBalancer. We are NOT trying to restrict untrusted from creating an HTTPRoute though (that is independent of this and driven by Gateway/Route selection agreement); rather, we are trying to restrict ingress from creating an HTTPRoute referencing untrusted.

So by restricting untrusted from creating a ReferencePolicy, we think we have achieved this. In reality, the ingress namespace can just point to something in its own namespace that forwards to untrusted (there are many ways to do this, for simplicity lets just say they set up a dumb TCP proxy that forwards all requests to untrusted). As a result, I would say that the use of restrictive RBAC policy on ReferencePolicy does not achieve the goal of not allowing a Service to expose itself.

The point of the ReferencePolicy is to require the owner of the untrusted namespace to accept incoming references. If they can't create ReferencePolicy objects, then they can't accept incoming references, and so say, the admin can create a more-open HTTPRoute that may reference a lot of Services, but only allow some to complete the two-way handshake.

I agree that it's a bit convoluted, but the idea here is to put the control over incoming references into the hands of the person who wons the referent. The main advantage of doing this with a separate object is that it allows us to apply this pattern to things whose spec we can't change (like Service), while still keeping that control in the hands of the object's owner. I think that the RBAC advantage of having a separate object is a minor one at best compared to that, but it may prove useful to people who are extremely careful with their RBAC policies.

I probably overemphasised this usecase previously, sorry about that.

The point of the ReferencePolicy is to require the owner of the untrusted namespace to accept incoming references. If they can't create ReferencePolicy objects, then they can't accept incoming references, and so say, the admin can create a more-open HTTPRoute that may reference a lot of Services, but only allow some to complete the two-way handshake.

I think I understand what you are discussing now, but I don't think its a good idea. The fact its possible to do this with the current design is no problem - obviously we cannot stop users from doing obscure things, but I wouldn't want to recommend this.

Instead of the admin "handshaking" every namespace, but then restricting their ability to handshake back, it seems like they should just not "handshake" in the first place?

Might be getting a bit side tracked here though 🙂

Agreed. I think you make a fair point, that it's a pretty unlikely scenario that we already provide betters tools to deal with. But I think that having the ability to RBAC the ReferencePolicy object separately is an advantage, just not that big of one.

howardjohn · 2021-07-12T21:57:52Z

site-src/geps/gep-709.md

+  cross-namespace references.
+* The implementation clearly documents that ReferencePolicy is not honored.
+
+This exception is very unlikely to apply to any ingress implementations of the


Generally agree, but this could be plausible in a single-tenant ingress. For example ingress namespace has all Gateways and Routes for the ingress. Some routes reference other namespace. There is no security risk here.

How is there no security risk in that scenario? Would it be possible the implementation to know that it was always going to be deployed in that way?

After discussing this a bit more, I think there are some instances where ingress implementations deployed in this way could potentially deserve an exception. Fundamentally the distinction is if the implementation would be subject to some other cross-namespace restrictions such as NetworkPolicy. I'll update this section to be a bit more clear.

I suggested a wording update here, I think that if we make that change, it covers this usecase already.

Completely reworded this section, PTAL.

youngnick

I really like this API, and think it's a great evolution of the earlier work.

youngnick · 2021-07-13T06:32:25Z

site-src/geps/gep-709.md

+  name: bar
+  namespace: bar
+spec:
+  from:


I think it's better to encourage people to have more, smaller objects rather than less, larger objects as well.

I note that the from and two stanzas are slices here though, but we don't have any discussion on how they should be combined. I'll add more here on the structs themselves.

site-src/geps/gep-709.md

youngnick · 2021-07-13T06:38:50Z

site-src/geps/gep-709.md

+}
+
+// ReferencePolicyTo describes trusted kinds.
+type ReferencePolicyTo struct {


Yeah, I think one of the key parts about this proposal is that it allows access from a Group/Kind and namespace to a Group/Kind and namespace (where the to namespace is always "the namespace the object is in".) I think that if we want to make it more flexible than "named namespace", it should be a label selector across namespaces only.

As it stands, we can do "Allow from a group of things in a list of namespaces" using these structs, since from is a list.

youngnick · 2021-07-13T06:41:14Z

site-src/geps/gep-709.md

+    //
+    // +kubebuilder:validation:MinLength=1
+    // +kubebuilder:validation:MaxLength=253
+    Group string `json:"group"`


Because all three of these values are required, each from reference must fully specify a group, kind, and namespace. This is nice for implementers because it's very straightforward, but will this be flexible enough, or should we consider label selectors here?

The downside of including label selectors is that you then need two sets of fields that are both optional, so the zero values of the fields and slices of this struct really matter. This way is much simpler.

I've been tempted to introduce selectors here but so far have avoided it due to the increased complexity they add. In v1alpha1 I think we have had a few too many selectors and it became difficult to keep track of everything. Direct references can still be confusing, but I think they're generally easier to understand. One of my primary concerns with selectors is that their default is so open. Interpreted literally, an empty selector would accept references from all namespaces.

So all of that to say that I agree with you. If we really need selectors, we can always consider adding them in the future.

youngnick · 2021-07-13T06:45:12Z

site-src/geps/gep-709.md

+### Benefits
+
+* Conceptually similar to NetworkPolicy.
+* A separate resource enables admins to restrict who can allow cross namespace


The use case for not allowing things to expose themselves to other namespaces is any cluster where a namespace admin for one namespace is not totally trusted. In that case, allowing inbound references is a critical step in privilege escalation (another reason to only allow this interaction in one direction).

For example, I've worked before on CI clusters, where each namespace ran a step in a CI pipeline. Now, CI pipelines are essentaily remote-code-execution-as-a-service, so those workloads are very untrusted. Would someone here be able to create a Service record? Possibly, and if so, it's almost certainly good to ensure that they cannot create cross-namespace references to their namespace.

This is a little contrived, I know, but I think the principle of "relatively untrusted namespace owner" is a pretty sound usecase for this overall.

youngnick · 2021-07-13T06:46:36Z

site-src/geps/gep-709.md

+
+* Conceptually similar to NetworkPolicy.
+* A separate resource enables admins to restrict who can allow cross namespace
+  references.


If we do update anything, it's probably only to point out something like "the purpose of this is to allow the person who controls the referent object the requirement to accept that reference, explicitly, with the intent of making cross-namespace references more secure by default."

youngnick · 2021-07-13T06:50:01Z

site-src/geps/gep-709.md

+ReferencePolicy. This should only be done if:
+* Other mechanisms like NetworkPolicy can be used to effectively limit
+  cross-namespace references.
+* The implementation clearly documents that ReferencePolicy is not honored.


Small change to make this clearer:

Suggested change

ReferencePolicy. This should only be done if:

* Other mechanisms like NetworkPolicy can be used to effectively limit

cross-namespace references.

* The implementation clearly documents that ReferencePolicy is not honored.

ReferencePolicy. This may only be done if:

* Other mechanisms like NetworkPolicy are used to effectively limit

cross-namespace references.

* The implementation must clearly document that ReferencePolicy is not honored.

I think that "may" is okay here because we are following it up with clarifying "must" clauses straight afterwards.

MAY, MUST, SHOULD should be capitalized so it's obvious we are using them in LOUD RFC TALK.

Yes, that's a great way to do it.

Now that the CVE is live, I've reworked this entire section, including "LOUD RFC TALK" :), PTAL.

youngnick · 2021-07-13T06:50:35Z

site-src/geps/gep-709.md

+  cross-namespace references.
+* The implementation clearly documents that ReferencePolicy is not honored.
+
+This exception is very unlikely to apply to any ingress implementations of the


I suggested a wording update here, I think that if we make that change, it covers this usecase already.

bowei · 2021-07-14T23:35:00Z

site-src/geps/gep-709.md

+
+## TLDR
+
+This GEP attempts to tackle both cross namespace forwarding and route inclusion.


It doesn't seem like we are actually defining route inclusion in this GEP.

We should probably say that this is: prepare the API for route inclusion.

BTW if there are references, it would be good to include to define what route inclusion means.

Unfortunately we only have feature request issues that can be solved with Route inclusion (#634, #695) and a couple design docs. For now I've just clarified this sentence and added a brief description of Route inclusion.

bowei · 2021-07-14T23:47:06Z

site-src/geps/gep-709.md

+      - name: bar
+        namespace: bar
+---
+kind: ReferencePolicy


We might want to call this "InboundReferencePolicy" or something make the direction obvious

That's a good idea. Any thoughts on this name change @youngnick @hbagdi @jpeach @howardjohn?

I'm okay with it, although it's a little long. I had discounted it before because I don't see us adding an OutboundReferencePolicy at any point, so I didn't think the disambiguation was necessary. But I don't object if others think it makes things clearer, I'm a little deep into this design to be able to have a good idea of how the name looks at first glance.

I think ReferencePolicy is fine. (per discussion)

site-src/geps/gep-709.md

bowei · 2021-07-15T00:07:27Z

site-src/geps/gep-709.md

+* Issue URL: https://github.com/kubernetes-sigs/gateway-api/issues/709
+* Status: Implementable
+
+## TLDR


Do we say very explicitly what fields and kinds are intended to be covered as part of "core" behavior in this proposal?

Also do we say that implementations may cover more Kinds and fields, but that is Custom behavior and there is no conformance there. (is this a good idea?)

I've added this to the proposed spec. I think non-core kinds would fall under extended conformance since the expected behavior is clear, it's just a difference of which types we expect all implementations to support.

robscott · 2021-07-15T01:37:36Z

Thanks for all the great feedback on this! I think I've responded to all the feedback now, let me know if I missed anything. I just pushed a round of updates that should get this closer. It also includes a reference to the relevant CVE now: kubernetes/kubernetes#103675.

bowei · 2021-07-15T03:20:17Z

site-src/geps/gep-709.md

+
+## ReferencePolicy
+
+Anytime we allow crossing a namespace boundary, we need to be very cautious. To


[very cautious](cite CVE)

youngnick

/lgtm

I'll leave it up to others to make the call as to if InboundReferencePolicy or ReferencePolicy is clearer. I don't have a strong enough opinion to have it affect my call. :)

youngnick · 2021-07-15T04:28:23Z

uh, I didn't think my lgtm would be enough to merge this, oops. Sorry.

bowei · 2021-07-15T04:30:57Z

doh - rob you forgot to hold it

robscott · 2021-07-15T04:32:50Z

No worries, that's on me, meant to add a hold on these, will open another one with next round of changes.

youngnick · 2021-07-15T04:33:23Z

I think we were just down to the name of the object anyway, pretty much.

robscott · 2021-07-15T04:34:05Z

Yeah agreed, think this one was pretty close

robscott added the kind/gep PRs related to Gateway Enhancement Proposal(GEP) label Jul 10, 2021

k8s-ci-robot requested review from hbagdi, jpeach and danehans July 10, 2021 00:55

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 10, 2021

k8s-ci-robot requested a review from youngnick July 10, 2021 00:55

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 10, 2021

robscott force-pushed the referencepolicy-gep branch from b8f0e24 to 044535b Compare July 10, 2021 00:59

mark-church mentioned this pull request Jul 10, 2021

Canary deployments with Gateway API between namespaces #695

Closed

hbagdi reviewed Jul 12, 2021

View reviewed changes

Adding GEP-709: Cross namespace references from Routes

8d7c454

robscott force-pushed the referencepolicy-gep branch from 044535b to 8d7c454 Compare July 12, 2021 21:40

howardjohn reviewed Jul 12, 2021

View reviewed changes

youngnick reviewed Jul 13, 2021

View reviewed changes

bowei reviewed Jul 14, 2021

View reviewed changes

site-src/geps/gep-709.md Outdated Show resolved Hide resolved

bowei reviewed Jul 14, 2021

View reviewed changes

site-src/geps/gep-709.md Show resolved Hide resolved

bowei reviewed Jul 14, 2021

View reviewed changes

site-src/geps/gep-709.md Outdated Show resolved Hide resolved

bowei reviewed Jul 15, 2021

View reviewed changes

site-src/geps/gep-709.md Outdated Show resolved Hide resolved

bowei reviewed Jul 15, 2021

View reviewed changes

site-src/geps/gep-709.md Outdated Show resolved Hide resolved

bowei reviewed Jul 15, 2021

View reviewed changes

First round of revisions

7c0a21b

bowei reviewed Jul 15, 2021

View reviewed changes

youngnick reviewed Jul 15, 2021

View reviewed changes

k8s-ci-robot assigned youngnick Jul 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2021

k8s-ci-robot merged commit 8791c4f into kubernetes-sigs:master Jul 15, 2021

robscott mentioned this pull request Jul 15, 2021

Tweaks to ReferencePolicy GEP #722

Merged

robscott deleted the referencepolicy-gep branch January 8, 2022 01:05


		## TLDR

		This GEP attempts to tackle both cross namespace forwarding and route inclusion.


		## ReferencePolicy

		Anytime we allow crossing a namespace boundary, we need to be very cautious. To

Adding GEP-709: Cross namespace references from Routes #711

Adding GEP-709: Cross namespace references from Routes #711

Conversation

robscott commented Jul 10, 2021

k8s-ci-robot commented Jul 10, 2021

k8s-ci-robot commented Jul 10, 2021

robscott commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Jul 15, 2021

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

youngnick commented Jul 15, 2021

bowei commented Jul 15, 2021

robscott commented Jul 15, 2021 • edited Loading

youngnick commented Jul 15, 2021

robscott commented Jul 15, 2021

bowei Jul 20, 2021 •

edited

Loading

robscott commented Jul 15, 2021 •

edited

Loading