AuthorizationPolicy: add serviceAccounts field
#3340
+124
−34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a minor implementation complexity in favor of a dramatic simplification to usage of Istio authorization. Today, if a user wants to dive into zero-trust 101, they are presented with a requirement to set `principals`: `A list of peer identities derived from the peer certificate`, and write `<TRUST_DOMAIN>/ns/<NAMESPACE>/sa/<SERVICE_ACCOUNT>`. This simple sentance is a huge cognitive overload for users in my experience working with users, and unnecesarily pushes SPIFFE, trust domains, and other unneccesary concepts onto users. Additionally, the requirement to set 'trust domain', which is overwhelmingly not desired by users who just want SA auth, leads to all sorts of wonky workarounds in Istio like `cluster.local` being a magic value. Instead, we just add a SA field directly. This takes the format `ns/sa`, as you cannot safely reference a SA without a namespace field as well. Note we do this, rather than just require you to set 'service account' and 'namespace' as individual fields, since you could have `namespace=[a,b],sa=[d,e]` which is ambiguous. If this is directionally approved, I will add some more documentation and CEL validation and testing.
howardjohn
added
the
release-notes-none
istio-testing
added
the
size/S
hzxuzhonghu
reviewed
Oct 21, 2024
| @@ -428,6 +428,8 @@ message Source { | |||
| // `"<TRUST_DOMAIN>/ns/<NAMESPACE>/sa/<SERVICE_ACCOUNT>"`, for example, `"cluster.local/ns/default/sa/productpage"`. | |||
| // This field requires mTLS enabled and is the same as the `source.principal` attribute. | |||
| // | |||
| // Usage of `serviceAccounts` is typically simpler and offers the same functionality. | |||
There was a problem hiding this comment.
nope, if this is a client from outside, sa is not known, in this case principals is still needed.
There was a problem hiding this comment.
I don't get it. If I know to write spiffe://cluster.local/ns/foo/sa/bar then surely I can know to write foo/bar?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a minor implementation complexity in favor of a dramatic
simplification to usage of Istio authorization.
Today, if a user wants to dive into zero-trust 101, they are presented
with a requirement to set
principals:A list of peer identities derived from the peer certificate, and write<TRUST_DOMAIN>/ns/<NAMESPACE>/sa/<SERVICE_ACCOUNT>.This simple sentance is a huge cognitive overload for users in my
experience working with users, and unnecesarily pushes SPIFFE, trust
domains, and other unneccesary concepts onto users. Additionally, the
requirement to set 'trust domain', which is overwhelmingly not desired
by users who just want SA auth, leads to all sorts of wonky workarounds
in Istio like
cluster.localbeing a magic value.Instead, we just add a SA field directly. This takes the format
ns/sa,as you cannot safely reference a SA without a namespace field as well.
Note we do this, rather than just require you to set 'service account' and 'namespace'
as individual fields, since you could have
namespace=[a,b],sa=[d,e]which is ambiguous.
If this is directionally approved, I will add some more documentation
and CEL validation and testing.