envoy: use upstream JWT filter instead of the custom Istio extension #47407

kyessenov · 2023-10-17T19:30:27Z

Change-Id: I1a34a470f9770e1c7990e12a8370ee93fb297279

Modifies xDS to use the upstream JWT filter from Envoy instead of the extra Istio istio_authn HTTP filter for JWT matching and JWT policy evaluation. It is functionally equivalent with the main difference being that the space-delimited claims must be explicitly opted-in. The default space-delimited claims are scope and permission.

Issue: envoyproxy/envoy#29681

Change-Id: I1a34a470f9770e1c7990e12a8370ee93fb297279 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: I99c29a3b28ffad9ae4125b2ea9da1d00b0430044 Signed-off-by: Kuat Yessenov <kuat@google.com>

This reverts commit b444bd2.

This reverts commit ea73ad6.

Change-Id: I2943e8a1fe415c7fa3558a903d374413a7e3a6da Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: I87ef250fe59af07aa43ae0684fe944dc446f5713 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Id46d841ab58abfcb619d4c15df23604e2aafd26e

Change-Id: I243387881193c0676796472c30d0bcf07ca8c047 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Ie69f3d0728e9b4c48c9d6807e67075a904066d66

Change-Id: I0e684914d85f6fb5edebd92127e0426f5b05b4c3 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Ic5dabdc5cd6681899e251eaa89f1c221a9a285a7 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Ia1bba5731a98d8841adbe99ae1d1518cbafa852a Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: I0695b5c1aefec0198886d0068cc3667c8c3225e1 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: I31b1278dbeba17203766cade3f94c3003d366a9f Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Ic1445984457022cf6099a3df43ad79c9f95b1e4d Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: Iaac6c9912d4d20851e524075f899e90827bea118 Signed-off-by: Kuat Yessenov <kuat@google.com>

pilot/pkg/networking/core/v1alpha3/route/route_internal_test.go

pilot/pkg/networking/core/v1alpha3/route/route_test.go

hzxuzhonghu · 2023-11-09T02:12:48Z

pilot/pkg/networking/grpcgen/lds.go

@@ -270,7 +270,7 @@ func buildRBAC(node *model.Proxy, push *model.PushContext, suffix string, contex
 	for _, policy := range policies {
 		for i, rule := range policy.Spec.Rules {
 			name := fmt.Sprintf("%s-%s-%d", policy.Namespace, policy.Name, i)
-			m, err := authzmodel.New(rule)
+			m, err := authzmodel.New(rule, true)


Does grpc support this filter before? I am concerned about the compatibility here

gRPC doesn't support it either way. Any value here would not work for JWT.

hzxuzhonghu · 2023-11-09T02:16:10Z

pilot/pkg/security/authn/policy_applier.go

@@ -32,7 +32,7 @@ type PolicyApplier interface {

 	// JwtFilter returns the JWT HTTP filter to enforce the underlying authentication policy.
 	// It may return nil, if no JWT validation is needed.
-	JwtFilter() *hcm.HttpFilter
+	JwtFilter(useExtendedJwt, clearRouteCache bool) *hcm.HttpFilter


clearRouteCache looks weired

Suggestions? That's what xDS field is called.

actually i am not familiar with jwt clear cache, will take a look though

It is to clear the cache used for making route decision, right? @kyessenov

Yes, it's to re-calculate the route selection

hzxuzhonghu · 2023-11-09T04:10:14Z

pilot/pkg/networking/plugin/authn/authentication.go

@@ -99,8 +99,15 @@ func (b *Builder) BuildHTTP(class networking.ListenerClass) []*hcm.HttpFilter {
 		// Only applies to inbound and gateways
 		return nil
 	}
+	if b.proxy.SupportsEnvoyExtendedJwt() {
+		filter := b.applier.JwtFilter(true, b.proxy.Type != model.SidecarProxy)


why do we clear cache only for no sidecar? can we always set it true

And actually envoy only clear when ClearRouteCache = true and claim_to_headers added

It only makes sense on gateways because there are routes there. Inbound listener has no routes, see https://istio.io/latest/docs/tasks/security/authentication/jwt-route/ disclaimer at the top.

IC，i mean once we add another type, this could be broken. Is there any side-effect if we set it true for sidecar

Yes, it causes overhead to reset a cache. It is also backwards incompatible since other filters might affect the route as well.

Change-Id: I67743d13a0670fca111cfe8dd923bc3f6906625a

Change-Id: I31a7035c45b2647379e365b72f4d5ef4cf3db6c3 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2023-11-14T23:25:41Z

Gentle ping.

zirain

overall lgtm

howardjohn · 2023-11-15T01:31:30Z

pilot/pkg/security/authz/model/generator.go

 	if forTCP {
 		return nil, fmt.Errorf("%q is HTTP only", key)
 	}
-
+	if rpg.useExtendedJwt {
+		iss, sub, _ := strings.Cut(value, "/")


we should probably reject if there is no / in it here, and add a warning into the webhook if a user does this?

Actually it was valid to have something like a* which matches iss with prefix a and any sub. Let me update it.

howardjohn · 2023-11-15T01:34:58Z

pilot/pkg/security/authz/model/generator.go

+		}
+		im := MetadataMatcherForJWTClaims([]string{"iss"}, matcher.StringMatcher(iss), true)
+		sm := MetadataMatcherForJWTClaims([]string{"sub"}, matcher.StringMatcher(sub), true)
+		return principalAnd([]*rbacpb.Principal{principalMetadata(im), principalMetadata(sm)}), nil


I think we would now allow matching a*/b where before we would not since we treated the whole thing as one unit.

Which is probably reasonable. Not sure why we didn't just make 2 fields to begin with

Yeap, added a fix for this.

Change-Id: Iaf2d407dd1e95312d1aee5bf72a7da22f2b1c211 Signed-off-by: Kuat Yessenov <kuat@google.com>

Change-Id: I8f0a68fa400f83e120a99ad634cac54cb5901758 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2023-11-17T17:21:09Z

Any more comments?

aryan16 · 2023-11-17T18:10:49Z

pilot/pkg/model/context.go

@@ -1338,6 +1338,13 @@ func (node *Proxy) WorkloadEntry() (string, bool) {
 	return node.workloadEntryName, node.workloadEntryAutoCreated
 }

+// SupportsEnvoyExtendedJwt indicates that the proxy JWT extension is capable of
+// replacing istio_authn filter.
+func (node *Proxy) SupportsEnvoyExtendedJwt() bool {


This wouldn't be a breaking change for any user right and users don't need to configure this? Why can't we just cherrypick for all the supported versions instead of maintaining different configs?

Yes, it should be fully compatible - it's never 100% but close to it. We can't backport Envoy features, that's against the project policy to backport features and not fixes.

aryan16 · 2023-11-17T18:24:27Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

@@ -226,6 +226,13 @@ func convertToEnvoyJwtConfig(jwtRules []*v1beta1.JWTRule, push *model.PushContex
 			ForwardPayloadHeader: jwtRule.OutputPayloadToHeader,
 			PayloadInMetadata:    jwtRule.Issuer,
 		}
+		if useExtendedJwt {
+			provider.PayloadInMetadata = filters.EnvoyJwtFilterPayload


this would break a user who is already assuming Issuer in the payload and using that for some reason? We should atlease document this?

You mean the internal dynamic metadata from istio_authn? That was not a contract we generally follow - most of Envoy internal interfaces are private for a good reason.

I mean any user can have a custom envoy filter that is reading the dynamic metadata from jwt_authn filter. It could break them?

previously we were just adding issuer now payload. Wouldn't it be a problem?

Yes, if they have a custom Envoy/Wasm filter, they need to update. That's always the expectation that the low-level Envoy interfaces are not stable; otherwise, we could never make Envoy changes. I'll update the change note to warn against this. It's a fairly simple change to rename the metadata key.

sounds good!!

aryan16 · 2023-11-17T18:31:13Z

pilot/pkg/networking/plugin/authn/authentication.go

 	res := []*hcm.HttpFilter{}
-	if filter := b.applier.JwtFilter(); filter != nil {
+	if filter := b.applier.JwtFilter(false, false); filter != nil {


where's the code to ignore the istio_authn filter. I see we are still building isito_authn filter for all the cases?

The above if block exits early.

ah I missed that. Thanks

Change-Id: I668691cc926be197a253827ff02e85bf2bebd6fc

Change-Id: Iaea2503e3f0403766b96442ae3b3ce490e8bac08 Signed-off-by: Kuat Yessenov <kuat@google.com>

aryan16 · 2023-11-17T19:53:18Z

pilot/pkg/networking/plugin/authn/authentication.go

 	res := []*hcm.HttpFilter{}
-	if filter := b.applier.JwtFilter(); filter != nil {
+	if filter := b.applier.JwtFilter(false, false); filter != nil {


ah I missed that. Thanks

aryan16 · 2023-11-17T19:54:37Z

pilot/pkg/security/authn/v1beta1/policy_applier.go

@@ -226,6 +226,13 @@ func convertToEnvoyJwtConfig(jwtRules []*v1beta1.JWTRule, push *model.PushContex
 			ForwardPayloadHeader: jwtRule.OutputPayloadToHeader,
 			PayloadInMetadata:    jwtRule.Issuer,
 		}
+		if useExtendedJwt {
+			provider.PayloadInMetadata = filters.EnvoyJwtFilterPayload


sounds good!!

kyessenov · 2023-11-20T17:15:50Z

@ericvn can I get a test&release WG review?

kyessenov · 2023-11-20T19:05:51Z

@ericvn is OoO, @howardjohn can you give this another look, please?

ericvn · 2023-11-27T21:23:58Z

This PR looks to have caused a failure in step 3 of the jwt-route test: https://istio.io/latest/docs/tasks/security/authentication/jwt-route/#validating-ingress-routing-based-on-jwt-claims. Docs say we should get a 200, but we are getting back a 404.

test: remove istio_authn HTTP filter

b444bd2

Change-Id: I1a34a470f9770e1c7990e12a8370ee93fb297279 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested a review from a team as a code owner October 17, 2023 19:30

istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 17, 2023

match principal

0ed6ef9

Change-Id: I99c29a3b28ffad9ae4125b2ea9da1d00b0430044 Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 17, 2023

kyessenov added 3 commits October 17, 2023 20:36

Revert "test: remove istio_authn HTTP filter"

ea73ad6

This reverts commit b444bd2.

Reapply "test: remove istio_authn HTTP filter"

b0604b4

This reverts commit ea73ad6.

test

769f900

Change-Id: I2943e8a1fe415c7fa3558a903d374413a7e3a6da Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2023

map claims to envoy jwt

d0d018f

Change-Id: I87ef250fe59af07aa43ae0684fe944dc446f5713 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested a review from a team as a code owner October 17, 2023 21:02

kyessenov added 2 commits October 23, 2023 23:23

Merge remote-tracking branch 'upstream/master' into no_authn

daccbf2

Change-Id: Id46d841ab58abfcb619d4c15df23604e2aafd26e

value or

9ddc2d6

Change-Id: I243387881193c0676796472c30d0bcf07ca8c047 Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 23, 2023

kyessenov added 6 commits November 8, 2023 05:10

Merge remote-tracking branch 'upstream/master' into no_authn

964266b

Change-Id: Ie69f3d0728e9b4c48c9d6807e67075a904066d66

use scope delimited

619b753

Change-Id: I0e684914d85f6fb5edebd92127e0426f5b05b4c3 Signed-off-by: Kuat Yessenov <kuat@google.com>

test

f7b781c

Change-Id: Ic5dabdc5cd6681899e251eaa89f1c221a9a285a7 Signed-off-by: Kuat Yessenov <kuat@google.com>

drop authn

ed5eb68

Change-Id: Ia1bba5731a98d8841adbe99ae1d1518cbafa852a Signed-off-by: Kuat Yessenov <kuat@google.com>

match any

c65e595

Change-Id: I0695b5c1aefec0198886d0068cc3667c8c3225e1 Signed-off-by: Kuat Yessenov <kuat@google.com>

presence

4254ade

Change-Id: I31b1278dbeba17203766cade3f94c3003d366a9f Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov mentioned this pull request Nov 8, 2023

remove istio_authn http filter istio/proxy#5133

Merged

fix

622bf03

Change-Id: Ic1445984457022cf6099a3df43ad79c9f95b1e4d Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from a team as code owners November 8, 2023 18:19

fix

1c14b63

Change-Id: Iaac6c9912d4d20851e524075f899e90827bea118 Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov changed the title ~~test: remove istio_authn HTTP filter~~ envoy: use upstream JWT filter instead of the custom Istio extension Nov 8, 2023

kyessenov assigned aryan16 Nov 8, 2023

hzxuzhonghu reviewed Nov 9, 2023

View reviewed changes

kyessenov added 2 commits November 14, 2023 23:15

Merge remote-tracking branch 'upstream/master' into no_authn

1aa1a73

Change-Id: I67743d13a0670fca111cfe8dd923bc3f6906625a

revieww

b17f590

Change-Id: I31a7035c45b2647379e365b72f4d5ef4cf3db6c3 Signed-off-by: Kuat Yessenov <kuat@google.com>

zirain approved these changes Nov 15, 2023

View reviewed changes

howardjohn reviewed Nov 15, 2023

View reviewed changes

kyessenov added 2 commits November 15, 2023 04:23

fix

78b5768

Change-Id: Iaf2d407dd1e95312d1aee5bf72a7da22f2b1c211 Signed-off-by: Kuat Yessenov <kuat@google.com>

add tests

b9180bf

Change-Id: I8f0a68fa400f83e120a99ad634cac54cb5901758 Signed-off-by: Kuat Yessenov <kuat@google.com>

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2023

aryan16 reviewed Nov 17, 2023

View reviewed changes

kyessenov added 2 commits November 17, 2023 18:41

Merge remote-tracking branch 'upstream/master' into no_authn

2d56c67

Change-Id: I668691cc926be197a253827ff02e85bf2bebd6fc

add warning

e144b93

Change-Id: Iaea2503e3f0403766b96442ae3b3ce490e8bac08 Signed-off-by: Kuat Yessenov <kuat@google.com>

aryan16 approved these changes Nov 17, 2023

View reviewed changes

hzxuzhonghu approved these changes Nov 20, 2023

View reviewed changes

kyessenov assigned howardjohn Nov 20, 2023

howardjohn approved these changes Nov 20, 2023

View reviewed changes

istio-testing merged commit d4d1475 into istio:master Nov 20, 2023
27 checks passed

This was referenced Nov 28, 2023

Update istio test reference to latest commit and fix tests istio/istio.io#14200

Merged

Re-enable tasks/security/authentication/jwt-route/test.sh istio/istio.io#14224

Closed

keithmattix mentioned this pull request Mar 22, 2024

AuthorizationPolicy rules.from.source.requestPrincipals with https URL not working #49979

Closed

2 tasks

keithmattix mentioned this pull request Apr 4, 2024

JWT metadata broken in Envoyfilter since 1.21.0 #50246

Closed

2 tasks

AleksanderBrzozowski mentioned this pull request Jun 7, 2024

istio_authn filter disappeared after upgrading from 1.19.7 to 1.20.6 #51459

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

envoy: use upstream JWT filter instead of the custom Istio extension #47407

envoy: use upstream JWT filter instead of the custom Istio extension #47407

kyessenov commented Oct 17, 2023 •

edited

Loading

hzxuzhonghu Nov 9, 2023

kyessenov Nov 9, 2023

hzxuzhonghu Nov 9, 2023

kyessenov Nov 9, 2023

hzxuzhonghu Nov 9, 2023

hzxuzhonghu Nov 15, 2023

kyessenov Nov 15, 2023

hzxuzhonghu Nov 9, 2023

kyessenov Nov 14, 2023

hzxuzhonghu Nov 15, 2023

kyessenov Nov 15, 2023

kyessenov commented Nov 14, 2023

zirain left a comment

howardjohn Nov 15, 2023

kyessenov Nov 15, 2023 •

edited

Loading

howardjohn Nov 15, 2023

kyessenov Nov 15, 2023

kyessenov commented Nov 17, 2023

aryan16 Nov 17, 2023 •

edited

Loading

kyessenov Nov 17, 2023

aryan16 Nov 17, 2023 •

edited

Loading

kyessenov Nov 17, 2023

aryan16 Nov 17, 2023

aryan16 Nov 17, 2023

kyessenov Nov 17, 2023 •

edited

Loading

aryan16 Nov 17, 2023

aryan16 Nov 17, 2023

kyessenov Nov 17, 2023

aryan16 Nov 17, 2023

aryan16 Nov 17, 2023

aryan16 Nov 17, 2023

kyessenov commented Nov 20, 2023

kyessenov commented Nov 20, 2023

ericvn commented Nov 27, 2023

envoy: use upstream JWT filter instead of the custom Istio extension #47407

envoy: use upstream JWT filter instead of the custom Istio extension #47407

Conversation

kyessenov commented Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Nov 14, 2023

zirain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Nov 17, 2023

aryan16 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aryan16 Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyessenov commented Nov 20, 2023

kyessenov commented Nov 20, 2023

ericvn commented Nov 27, 2023

kyessenov commented Oct 17, 2023 •

edited

Loading

kyessenov Nov 15, 2023 •

edited

Loading

aryan16 Nov 17, 2023 •

edited

Loading

aryan16 Nov 17, 2023 •

edited

Loading

kyessenov Nov 17, 2023 •

edited

Loading