search

istio / api

API definitions for the Istio project
Apache License 2.0
456 stars 550 forks source link

Add docs and examples for path templating #3162

Closed jaellio closed 5 months ago

jaellio commented 5 months ago

Related to https://github.com/istio/istio/pull/50365

Part of https://github.com/istio/istio/issues/16585

istio-testing commented 5 months ago

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 commented 5 months ago

/test all

costinm commented 5 months ago

On Thu, Apr 18, 2024, 14:43 John Howard @.***> wrote:

@.**** commented on this pull request.

In security/v1/authorization_policy.proto https://github.com/istio/api/pull/3162#discussion_r1571289034:

@@ -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.

{} 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 commented 5 months ago

On Thu, Apr 18, 2024, 14:58 Keith Mattix II @.***> wrote:

@.**** commented on this pull request.

In security/v1/authorization_policy.proto https://github.com/istio/api/pull/3162#discussion_r1571303795:

@@ -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.

+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 commented 5 months ago

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 https://github.com/istio/api/pull/3162#discussion_r1571289034:

@@ -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.

{} 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 commented 5 months ago

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 https://github.com/istio/api/pull/3162#discussion_r1572830827:

@@ -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.

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 https://github.com/istio/api/pull/3162#discussion_r1572830827, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2TMRPGIO56S6KNQVWDY6FVYRAVCNFSM6AAAAABGMH3AGWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJSGIYTIMRRHE . You are receiving this because you commented.Message ID: @.***>