Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AuthorizationPolicy: add serviceAccounts field #3340

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

AuthorizationPolicy: add serviceAccounts field #3340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5121fa0
AuthorizationPolicy: add `serviceAccounts` field
howardjohn Oct 18, 2024
15e93f5
Tests and validation
howardjohn Oct 28, 2024
6dd0b03
add doc
howardjohn Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions kubernetes/customresourcedefinitions.gen.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

99 changes: 65 additions & 34 deletions security/v1beta1/authorization_policy.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions security/v1beta1/authorization_policy.pb.html
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 13 additions & 0 deletions security/v1beta1/authorization_policy.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@bleggett bleggett Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

//
// If not set, any principal is allowed.
repeated string principals = 1;

Expand All @@ -453,6 +455,17 @@ message Source {
// Optional. A list of negative match of namespaces.
repeated string not_namespaces = 7;

// Optional. A list of service accounts derived from the peer certificate.
// This field requires mTLS enabled and is the same as the `source.serviceaccount` attribute.
//
// This takes the format `<namespace>/<serviceaccount>`.
//
// If not set, any service account is allowed.
bleggett marked this conversation as resolved.
Show resolved Hide resolved
repeated string service_accounts = 11;

// Optional. A list of negative match of service accounts.
bleggett marked this conversation as resolved.
Show resolved Hide resolved
repeated string not_service_accounts = 12;

// Optional. A list of IP blocks, populated from the source address of the IP packet. Single IP (e.g. `203.0.113.4`) and
// CIDR (e.g. `203.0.113.0/24`) are supported. This is the same as the `source.ip` attribute.
//
Expand Down