authz support string contains match

[wulianglongrd]

support string contains match or authz request.headers, fix https://github.com/istio/istio/issues/52557

This is probably a behavior change: the old unit tests treat it as a suffix match, this PR treat it as a contains match, see: https://github.com/istio/istio/blob/42e7e83686df24d519c5dd6ac626e3394e44c61b/pilot/pkg/security/authz/matcher/header_test.go#L48

Another option is to treat .*foo.* (rather than *foo*) as a contains semantic. (Or use other methods to support contains or regex semantics.)

[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

[wulianglongrd]

/test all

[wulianglongrd]

/test all

[wulianglongrd]

I think the right solution should to support regex match

Thanks for the review, I have similar ideas to yours, but I currently don't have a good way to express regex semantics.

Do you have any suggestions for using key or values to express regex semantics?

[wulianglongrd]

BTW, currently values support the following semantics:

1. *: preset match
2. *{anything}: suffix match
3. {anything}*: prefix match
4. the rest: exact match

Use regex:// prefix to express regex semantics?

[hzxuzhonghu]

seems too hardcoding, how about adding a new field type []StringMatcher like what we do in VS, this field to deprecate the old values

[howardjohn]

Thanks for the PR. I have a few concerns

1. As this is an API change to a v1 API, this should go through API review before implementation
2. This seems inconsistent to apply only to headers
3. The intended use case here seems to be matching multiple headers flattened into a single one, but doesn't actually do that securely - I worry this would lead to easily miswritten policies that introduce vulnerability

[wulianglongrd]

seems too hardcoding, how about adding a new field type []StringMatcher like what we do in VS, this field to deprecate the old values

@hzxuzhonghu Thanks, this solution makes sense to me.

[wulianglongrd]

@howardjohn Thanks.

1. As this is an API change to a v1 API, this should go through API review before implementation

agree.

2. This seems inconsistent to apply only to headers

Thanks for pointing this out, it also applies to :method and probably nothing else.

3. The intended use case here seems to be matching multiple headers flattened into a single one, but doesn't actually do that securely - I worry this would lead to easily miswritten policies that introduce vulnerability

😊 Can you give an example for this?

[howardjohn]

😊 Can you give an example for this?

Yes, sorry I sent that from my phone so was hard to elaborate..

In your example:

• rule is *foo=*. "Cookie: not-foo=bar" unexpecedly matches
• rule is *foo*. "X-foo: not foo bar" unexpectedly matches

[wulianglongrd]

@hzxuzhonghu @howardjohn

How about using the following syntax? One of values, notValues, match. The new match field is StringMatch type.

  rules:
  - when:
    # old syntax
    - key: request.headers[header]
      values: ["value"]

    # new syntax: exact match
    - key: request.headers[header1]
      match:
        exact: value1

    # new syntax: prefix match
    - key: request.headers[header2]
      match:
        prefix: value2

    # new syntax: regex match
    - key: request.headers[header3]
      match:
        regex: value3

// Condition specifies additional required attributes.
message Condition {
  // The name of an Istio attribute.
  string key = 1 [(google.api.field_behavior) = REQUIRED];

  // Optional. A list of allowed values for the attribute.
  // Note: at least one of `values`, `notValues` or `match` must be set.
  repeated string values = 2;

  // Optional. A list of negative match of values for the attribute.
  // Note: at least one of `values`, `notValues` or `match` must be set.
  repeated string not_values = 3;

  // Optional. Allowed values for the attribute using StringMatch.
  // Note: at least one of `values`, `notValues` or `match` must be set.
  istio.networking.v1alpha3.StringMatch match = 4;
}

[hzxuzhonghu]

Note: at least one of values, notValues or match must be set.

only one or at least one?

[wulianglongrd]

Note: at least one of values, notValues or match must be set.

only one or at least one?

a. values, not_values b. match

At least one of a or b. when a is selected, at least one of values or not_values.

At least one of values, not_values or match also look good to me.

[howardjohn]

In that past we have had pretty conclusive decision to not add regex into authz policy. See discussion in https://github.com/istio/istio/issues/25021 and many linked issues.

Even to serve this use case, regex is inappropriate as I tried to show in https://github.com/istio/istio/pull/52583#issuecomment-2284453746.

[howardjohn]

What is really desired here, it seems, is to be able to match on individual headers instead of merged ones

[hzxuzhonghu]

Note: at least one of values, notValues or match must be set.

only one or at least one?

@wulianglongrd I think it should be only one, not more than one. Like you cannot specify string match with values, they can conflict

[hzxuzhonghu]

What is really desired here, it seems, is to be able to match on individual headers instead of merged ones

@howardjohn regex can be used to match this with ; provided, for more complex case, it may need envoy to support

[wulianglongrd]

Sorry for the late reply.

In that past we have had pretty conclusive decision to not add regex into authz policy. See discussion in #25021 and many linked issues.

Thanks @howardjohn . I take a quick look and saw some reasons for not implementing regex: https://github.com/istio/istio/issues/16585#issuecomment-912846421, https://github.com/istio/istio/issues/16585#issuecomment-1456359549, https://github.com/istio/istio/issues/16585#issuecomment-1164633757, https://github.com/istio/istio/issues/16585#issuecomment-984790462 . BTW, it seems that the path templating matching(https://github.com/istio/istio/pull/50365) currently only supports path, which does not cover https://github.com/istio/istio/issues/52557 issue (request.headers).

Even to serve this use case, regex is inappropriate as I tried to show in #52583 (comment).

I think correct regex can prevent these issues. But, inaccurate regex may cause security issues.

What is really desired here

The motivation is to fix https://github.com/istio/istio/issues/52557 and hope that the matching rules of authz and VS are consistent.

I think it should be only one, not more than one. Like you cannot specify string match with values, they can conflict

Thanks @hzxuzhonghu . Yes, if it is "more than one", the usage will be more complicated and there may be conflicts.

[howardjohn]

#52557 is really a structured match requirement.. If we had a way to match on individual split headers instead of aggregated ones, it would solve this.

That is, instead of matching against cookie1=whatever; cookie2=whatever;, it would match against cookie1=whatever and cookie2=whatever. Not sure what this would look like in terms of API, AND vs OR them together, etc though.

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2024-08-28. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.