AuthorizationPolicy: add serviceAccounts field

[howardjohn]

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.

[hzxuzhonghu]

And you should document only one field can be specified

[hzxuzhonghu]

nope, if this is a client from outside, sa is not known, in this case principals is still needed.

[howardjohn]

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?

[hzxuzhonghu]

If the client is from external, the identity could be any format, nit limited to spiffe://cluster.local/ns/foo/sa/bar

[howardjohn]

In that case the user would not use this field then.

[howardjohn]

It's not deprecating principals,just making the 99.9999% use case easier

[ilrudie]

Maybe something like this would clarify?

Suggested change

	// Usage of `serviceAccounts` is typically simpler and offers the same functionality.
	// Usage of `serviceAccounts` is typically simpler and offers similar functionality. For complex scenarios principals are still fully supported.

[bleggett]

It's not even for "complex" scenarios - hardcoding principals in the bespoke Istio format in our Auth policies is one reason we can't currently support complex scenarios at all (custom SPIFFE IDs, SPIRE etc) - so we should just say that:

Suggested change

	// Usage of `serviceAccounts` is typically simpler and offers the same functionality.
	// Usage of `serviceAccounts` is typically simpler and offers the same security guarantees. Principals are still fully supported, but not recommended, as encoding complete principal strings leads to fragile policies.

[ilrudie]

Direction looks good. This seems inline with ambient's overarching mission to simplify the things which can be simple.

[ilrudie]

And you should document only one field can be specified

Is this decided for sure? It seems to me that mixing and matching is plausible even if likely not recommended and more error prone.

[howardjohn]

Is this decided for sure? It seems to me that mixing and matching is plausible even if likely not recommended and more error prone.

IMO you should be able to set both. The fields are not strictly related... its fine to say I want to allow from 'foo/bar OR spiffe://something-else'

[bleggett]

And you should document only one field can be specified

You should be able to include as many fields as Istio chooses to support in the AuthPolicy, ultimately - if they can be matched against the identity/principal, we will match them.

So this will probably eventually be a list of substrings to match against OR a whole SPIFFE ID.

SA is all we need to start, but this is also heavily related to istio/istio#43105 - effectively we cannot even properly support arbitrary SPIFFE IDs without this, so this is required for better SPIFFE/SPIRE support as well.

All that is really required is to match against substrings - whether Istio happens to be matching against a SPIFFE ID principal in the Istio format internally or not doesn't matter.

IMO you should be able to set both. The fields are not strictly related... its fine to say I want to allow from 'foo/bar OR spiffe://something-else'

as long as this is a strict OR, I agree - this will help encourage people to stop hardcoding SPIFFE IDs in auth policies in the Istio-specific format, which is the only real obstacle we have to supporting customizable SPIFFE IDs.

EDIT: actually wrong - this will still work just fine.

[bleggett]

This LGTM and makes istio/istio#43105 much easier to boot.

It may be necessary to add something like a trust domain later, but I do think it's much better to have an API that looks like

• service_account
• trust_domain
• ...etc

versus

• exactPrincipal

or

• matcherTupleWhichIsAKindOfIdentity (random fixed fields)

[howardjohn]

I've added some tests and validation. I have blocked usage of SA with principals, in the same from, since they are ANDed -- it does not make sense to say "from ns=a AND sa=foo/bar".

[bleggett]

I've added some tests and validation. I have blocked usage of SA with principals, in the same from, since they are ANDed -- it does not make sense to say "from ns=a AND sa=foo/bar".

It occurs to me we have historically supported (in the API validation sense, not the logical sense)

from ns=a AND principal=spiffe://td/ns/b/sa/c

which is, by the same token, also ~always a validation error we should probably check for (but that's OOS for this PR - I agree we should proactively validate the net-new bits like you've done it here)

[hzxuzhonghu]

Thank you