Set alpnOverride depending on TLS Mode #44918

ksubrmnn · 2023-05-15T17:02:16Z

Please provide a description of this PR:

By setting tls.mode SIMPLE, the user is signaling that they want to opt out of mTLS, Istio managed or otherwise. Since ALPN rewrite exists solely for the purpose of starting Istio mTLS, it violates the user’s intent to perform said rewrite.

This PR checks if the TLS mode is simple and then adds a field to the filter metadata to indicate whether the proxy should proceed with the alpn header rewrite.

Partially fixes #40680

Istio/proxy change: istio/proxy#4783

istio-testing · 2023-05-15T17:02:20Z

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

ksubrmnn · 2023-05-15T19:36:50Z

/retest

howardjohn · 2023-05-16T18:17:26Z

was there some istio/proxy change to enable this?

howardjohn · 2023-05-16T18:17:46Z

should add the test in #42902

ksubrmnn · 2023-05-16T19:31:57Z

was there some istio/proxy change to enable this?

#42902

Updated PR description to reflect that istio/proxy changes are to follow

ksubrmnn · 2023-05-16T19:33:44Z

#42902

I've included just a unit test for this change to verify that the filtermetada is set correctly. I will follow up with integration testing after the proxy changes.

pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go

pilot/pkg/networking/util/util.go

keithmattix

LGTM

howardjohn

Overall looks good, a few small comments. Its a bit hard to properly review without the istio/proxy changes though, ideally we merge those first.

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

howardjohn · 2023-05-31T14:37:13Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -294,6 +294,13 @@ func (cb *ClusterBuilder) applyDestinationRule(mc *MutableCluster, clusterMode C

 	if destRule != nil {
 		mc.cluster.Metadata = util.AddConfigInfoMetadata(mc.cluster.Metadata, destRule.Meta)
+
+		// ALPN header rewrite starts mTLS. Skip if TLS mode is SIMPLE.
+		trafficPolicy := destinationRule.GetTrafficPolicy()


I think we want to use opts.policy which is not the same as destinationRule. Actually I would do this inside of applyTrafficPolicy as well

what is the difference between destinationRule.GetTrafficPolicy() and opts.policy?

It is the result of MergeTrafficPolicy which does some merging logic

for applyTrafficPolicy, would we want to modify the opts.mutable.cluster.Metadata. If so, then we need to modify the return type which could mean making changes wherever it is called in the case that opts is nil

Why do we need to modify any return ttpes? Can't we just just set opts.mutable.cluster.metadata = util.AddALPNOverrideToMetadata(mc.cluster.Metadata, alpnOverride)?

howardjohn · 2023-05-31T14:39:00Z

pilot/pkg/networking/util/util.go

@@ -435,6 +435,30 @@ func AddSubsetToMetadata(md *core.Metadata, subset string) {
 	}
 }

+// AddALPNOverrideToMetadata adds whether the the ALPN prefix should be added to the header
+// to the given core.Metadata struct, if metadata is not initialized, build a new metadata.
+func AddALPNOverrideToMetadata(metadata *core.Metadata, alpnOverride bool) *core.Metadata {


can we just only include the metadata if alpnOverride is true (or false, I am not sure which one means "do not set it")?

Its not free to add the metadata, actually surprisingly high cost

Added return metadata if alpnOverride is true

added logic to configureALPNOverride

howardjohn · 2023-06-05T17:25:17Z

MUTUAL aspect is a TLS message: ClientCertificateType https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.4 It should not be tied to ALPN

…

On Mon, Jun 5, 2023 at 10:13 AM Keith Mattix II ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/networking/core/v1alpha3/cluster_builder.go <#44918 (comment)>: > @@ -294,6 +294,13 @@ func (cb *ClusterBuilder) applyDestinationRule(mc *MutableCluster, clusterMode C if destRule != nil { mc.cluster.Metadata = util.AddConfigInfoMetadata(mc.cluster.Metadata, destRule.Meta) + + // ALPN header rewrite starts mTLS. Skip if TLS mode is SIMPLE. FWIW, the original design doc was scoped to SIMPLE. MUTUAL probably makes sense semantically, but what's the existing mechanism for a tlsMode MUTUAL to indicate that it expects a client cert (outside of ALPN)? My concern is that it only works because we don't remove the alpn rewrite today — Reply to this email directly, view it on GitHub <#44918 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXNDVSTGWIBPYCOVI7TXJYHVDANCNFSM6AAAAAAYCPFM44> . You are receiving this because your review was requested.Message ID: ***@***.***>

jewertow · 2023-07-04T17:53:32Z

Hey @ksubrmnn, are you going to continue to work on this PR? I am interested in this change and I wonder if it will end up in master.

ksubrmnn · 2023-07-11T16:41:49Z

@jewertow Yes I'm working on this PR. Had some delay with proxy changes, but you can find them at istio/proxy#4783

ksubrmnn · 2023-07-13T19:48:01Z

WIP tests will fail since still working on applyTrafficPolicy part

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

keithmattix · 2023-07-13T23:00:11Z

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

@@ -1432,3 +1441,14 @@ func (cb *ClusterBuilder) buildExternalSDSCluster(addr string) *cluster.Cluster
 	}
 	return c
 }
+
+// configureALPNOverride determines whether alpn_override should be added to metadata
+func configureALPNOverride(tlsMode networking.ClientTLSSettings_TLSmode, md *core.Metadata) *core.Metadata {


I think you added a comment somewhere on the issue, but do you even need to return anything here since you're passing a pointer to md? In fact AddALPNOverrideToMetadata probably doesn't need a return value either if you're manipulating a pointer value

Yes you do because if the metadata is nil, we want to return a new metadata with the headers. That's the only reason. Empty metadata can be modified and set, but not nil.

Then why pass a pointer in the first place?

That's how it's stored in the mutable cluster.

pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go

pilot/pkg/networking/core/v1alpha3/cluster_builder.go

ksubrmnn · 2023-07-27T18:11:12Z

@howardjohn I've removed the changes to apply traffic policy. The PR was getting to be a bit large and unwieldy for me. I prefer to add those changes in a followup

Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>

keithmattix

Approving since changes are coming in a follow up PR

* Set alpnOverride Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> * remove test logs Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> --------- Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>

…e"` for SIMPLE and MUTUAL TLS (istio#833) * Set alpnOverride depending on TLS Mode (istio#44918) * Set alpnOverride Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> * remove test logs Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> --------- Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> * pilot: set `istio.alpn_override: false` in subset clusters (istio#46496) (istio#46529) * Add tests for destination rules with port level settings * Add test cases for subsets * Refactor AddALPNOverrideToMetadata * Update comment for AddALPNOverrideToMetadata * Fix lint error * Add release note * Fix release note --------- Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> --------- Signed-off-by: Kalya Subramanian <kasubra@microsoft.com> Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> Co-authored-by: Kalya Subramanian <42158129+ksubrmnn@users.noreply.github.com>

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 15, 2023

istio-policy-bot added area/networking release-notes-none Indicates a PR that does not require release notes. labels May 15, 2023

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

ksubrmnn force-pushed the alpn branch 2 times, most recently from 4bdbda5 to 6d0c539 Compare May 15, 2023 19:26

ksubrmnn force-pushed the alpn branch from 6d0c539 to e6325a0 Compare May 15, 2023 19:53

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 May 15, 2023

ksubrmnn changed the title ~~Set alpnOverride~~ Set alpnOverride depending on TLS Mode May 16, 2023

ksubrmnn marked this pull request as ready for review May 16, 2023 18:09

ksubrmnn requested review from a team as code owners May 16, 2023 18:09

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label May 16, 2023

keithmattix reviewed May 22, 2023

View reviewed changes

pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go Outdated Show resolved Hide resolved

pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go Outdated Show resolved Hide resolved

pilot/pkg/networking/util/util.go Outdated Show resolved Hide resolved

ksubrmnn force-pushed the alpn branch from 9233181 to fbbdcfe Compare May 31, 2023 01:44

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2023

keithmattix approved these changes May 31, 2023

View reviewed changes

howardjohn reviewed May 31, 2023

View reviewed changes

ksubrmnn mentioned this pull request Jul 6, 2023

Skip alpn rewrite if alpnOverride filter metadata is false istio/proxy#4783

Merged

ksubrmnn force-pushed the alpn branch from 03b3be2 to 7195108 Compare July 11, 2023 16:47

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 13, 2023

ksubrmnn added do-not-merge Block automatic merging of a PR. do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. and removed do-not-merge Block automatic merging of a PR. labels Jul 13, 2023

keithmattix requested changes Jul 13, 2023

View reviewed changes

jewertow reviewed Jul 19, 2023

View reviewed changes

pilot/pkg/networking/core/v1alpha3/cluster_builder.go Outdated Show resolved Hide resolved

pilot/pkg/networking/core/v1alpha3/cluster_builder.go Outdated Show resolved Hide resolved

ksubrmnn force-pushed the alpn branch from e3ca514 to c1e85e5 Compare July 27, 2023 17:34

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 27, 2023

Set alpnOverride

acb57f8

Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>

ksubrmnn force-pushed the alpn branch from ccca5da to acb57f8 Compare July 27, 2023 19:00

ksubrmnn removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 27, 2023

howardjohn approved these changes Jul 27, 2023

View reviewed changes

remove test logs

8be2a63

Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>

keithmattix approved these changes Jul 28, 2023

View reviewed changes

istio-testing merged commit bd9a7c4 into istio:master Jul 28, 2023

jewertow mentioned this pull request Aug 11, 2023

pilot: set istio.alpn_override: false in subset clusters #46496

Merged

Set alpnOverride depending on TLS Mode #44918

Set alpnOverride depending on TLS Mode #44918

Uh oh!

Conversation

ksubrmnn commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented May 15, 2023

Uh oh!

ksubrmnn commented May 15, 2023

Uh oh!

howardjohn commented May 16, 2023

Uh oh!

howardjohn commented May 16, 2023

Uh oh!

ksubrmnn commented May 16, 2023

Uh oh!

ksubrmnn commented May 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithmattix left a comment

Choose a reason for hiding this comment

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksubrmnn Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

howardjohn commented Jun 5, 2023 via email

Uh oh!

jewertow commented Jul 4, 2023

Uh oh!

ksubrmnn commented Jul 11, 2023

Uh oh!

ksubrmnn commented Jul 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ksubrmnn commented Jul 27, 2023

Uh oh!

keithmattix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ksubrmnn commented May 15, 2023 •

edited

Loading

ksubrmnn Jul 13, 2023 •

edited

Loading