Feature/wildcard matching

[mstrYoda]

Please provide a description of this PR:

Add wildcard path matching. This pr makes possible to write authz. paths like:

• "/test/*/resource"
• "/test/*/resource/*/subResource"

Resolves: #16585

I've test it locally and it works fine.

Here is the AuthorizationPolicy I wrote:

apiVersion: v1
items:
- apiVersion: security.istio.io/v1beta1
  kind: AuthorizationPolicy
  metadata:
    name: test-auth-policy
  spec:
    action: DENY
    rules:
    - to:
      - operation:
          methods:
          - GET
          paths:
          - "/test/*/example"
          - "/test/*/example/*/subpath"
      when:
      - key: request.headers[test]
        notValues:
        - example
    - to:
      - operation:
          methods:
          - POST
          paths:
          - "/test/*/example"
      when:
      - key: request.headers[test]
        notValues:
        - example
    selector:
      matchLabels:
        app: nginx
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

Here is the example requests and expected behaviors:

[istio-policy-bot]

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

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @mstrYoda. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mstrYoda]

@howardjohn @yangminzhu @shamsher31 @hzxuzhonghu can you guys review it when you have free time? 🙏

[howardjohn]

I am concerned this will lead to another stream of CVEs related to path matching. Path matching is an extremely tricky subject, and this introduces a ton of ambiguities that can lead to unauthorized access.

Additionally, I am concerned about the matching discrepancies with Authz and VirtualService with this. It seems odd I can allow /foo/*/bar but I cannot then route on the same decision.

[howardjohn]

This is not safe. At the very least you need regex.QuoteMeta. Also, is a/*/b supposed to match a/x/y/z/b or only one path element?

[mstrYoda]

I thought that it's up to developer who writes authz policy. If developers want to allow any path between a/.../b it is ok to write a/.+/b but if anyone wants to restrict specific paths, it is need to be specified, am I wrong?

+1 for regex.QuoteMeta I will send the changes 🙏

[howardjohn]

I don't think the dev can chose to write a/.+/b though since we are only allowing wildcard.

Actually looking at it more we are not even providing 'wildcard' support - really we are providing ANY regex support, if it happens to contain a *?

[mstrYoda]

I mean that if anyone want to allow any sub paths between a/.../b is not that ok to write a/*/b in policy? So we can match all paths between that two path. Sorry if I could not understand you exactly.

[mstrYoda]

Hi @howardjohn, what is your opinion on my last comment?

[yangminzhu]

Thanks for the PR, this is a very useful feature but it's also very tricky to get it right.

As this changes the semantic of the path field in the authorization policy (beta API), I think we should get approval at least from the security work group. Could you discuss it in work group meeting and probably write a quick doc about the change for collecting feedback and helping discussions?

[sschepens]

@howardjohn we also would need this feature, or be able to specify regexes.

Additionally, I am concerned about the matching discrepancies with Authz and VirtualService with this. It seems odd I can allow /foo/*/bar but I cannot then route on the same decision.

This also comes to mind, why do we handle Authorization matching in a different way that we do Route matching? i seems as though they should be the same or very similar. Shouldn't we just migrate Authorization matching to use StringMatch ?

[ognjen-it]

@yangminzhu Hi, what about securty team? Does anyone sent this PR to tham?

We really need it 💯☺️🚀🎉

[qRainBirdp]

Has this function already beeHas this function already been available in the official version of istio?
We really need it！

[mstrYoda]

@yangminzhu Hi, what about securty team? Does anyone sent this PR to tham?

We really need it 💯☺️🚀🎉

Has this function already beeHas this function already been available in the official version of istio? We really need it！

Is there any Istio Security Team member here? Can anyone mention someone that can review this feature?

@howardjohn @yangminzhu can you make any comments or change suggestions (in ur free time) according to my last comments in review? 🙏

[craigbox]

Are you able to drop by a working group meeting to discuss?

See #35641 (review):

Thanks for the PR, this is a very useful feature but it's also very tricky to get it right.

As this changes the semantic of the path field in the authorization policy (beta API), I think we should get approval at least from the security work group. Could you discuss it in work group meeting and probably write a quick doc about the change for collecting feedback and helping discussions?

[mstrYoda]

Are you able to drop by a working group meeting to discuss?

See #35641 (review):

Thanks for the PR, this is a very useful feature but it's also very tricky to get it right.
As this changes the semantic of the path field in the authorization policy (beta API), I think we should get approval at least from the security work group. Could you discuss it in work group meeting and probably write a quick doc about the change for collecting feedback and helping discussions?

Of course, I am willing to attend to wg meeting 🙏

[mstrYoda]

Are you able to drop by a working group meeting to discuss?

See #35641 (review):

Thanks for the PR, this is a very useful feature but it's also very tricky to get it right.
As this changes the semantic of the path field in the authorization policy (beta API), I think we should get approval at least from the security work group. Could you discuss it in work group meeting and probably write a quick doc about the change for collecting feedback and helping discussions?

Hi @craigbox, can we reopen this PR? This is a highly wanted feature from community.

I try to attend one of the sec. workgroup meeting 🙏

[nberkley-gp]

Is there any chance of getting this merged? The lack of unanchored string matches in auth policies is an issue for multiple use cases and all the related PRs/Issues seem to have gone silent for a while.

[craigbox]

Did @mstrYoda meet with the working groups?
@howardjohn for further comment

[mstrYoda]

Did @mstrYoda meet with the working groups?

@howardjohn for further comment

I did not after I saw that there will be a proposal doc. But I can attend to the closest meeting ASAP if you want to 👍

[howardjohn]

This PR cannot be accepted as is (not the feature in general, just this PR from a process point of view); this will require an API change - even if not changing the proto, the docs at least.

To keep the discussion focused lets consolidate on istio/api#2173 or the issue #16585

Thanks!