Add API details GEP-3779 - E/W Authorization#3891
Conversation
2aada9b to
a410c68
Compare
| // While the exact workload identifier structure is implementation-specific, | ||
| // implementations are encouraged to follow the convention of | ||
| // `spiffe://<trust_domain>/ns/<namespace>/sa/<serviceaccount>` | ||
| // when representing Kubernetes workload identities. |
There was a problem hiding this comment.
Why would someone use this matcher to refer to Kubernetes identities when the serviceAccounts list is below? Is this intended primarily for cross-cluster use cases?
There was a problem hiding this comment.
I think cross-cluster or external workloads are reasonable examples which might apply.
It's niche at the moment, but I have seen users express desire to be able to assert policy at a sub-serviceAccount granularity. This seems to leave the door open for an implementation to support that use case.
There was a problem hiding this comment.
I'm really torn about this.
Ultimately this GEP boils down to attaching authorization policy to things described at layer 4, and what we're specifically looking at in this clause is the definition of a an authorization principal. Do we really want different stanzas for every possible kind of principal? If we split out ServiceAccounts and SPIFFE IDs, should we also split out JWT IDs? What if a given mesh can support Unix IDs in some way, or, I dunno, PGP keys or the like?
Likewise, having the different stanzas reflect where the implementation is getting the information feels very strange to me – forcing Chihiro to think about the way ServiceAccounts come from the mTLS handshake but oh right the JWT ID happens after that seems kind of surreal.
In a lot of ways, I'd rather just have a single set of principals that uses a URI-style specification. SPIFFE already definies this with the spiffe:// URI scheme, and it's not hard to imagine defining a parallel scheme for ServiceAccounts (k8s-sa:// maybe?). It's true that a given implementation might not support a given scheme, but then it's already true that a given implementation might not support the Identities stanza because it doesn't support SPIFFE IDs...
There was a problem hiding this comment.
JWTs are explicitly higher level, and would probably have their own definition.
I dont think we are actually opinionated on where implementation is getting the info in this API. we just listed a few options.
ServiceAccounts is just a way to make things more convenient for users with less-advanced needs. Istio has it, and I am pretty sure linkerd as well. So think of it as a way for the user to provide the service accounts (constructs they are probably more familiar with) then knowing and specifying the full identity in the required URI format.
For more context, initially I had also proposed Namespace field, but based on previous feedback here, we settled on ServiceAccounts as an easier way for common auth usecases -- see this as well.
Coming up with yet another reserved string prefix for serviceAccounts is more confusing imo, and will probably as well come at a cost of looser validation
There was a problem hiding this comment.
Magic strings are relatively more common in Istio config, but definitely go against the UX Gateway API has established, would strongly prefer to avoid them as a primary UX, instead reserving just for impl-specific edge cases like @ilrudie described.
I like having Service Accounts as a clean interface that masks the details of how each implementation has slightly different SPIFFE URIs, but I do think the separate stanzas are awkward - I think ideally I'd want some sort of union type where identities could hold objects that express different identity types (similar to https://gateway-api.sigs.k8s.io/reference/spec/#httproutefilter), but I'm not sure how feasible that might be...
There was a problem hiding this comment.
chiming in from SPIFFE over here (but if i'm missing context, let me know). I can imagine that Identities can be useful for cross-cluster or more granular expressivity than the ServiceAccounts below.
However, thinking more about it, for the cross-cluster case, I have a problem with how one can establish trust with the foreign trust domain and actually verify the identity properly. In addition to the Identity string, there would need to be (either here or somewhere else) some information about that foreign trust domain. We cannot simply trust that the extracted name is correct but validate against some out-of-band knowledge. Where is that information supposed to come from?
There was a problem hiding this comment.
In addition to the Identity string, there would need to be (either here or somewhere else) some information about that foreign trust domain.
Agreed @maia-iyer - I don't think cross-cluster with separate roots of trust ("mesh federation") is in scope yet, but we may want to consider this user story when thinking about how to express CA root(s) in the Mesh resource @LiorLieberman
12b8a42 to
776e35c
Compare
|
/cc @kflynn @mikemorris |
b6dcc79 to
6ad28ad
Compare
|
/cc @howardjohn |
|
/label v1.4-release/targeting-experimental |
|
@LiorLieberman: The label(s) 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 kubernetes-sigs/prow repository. |
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
044ecb9 to
018a571
Compare
f4ee0c0 to
54163c2
Compare
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
54163c2 to
1017500
Compare
|
@LiorLieberman, I've commented on a few things that I think you can very easily tweak, but I'm going to go ahead and approve so that @mikemorris can LGTM with one more round. Thanks for sticking with it! 🙂 /approve |
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
|
I think this is in a pretty good place for an initial implementation now - we should track the ongoing work in kubernetes-sigs/network-policy-api#297 and kubernetes-sigs/network-policy-api#342 for how we might want to consider modelling more complex port rules (ranges, named ports, etc) but overall this feels fine to get started. /lgtm |
|
/unhold |
|
🎉 Thanks for all the work on this @LiorLieberman and for all the detailed review, particularly from @kflynn and @mikemorris. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kflynn, LiorLieberman, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
For future reference and alignment/discussion between specs, I just wanted to note that ClusterNetworkPolicy is facing a very similar challenge on how to safely layer additional attributes onto coarse allow/deny rules (related to challenges we might have with how to handle adding L7 rules to the spec later, and how to handle those in implementations unaware of them). |
* add api details gep-3779 * fmting * some-fixes * some-fixes * add Namespace wide section and more clarifications * add clarifications and link to google doc for targetRef * more feedback * more feedback * add feedback and agreements from meeting * formatting * add guardrails * organize the GEP * remove l7 and deny language from the gep * flynns feedback * clarify whats being targeted in a namespace-wide policy * Apply suggestions from code review Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * feedback from kubecon * more feedback * added label selector in GEP-713 as a graduation criteria Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Update geps/gep-3779/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Update geps/gep-3779/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * add enforecementLevel to the API and ports clarifications --------- Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
* add api details gep-3779 * fmting * some-fixes * some-fixes * add Namespace wide section and more clarifications * add clarifications and link to google doc for targetRef * more feedback * more feedback * add feedback and agreements from meeting * formatting * add guardrails * organize the GEP * remove l7 and deny language from the gep * flynns feedback * clarify whats being targeted in a namespace-wide policy * Apply suggestions from code review Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * feedback from kubecon * more feedback * added label selector in GEP-713 as a graduation criteria Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Update geps/gep-3779/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * Update geps/gep-3779/index.md Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com> * add enforecementLevel to the API and ports clarifications --------- Co-authored-by: Mike Morris <mikemorris@users.noreply.github.com>
Add API details for GEP-3779.
More comprehensive comparison of targetRef and options for L4 and L7 authz apis is available here in https://docs.google.com/document/d/1CeagBnHDPbzpYAxBmtJqTshxAW8l-aRPwvwgAGbnv2I/edit?tab=t.0
/kind gep
Related #3779
Does this PR introduce a user-facing change?: