Add one/many path templating support for AuthPolicy #50365

jaellio · 2024-04-10T21:47:57Z

Please provide a description of this PR:

Updates paths and notPaths in the AuthorizationPolicy resource to support a limited set of templating based on envoy's uri templating.

#16585

Adds support for:

{*}
{**}

If a path contains {*} or {**} it will be interpreted as a uri template rather than a string match. {*} will be converted to * and {**} will be convert to ** when applied in the envoy config.

Open questions:

If a path contains an invalid template (ie. {}, {*, etc) should we return an error or interpret the path as a string match?
If a path contains * or ** (invalid chars in uris, but valid for paths and notPaths) and supported templates ({*} and {**}), should we return an error, interpret the path as a string match, or interpret the path as a uri template (which might result in unintended behavior since * and ** will be interpreted as uri templates)?

Todo:

Write integration tests
Update analyzer support (if applicable)
Add path validation

istio-testing · 2024-04-10T21:48:00Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2024-04-10T21:48:01Z

😊 Welcome @jaellio! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

jaellio · 2024-04-10T21:48:16Z

/test all

jaellio · 2024-04-10T23:22:12Z

/test all

jaellio · 2024-04-11T00:10:43Z

pilot/pkg/security/authz/model/generator.go

@@ -399,6 +399,17 @@ func (g pathGenerator) permission(key, value string, forTCP bool) (*rbacpb.Permi
 		return nil, fmt.Errorf("%q is HTTP only", key)
 	}

+	// TODO(jaellio): Should we interpret the path as a PathMatcher if it is an invalid or unsupported by Istio PathTemplate?


@michaelbannister what are your initial thoughts on this? For my initial draft I didn't add any logic to disallow invalid or unsupported templating. Wanted to get this out for review ASAP.

I don't know – is there a general philosophy that Istio applies regarding invalid config? As a user I'd rather be told early that my authorization policy is broken or will never match a path, than spend ages debugging it at runtime, so I'd be in favour of fairly strict validation and failing with a meaningful error, rather than generating a rule which won't match anything.

But would such an error get surfaced to the client through a validation webhook? I suspect it may depend on how istio's been set up…

We should block in validation, and make this an error probably

The hard part about error is that previously allowed policies could now be blocked. That may be acceptable though due to a low likelihood?

I agree with Keith, I was concerned about disallowing previously allowed paths/policies. However, those paths might technically have been invalid so this might be an acceptable regression.

Looks like validation was pushed up @jaellio?

I have added basic validation to ensure that if a path contains a support path template ({*} or {**}) it must also not contain * or ** or any named variable ({path}, {path=*}, or {path=**}).

I don't know – is there a general philosophy that Istio applies regarding invalid config? As a user I'd rather be told early that my authorization policy is broken or will never match a path, than spend ages debugging it at runtime, so I'd be in favour of fairly strict validation and failing with a meaningful error, rather than generating a rule which won't match anything.

But would such an error get surfaced to the client through a validation webhook? I suspect it may depend on how istio's been set up…

Agree - invalid should be reflected in resource status at least. As we move to CEL validation - not sure if all validations will be possible in CEL and it may be best to use status consistently.

Since this is a very envoy-specific feature - may be worth adopting the same doc and validations that envoy makes. And to confirm - is the syntax in this PR identical to the syntax used by Envoy, or did we make any subtle changes ?

jaellio · 2024-04-11T19:37:44Z

Marking as ready for review. Would appreciate feedback on the general approach. Hold on merging until I have added integration tests.

tests/integration/security/authz_test.go

michaelbannister · 2024-04-15T10:15:53Z

pilot/pkg/security/authz/model/generator.go

+	// Ex: "/foo/{*" is not a valid PathTemplate.
+	// Ex: "/foo/{bar}" is an unsupported PathTemplate.
+	//
+	// Should we disallow the use of `*` and `**` in the path if it is a PathTemplate (the path contains `{*}` or `{**}`, and `*` or `**`)?


I'd be inclined towards disallowing it. I think the chances of someone trying to match a path that genuinely has a * in it, and using PathTemplate matchers as well, are vanishingly small, and if they do exist they will raise a feature request to have the validation relaxed.

@michaelbannister added logic to disallow the use of * and ** with path templating. Did not add the logic to check for invalid path templates (i.e. /foo/{*).

howardjohn

Overall looks good. I think we need validation (pkg/config/validation/validation.go) and integration tests

howardjohn · 2024-04-15T21:00:45Z

pilot/pkg/security/authz/model/generator.go

@@ -399,6 +399,17 @@ func (g pathGenerator) permission(key, value string, forTCP bool) (*rbacpb.Permi
 		return nil, fmt.Errorf("%q is HTTP only", key)
 	}

+	// TODO(jaellio): Should we interpret the path as a PathMatcher if it is an invalid or unsupported by Istio PathTemplate?


We should block in validation, and make this an error probably

keithmattix · 2024-04-16T21:15:20Z

/test integ-basic-arm64_istio

jaellio · 2024-04-16T23:55:10Z

/test unit-tests-arm64

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

a to b requests - Adds a name to typed extension config for uri template Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Added additional path template validation. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

multiple matches Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

keithmattix

LGTM aside from some nits

pilot/pkg/security/authz/builder/testdata/http/allow-full-rule-in.yaml

pkg/config/security/security.go

releasenotes/notes/16585.yaml

pkg/config/security/security_test.go

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

howardjohn · 2024-04-18T20:37:48Z

tests/integration/security/authz_test.go

+						},
+						// Test matches for `/store/{**}/cart`
+						{
+							path:  "/store//cart",


this one is interesting because with https://istio.io/latest/docs/reference/config/security/normalization/#3-multiple-forward-slashes---etc this would not match

Yep; we should update that page or the docs for this field/feature with a warning

@howardjohn Created a PR to clarify behavior of MERG_SLASHES with {**} https://github.com/istio/istio.io/pull/14954/files

howardjohn · 2024-04-18T20:38:22Z

tests/integration/security/testdata/authz/path-templating.yaml.tmpl

+  rules:
+    - to:
+        - operation:
+            paths: ["/allow/admin/{**}", "/foo/{*}/bar/{**}.txt", "/store/{**}/cart"]


can we test an inline {*}? like /abc{*}efg/?

Also do we even want to support these inline ones? or just whole segments?

I thought envoy only allows ** as the last component ?

It has to be the last operator, not the last component of the path.

Can we add a test with url-encoded *, {, etc ?

costinm · 2024-04-19T20:53:11Z

Is this something based on some prior art - or another Envoy NIH ? I assume we confirmed this is going to be a long term supported API on their side. If we didn't have the K8SGateway mode mixed and the waypoints - I wouldn't mind this too much.

…

On Fri, Apr 19, 2024, 14:31 Jackie Elliott ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/integration/security/testdata/authz/path-templating.yaml.tmpl <#50365 (comment)>: > @@ -0,0 +1,13 @@ +apiVersion: security.istio.io/v1beta1 +kind: AuthorizationPolicy +metadata: + name: {{ .To.ServiceName }} +spec: + selector: + matchLabels: + "app": "{{ .To.ServiceName }}" + action: ALLOW + rules: + - to: + - operation: + paths: ["/allow/admin/{**}", "/foo/{*}/bar/{**}.txt", "/store/{**}/cart"] It has to be the last operator, not the last component of the path. — Reply to this email directly, view it on GitHub <#50365 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2SIK37N7IH2LUVO3GDY6FWK3AVCNFSM6AAAAABGBGE6PGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJSGIZDCNBRGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

template it cannot have the following chars outside of a supported template: {, }, and *. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

howardjohn

LGTM, just one small validation improvement I think we can use and a question

pilot/pkg/security/authz/builder/testdata/http/allow-path-out.yaml

howardjohn · 2024-04-19T22:56:19Z

pkg/config/security/security.go

+func CheckValidPathTemplate(key string, paths []string) error {
+	for _, path := range paths {
+		// If path does not contain any path templates, skip the check.
+		if !ContainsPathTemplate(path) {


I think we want to remove this if now, so we would reject something like /foo/{name}/bar.

Though we will need to improve the Contains(*) check since that is valid for !ContainsPathTemplate.

template operators in path. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Follow up to istio/istio#50365 (comment) Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

wulianglongrd · 2024-05-14T10:22:04Z

pkg/config/security/security.go

@@ -100,6 +105,36 @@ func CheckEmptyValues(key string, values []string) error {
 	return nil
 }

+func CheckValidPathTemplate(key string, paths []string) error {


Moving this into pilot/pkg/security/authz/matcher/template.go will look more cohesive.

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 10, 2024

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 10, 2024

jaellio force-pushed the jaellio/updatepaths branch from 8d0a246 to 8b38e95 Compare April 10, 2024 23:21

jaellio commented Apr 11, 2024

View reviewed changes

jaellio added the do-not-merge/hold Block automatic merging of a PR. label Apr 11, 2024

jaellio marked this pull request as ready for review April 11, 2024 19:37

jaellio requested a review from a team as a code owner April 11, 2024 19:37

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 11, 2024

jaellio force-pushed the jaellio/updatepaths branch from 8b38e95 to c8781df Compare April 13, 2024 00:41

jaellio commented Apr 13, 2024

View reviewed changes

tests/integration/security/authz_test.go Show resolved Hide resolved

michaelbannister reviewed Apr 15, 2024

View reviewed changes

howardjohn reviewed Apr 15, 2024

View reviewed changes

jaellio force-pushed the jaellio/updatepaths branch from 214c135 to 0593921 Compare April 16, 2024 20:52

jaellio requested a review from a team as a code owner April 16, 2024 23:53

keithmattix added this to the 1.22 milestone Apr 17, 2024

jaellio added 8 commits April 17, 2024 21:16

inital path templating changes

ef1bb72

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Clean up initial changes

b82f0b9

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Fix failing tests, release notes, and lint

422460d

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

- Adds initial basic integration tests. Currently failing for non

d4b01c3

a to b requests - Adds a name to typed extension config for uri template Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Fix failing integration tests

e11772b

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Update gen files

cb14d9d

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Add validation for support path templates

66d4fe0

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

Moved IsPathTemplate to resolve check-agent-deps failures.

0146c61

Added additional path template validation. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

jaellio force-pushed the jaellio/updatepaths branch from 49704e8 to 0146c61 Compare April 17, 2024 23:20

This was referenced Apr 18, 2024

Add pathTemplates and notPathTemplates to AuthorizationPolicy istio/api#3142

Closed

Add docs and examples for path templating istio/api#3162

Merged

Simplify replacement logic and add additional test case for

40546e0

multiple matches Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

keithmattix approved these changes Apr 18, 2024

View reviewed changes

Fix nits

15c96fb

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

jaellio force-pushed the jaellio/updatepaths branch from 42205a0 to 15c96fb Compare April 19, 2024 16:50

howardjohn reviewed Apr 19, 2024

View reviewed changes

Add more restrictive validation. If a path contains a supported

b788d51

template it cannot have the following chars outside of a supported template: {, }, and *. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 19, 2024

Fix lint

c37e128

Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

howardjohn reviewed Apr 19, 2024

View reviewed changes

howardjohn approved these changes Apr 19, 2024

View reviewed changes

Consider named vars invalid with and without supported path

2b3cb4c

template operators in path. Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

jaellio force-pushed the jaellio/updatepaths branch from 9c5091f to 2b3cb4c Compare April 20, 2024 00:46

jaellio removed the do-not-merge/hold Block automatic merging of a PR. label Apr 20, 2024

istio-testing merged commit 6f0f913 into istio:master Apr 20, 2024
28 checks passed

jaellio added a commit to jaellio/istio.io that referenced this pull request Apr 24, 2024

Clarify use of path template operators with MERGE_SLASHES

60f2f75

Follow up to istio/istio#50365 (comment) Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

jaellio mentioned this pull request Apr 24, 2024

Clarify use of path template operators with MERGE_SLASHES istio/istio.io#14954

Merged

11 tasks

istio-testing pushed a commit to istio/istio.io that referenced this pull request Apr 24, 2024

Clarify use of path template operators with MERGE_SLASHES (#14954)

f563b81

Follow up to istio/istio#50365 (comment) Signed-off-by: Jackie Elliott <jaellio@microsoft.com>

craigbox mentioned this pull request May 10, 2024

Authorization policy: allow specifying string match for path and conditions istio/api#2173

Closed

jaellio mentioned this pull request May 11, 2024

Add jaellio as security maintainer istio/community#1399

Merged

6 tasks

wulianglongrd reviewed May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add one/many path templating support for AuthPolicy #50365

Add one/many path templating support for AuthPolicy #50365

jaellio commented Apr 10, 2024 •

edited

istio-testing commented Apr 10, 2024

istio-policy-bot commented Apr 10, 2024

jaellio commented Apr 10, 2024

jaellio commented Apr 10, 2024

jaellio Apr 11, 2024

michaelbannister Apr 15, 2024 •

edited

howardjohn Apr 15, 2024

keithmattix Apr 15, 2024

jaellio Apr 16, 2024

keithmattix Apr 18, 2024

jaellio Apr 19, 2024

costinm Apr 19, 2024

jaellio commented Apr 11, 2024

michaelbannister Apr 15, 2024 •

edited

jaellio Apr 19, 2024

howardjohn left a comment

howardjohn Apr 15, 2024

keithmattix commented Apr 16, 2024

jaellio commented Apr 16, 2024

keithmattix left a comment

howardjohn Apr 18, 2024

keithmattix Apr 19, 2024

jaellio Apr 24, 2024

howardjohn Apr 18, 2024

howardjohn Apr 18, 2024

costinm Apr 19, 2024

jaellio Apr 19, 2024

costinm Apr 19, 2024

costinm commented Apr 19, 2024 via email

howardjohn left a comment

howardjohn Apr 19, 2024

jaellio Apr 20, 2024

wulianglongrd May 14, 2024

Add one/many path templating support for AuthPolicy #50365

Add one/many path templating support for AuthPolicy #50365

Conversation

jaellio commented Apr 10, 2024 • edited

istio-testing commented Apr 10, 2024

istio-policy-bot commented Apr 10, 2024

jaellio commented Apr 10, 2024

jaellio commented Apr 10, 2024

Choose a reason for hiding this comment

michaelbannister Apr 15, 2024 • edited

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

jaellio commented Apr 11, 2024

michaelbannister Apr 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithmattix commented Apr 16, 2024

jaellio commented Apr 16, 2024

keithmattix 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

costinm commented Apr 19, 2024 via email

howardjohn 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

jaellio commented Apr 10, 2024 •

edited

michaelbannister Apr 15, 2024 •

edited

michaelbannister Apr 15, 2024 •

edited