Repository navigation
Conversation
|
Hi @sayap. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
1 similar comment
|
Hi @sayap. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
This builds on top of #3737, mapping In Consul, this is even hardcoded to be |
|
/ok-to-test |
|
Can you provide some more context on the exact-use case here and what this API will solve? |
| // If set to true, a verification failure is written to the filter's dynamic metadata | ||
| // under the fixed key `istio_jwt_verification_failure`. The failure is then observable | ||
| // even when the request is not rejected, such as when `presence` is `IGNORED`. | ||
| bool failed_status_in_metadata = 16; |
There was a problem hiding this comment.
This feels like something that should just be not a flag and used if IGNORED is used
There was a problem hiding this comment.
Yeah I was undecided about this, and then I thought it might be useful to also enable this flag with OPTIONAL / REQUIRED as well.
In our case, when we transition a provider from IGNORED (dry-run) to OPTIONAL (enforce), keeping the same level of observability is desirable. So we just leave this flag enabled all the time.
There was a problem hiding this comment.
I have changed this from bool to string. Previously, I used bool to be consistent with the hardcoded EnvoyJwtFilterPayload = "payload". However, payload is more for internal usage, and there can only be one anyway. Meanwhile, this failed status is more external facing, to be used in conjunction with Telemetry CR, Lua filter, etc. Furthermore, when there are multiple providers, Envoy can add multiple verification failures to dynamic metadata. So I think a configurable string field is a better fit here.
We want to handle the first scenario described in istio/istio#43982, i.e. having a dry-run period where a mistake in mesh config or a misbehaving client does not result in 401. |
| REQUIRED = 1; | ||
|
|
||
| // The JWT requirement is always satisfied. A request with a missing or | ||
| // failed token is not rejected. A present token is still verified and |
There was a problem hiding this comment.
Can we tighten the language so it's clear what a "failed" token is? It should be clear to users what the difference between IGNORED and OPTIONAL is
There was a problem hiding this comment.
Right, I updated the wordings to use "invalid" instead of "failed" (this came from Envoy), and also show the difference between the 3 options.
2cac46c to
0561058
Compare
0561058 to
0996cc9
Compare
| // `{"code": 3, "message": "Jwt is expired"}`. When there are multiple | ||
| // providers, each one should use its own key, otherwise failures can | ||
| // overwrite each other. | ||
| string failed_status_in_metadata = 16; |
There was a problem hiding this comment.
nit: I think this should be failed_status_metadata_key but we can fix in a follow up
There was a problem hiding this comment.
Thanks, that's a good suggestion. I should rename it
There was a problem hiding this comment.
Renamed, let me update the PR title and description as well
`presence: IGNORED` makes the requirement always satisfied. In this mode, requests with a missing token and requests with an invalid token are both allowed. `failedStatusMetadataKey`, if set, adds verification failure to the dynamic metadata under this key, such that failures are always observable regardless of the `presence` value.
0996cc9 to
2d46074
Compare
presence: IGNORED and failedStatusInMetadata to JWTRulepresence: IGNORED and failedStatusMetadataKey to JWTRule
presence: IGNOREDmakes the requirement always satisfied. In this mode, requests with a missing token and requests with an invalid token are both allowed.failedStatusMetadataKey, if set, adds verification failure to the dynamic metadata under this key, such that failures are always observable regardless of thepresencevalue.