Add docs and examples for path templating

[jaellio]

Related to istio/istio#50365

Part of istio/istio#16585

[istio-testing]

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

[jaellio]

/test all

[costinm]

Isn't this technically a backward incompatible change ? I can have path /a/* or /a/{foo} - I don't think * or {} were forbidden before. After the change - they'll no longer match literally but use the template.

Also: do we have the same model for routing ? Would be really bad for them to do things differently.

I would be concerned if authz and routing diverge - in particular in the new K8SGateway world where targetRef could be used with HttpRoutes for performance/scale ( in which case path would presumably be empty ). Not entirely clear if allowing namespaces to set authz rules in shared gateways at listener level is even secure, we don't typically do security reviews for our security features...

[costinm]

I'm not against using path matches - I think it's a great feature. But it should be reviewed in the broader context - including how we can use it with HttpRoute ( is it allowed ? Do we need to extend the spec ), and than look at how routing and authz work together - both in K8SGateway/Ambient mode and in Istio v1 mode. Adding new features that only work with Istio v1 as 'extension' may not be worth it unless we are sure we understand the security implications.

[howardjohn]

{} are not valid characters in a URL which I think makes it viable

[keithmattix]

+1 to @howardjohn the fact that {} are not valid URL characters according to the spec means that this change isn't breaking any active policies today. With respect to the security model, this is actually a fairly narrow diff compared to our existing path matching features

[louiscryan]

Yeah, Im not worried about collision for '{}' and there is no conflict with out path normalization so I think this is OK.

I guess there is a technical back-compat issue that if folks wrote rules that used '{}' that ALLOWED traffic, those rules would be dormant and now would start matching something. I think this is so esoteric as to be a non-concern.

I do agree with Costin we should look at this for routing and bring those into alignment. They were already misaligned to some extent anyway (routing has regex but authz does not for instance). I generally prefer UriTemplate/glob over regex for paths so Im definitely in favor of pushing on that.

Here is the list of follow-on work for UriTemplate that we should open tracking issue for

• routing
• attribute extraction via named sections into processing context
• SPIFFE SAN matching improvements (they are URIs after all)
• Query param matching in Authz

[keithmattix]

+1 to the follow up action items of doing this for routing. @jaellio or myself will open the tracking issues

[howardjohn]

This example is not consistent with the claim that we "matches a single path segment". If its a path segment I would think you can only have .../{*} or .../{*}/....

This is more like a "glob that cannot extend beyond a path segment"?

What does /foo/{*}.txt mean

[jaellio]

Good point, I'll clarify.

/foo/{*}.txt matches the glob until the specified path extension. Right now, this is supported. I can add an additional test case for foo/{*}.txt to see if it matches on /foo/bar.tmp.txt for example.

[costinm]

On Thu, Apr 18, 2024, 14:43 John Howard ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/v1/authorization_policy.proto <#3162 (comment)>: > @@ -521,6 +521,18 @@ message Operation { // for details of the path normalization. // For gRPC service, this will be the fully-qualified name in the form of `/package.service/method`. // + // If a path in the list contains the `{*}` or `{**}` operator, it will be interpreted as an [Envoy Uri Template](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/path/match/uri_template/v3/uri_template_match.proto). {} are not valid characters in a URL which I think makes it viable

I see - makes it backwards compatible. Not sure about viable or secure - see my other comment.

[costinm]

On Thu, Apr 18, 2024, 14:58 Keith Mattix II ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/v1/authorization_policy.proto <#3162 (comment)>: > @@ -521,6 +521,18 @@ message Operation { // for details of the path normalization. // For gRPC service, this will be the fully-qualified name in the form of `/package.service/method`. // + // If a path in the list contains the `{*}` or `{**}` operator, it will be interpreted as an [Envoy Uri Template](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/path/match/uri_template/v3/uri_template_match.proto). +1 to @howardjohn <https://github.com/howardjohn> the fact that {} are not valid URL characters according to the spec means that this change isn't breaking any active policies today. With respect to the security model, this is actually a fairly narrow diff compared to our existing path matching features

I suspect some of my concerns about miss-match between routing and authz may apply to existing path matching in authz. We already have subtle differences between routing with HttpRoute and VirtualService - and it was decided that authz and other configs apply to both. And it's not like the past review process was very good and took security ( or scalability) seriously into account...

…

Message ID: ***@***.***>

[costinm]

Perhaps a bit off-topic for this PR (but closely related to the "valid characters"): you are correct that {} are not allowed in the encoded form of the URL, and we have the 'path normalization' section that doesn't mention it - just like it doesn't mention any of the UTF8/Unicode characters. Obviously { and } are ok in URL-encoded form - and it is not at all clear from our docs on how users should handle this. The path normalization suggests some characters will be decoded as part of normalization - but I guess since it omits UTF and {, the expectation is that the matching will happen on the url-encoded form except for the normalized list ? It would be great to be very explicit about this - in particular if we are starting to depend on { } in url-encoded form. Also need to mention that this applies to all Unicode characters - i.e. all characters (except the safe list ) in the match expressions must be URL-encoded. It is not obvious - JSON and Yaml use UTF-8 strings. Hostname matcher also should be clarified - hosts are case-insensitive ASCII, but punycode allows most UTF-8 chars. So the doc should be clear that user must punycode for hostnames and url-encode for URLs - and I don't remember what needs to be used for http headers ( H2 allows binary headers for example - and I don't think it defines an encoding). Since handling of unicode and binary in YAML/JSON is UTF-8 and base64 - and we are doing things differently - it should be explicit.

…

On Thu, Apr 18, 2024 at 3:44 PM Costin Manolache ***@***.***> wrote: On Thu, Apr 18, 2024, 14:43 John Howard ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In security/v1/authorization_policy.proto > <#3162 (comment)>: > > > @@ -521,6 +521,18 @@ message Operation { > // for details of the path normalization. > // For gRPC service, this will be the fully-qualified name in the form of `/package.service/method`. > // > + // If a path in the list contains the `{*}` or `{**}` operator, it will be interpreted as an [Envoy Uri Template](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/path/match/uri_template/v3/uri_template_match.proto). > > {} are not valid characters in a URL which I think makes it viable > I see - makes it backwards compatible. Not sure about viable or secure - see my other comment.

[costinm]

Louis - which routing ? The Istio authz policy is both a workload-selector API for v1 and a parentRef for Gateway - and in Gateway the routing is subtly different. I think regex is optional - not sure which variant of regex, but it is more restrictive than VirtualService. That's the problem with mixing the APIs - someone will get hurt. Good point on attribute extraction - it is not clear how the named attributes will be used.

…

On Fri, Apr 19, 2024 at 12:27 PM Louis Ryan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In security/v1/authorization_policy.proto <#3162 (comment)>: > @@ -521,6 +521,18 @@ message Operation { // for details of the path normalization. // For gRPC service, this will be the fully-qualified name in the form of `/package.service/method`. // + // If a path in the list contains the `{*}` or `{**}` operator, it will be interpreted as an [Envoy Uri Template](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/path/match/uri_template/v3/uri_template_match.proto). Yeah, Im not worried about collision for '{}' and there is no conflict with out path normalization so I think this is OK. I guess there is a technical back-compat issue that if folks wrote rules that used '{}' that ALLOWED traffic, those rules would be dormant and now would start matching something. I think this is so esoteric as to be a non-concern. I do agree with Costin we should look at this for routing and bring those into alignment. They were already misaligned to some extent anyway (routing has regex but authz does not for instance). I generally prefer UriTemplate/glob over regex for paths so Im definitely in favor of pushing on that. Here is the list of follow-on work for UriTemplate that we should open tracking issue for - routing - attribute extraction via named sections into processing context - SPIFFE SAN matching improvements (they are URIs after all) - Query param matching in Authz — Reply to this email directly, view it on GitHub <#3162 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2TMRPGIO56S6KNQVWDY6FVYRAVCNFSM6AAAAABGMH3AGWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJSGIYTIMRRHE> . You are receiving this because you commented.Message ID: ***@***.***>

[keithmattix]

LGTM - it's just docs, so more clarification can be added later if needed