From 23d9e1b74be665bee738b291eddc7dd550ae1bbc Mon Sep 17 00:00:00 2001 From: Faseela K Date: Mon, 4 Apr 2022 22:50:22 +0200 Subject: [PATCH 1/3] [WIP] Add auto-sni support Signed-off-by: Faseela K --- .../core/v1alpha3/cluster_builder.go | 22 ++++++ .../core/v1alpha3/cluster_builder_test.go | 71 +++++++++++++++++-- releasenotes/notes/auto-sni-support.yaml | 8 +++ 3 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 releasenotes/notes/auto-sni-support.yaml diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go index 978aeb5cbf40..977cb6ccbe98 100644 --- a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go +++ b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go @@ -1047,6 +1047,9 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts, CommonTlsContext: defaultUpstreamCommonTLSContext(), Sni: tls.Sni, } + // Set auto-sni if sni field is not explicitly set + cb.setAutoSni(c, tls) + // Use subject alt names specified in service entry if TLS settings does not have subject alt names. if opts.serviceRegistry == provider.External && len(tls.SubjectAltNames) == 0 { tls.SubjectAltNames = opts.serviceAccounts @@ -1083,6 +1086,10 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts, CommonTlsContext: defaultUpstreamCommonTLSContext(), Sni: tls.Sni, } + + // Set auto-sni if sni field is not explicitly set + cb.setAutoSni(c, tls) + // Use subject alt names specified in service entry if TLS settings does not have subject alt names. if opts.serviceRegistry == provider.External && len(tls.SubjectAltNames) == 0 { tls.SubjectAltNames = opts.serviceAccounts @@ -1130,6 +1137,21 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts, return tlsContext, nil } +func (cb *ClusterBuilder) setAutoSni(mc *MutableCluster, tls *networking.ClientTLSSettings) { + if mc == nil || features.VerifyCertAtClient { + return + } + if mc.httpProtocolOptions == nil { + mc.httpProtocolOptions = &http.HttpProtocolOptions{} + } + if mc.httpProtocolOptions.UpstreamHttpProtocolOptions == nil { + mc.httpProtocolOptions.UpstreamHttpProtocolOptions = &core.UpstreamHttpProtocolOptions{} + } + if len(tls.Sni) == 0 { + mc.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni = true + } +} + func (cb *ClusterBuilder) setUseDownstreamProtocol(mc *MutableCluster) { if mc.httpProtocolOptions == nil { mc.httpProtocolOptions = &http.HttpProtocolOptions{} diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go b/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go index a2112ddf6b5c..77a5a4e669a4 100644 --- a/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go +++ b/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go @@ -1774,12 +1774,13 @@ func TestBuildUpstreamClusterTLSContext(t *testing.T) { credentialName := "some-fake-credential" testCases := []struct { - name string - opts *buildClusterOpts - tls *networking.ClientTLSSettings - h2 bool - router bool - result expectedResult + name string + opts *buildClusterOpts + tls *networking.ClientTLSSettings + h2 bool + router bool + result expectedResult + enableVerifyCertAtClient bool }{ { name: "tls mode disabled", @@ -1967,6 +1968,56 @@ func TestBuildUpstreamClusterTLSContext(t *testing.T) { err: nil, }, }, + { + name: "tls mode SIMPLE, with VerifyCert enabled and no sni specified in tls", + opts: &buildClusterOpts{ + mutable: newTestCluster(), + }, + tls: &networking.ClientTLSSettings{ + Mode: networking.ClientTLSSettings_SIMPLE, + SubjectAltNames: []string{"SAN"}, + }, + result: expectedResult{ + tlsContext: &tls.UpstreamTlsContext{ + CommonTlsContext: &tls.CommonTlsContext{ + TlsParams: &tls.TlsParameters{ + // if not specified, envoy use TLSv1_2 as default for client. + TlsMaximumProtocolVersion: tls.TlsParameters_TLSv1_3, + TlsMinimumProtocolVersion: tls.TlsParameters_TLSv1_2, + }, + ValidationContextType: &tls.CommonTlsContext_ValidationContext{}, + }, + }, + err: nil, + }, + enableVerifyCertAtClient: true, + }, + { + name: "tls mode SIMPLE, with VerifyCert enabled and sni specified in tls", + opts: &buildClusterOpts{ + mutable: newTestCluster(), + }, + tls: &networking.ClientTLSSettings{ + Mode: networking.ClientTLSSettings_SIMPLE, + SubjectAltNames: []string{"SAN"}, + Sni: "some-sni.com", + }, + result: expectedResult{ + tlsContext: &tls.UpstreamTlsContext{ + CommonTlsContext: &tls.CommonTlsContext{ + TlsParams: &tls.TlsParameters{ + // if not specified, envoy use TLSv1_2 as default for client. + TlsMaximumProtocolVersion: tls.TlsParameters_TLSv1_3, + TlsMinimumProtocolVersion: tls.TlsParameters_TLSv1_2, + }, + ValidationContextType: &tls.CommonTlsContext_ValidationContext{}, + }, + Sni: "some-sni.com", + }, + err: nil, + }, + enableVerifyCertAtClient: true, + }, { name: "tls mode SIMPLE, with certs specified in tls", opts: &buildClusterOpts{ @@ -2512,6 +2563,7 @@ func TestBuildUpstreamClusterTLSContext(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { + features.VerifyCertAtClient = tc.enableVerifyCertAtClient var proxy *model.Proxy if tc.router { proxy = newGatewayProxy() @@ -2528,6 +2580,13 @@ func TestBuildUpstreamClusterTLSContext(t *testing.T) { } else if diff := cmp.Diff(tc.result.tlsContext, ret, protocmp.Transform()); diff != "" { t.Errorf("got diff: `%v", diff) } + if tc.enableVerifyCertAtClient { + if len(tc.tls.Sni) == 0 { + assert.Equal(t, tc.opts.mutable.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni, true) + } else { + assert.Equal(t, tc.opts.mutable.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni, false) + } + } }) } } diff --git a/releasenotes/notes/auto-sni-support.yaml b/releasenotes/notes/auto-sni-support.yaml new file mode 100644 index 000000000000..820ae7340add --- /dev/null +++ b/releasenotes/notes/auto-sni-support.yaml @@ -0,0 +1,8 @@ +apiVersion: release-notes/v2 +kind: feature +area: traffic-management +releaseNotes: + - | + **Added** Auto-Sni Support. +docs: + - https://docs.google.com/document/d/1pTUl-Ng3nXAWJb7UGJtalftznpxQEfID/ From cf4fbc93ca5ea9d81f2375678badc87b9746ea8f Mon Sep 17 00:00:00 2001 From: Faseela K Date: Tue, 5 Apr 2022 11:52:23 +0200 Subject: [PATCH 2/3] Fix unit test failure Signed-off-by: Faseela K --- .../core/v1alpha3/cluster_builder.go | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go index 977cb6ccbe98..7dc9a139ed40 100644 --- a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go +++ b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go @@ -1138,17 +1138,16 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts, } func (cb *ClusterBuilder) setAutoSni(mc *MutableCluster, tls *networking.ClientTLSSettings) { - if mc == nil || features.VerifyCertAtClient { - return - } - if mc.httpProtocolOptions == nil { - mc.httpProtocolOptions = &http.HttpProtocolOptions{} - } - if mc.httpProtocolOptions.UpstreamHttpProtocolOptions == nil { - mc.httpProtocolOptions.UpstreamHttpProtocolOptions = &core.UpstreamHttpProtocolOptions{} - } - if len(tls.Sni) == 0 { - mc.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni = true + if mc != nil && features.VerifyCertAtClient { + if mc.httpProtocolOptions == nil { + mc.httpProtocolOptions = &http.HttpProtocolOptions{} + } + if mc.httpProtocolOptions.UpstreamHttpProtocolOptions == nil { + mc.httpProtocolOptions.UpstreamHttpProtocolOptions = &core.UpstreamHttpProtocolOptions{} + } + if len(tls.Sni) == 0 { + mc.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni = true + } } } From d3462a1635913eed881d0aa42596bad7f0ec3a70 Mon Sep 17 00:00:00 2001 From: Faseela K Date: Wed, 6 Apr 2022 16:46:49 +0200 Subject: [PATCH 3/3] enhance release-notes and small code improvements Signed-off-by: Faseela K --- pilot/pkg/networking/core/v1alpha3/cluster_builder.go | 6 ++---- pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go | 4 ++-- releasenotes/notes/auto-sni-support.yaml | 3 ++- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go index 7dc9a139ed40..171ba827af47 100644 --- a/pilot/pkg/networking/core/v1alpha3/cluster_builder.go +++ b/pilot/pkg/networking/core/v1alpha3/cluster_builder.go @@ -1138,16 +1138,14 @@ func (cb *ClusterBuilder) buildUpstreamClusterTLSContext(opts *buildClusterOpts, } func (cb *ClusterBuilder) setAutoSni(mc *MutableCluster, tls *networking.ClientTLSSettings) { - if mc != nil && features.VerifyCertAtClient { + if mc != nil && features.VerifyCertAtClient && len(tls.Sni) == 0 { if mc.httpProtocolOptions == nil { mc.httpProtocolOptions = &http.HttpProtocolOptions{} } if mc.httpProtocolOptions.UpstreamHttpProtocolOptions == nil { mc.httpProtocolOptions.UpstreamHttpProtocolOptions = &core.UpstreamHttpProtocolOptions{} } - if len(tls.Sni) == 0 { - mc.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni = true - } + mc.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni = true } } diff --git a/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go b/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go index 77a5a4e669a4..9013e1522297 100644 --- a/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go +++ b/pilot/pkg/networking/core/v1alpha3/cluster_builder_test.go @@ -2583,8 +2583,8 @@ func TestBuildUpstreamClusterTLSContext(t *testing.T) { if tc.enableVerifyCertAtClient { if len(tc.tls.Sni) == 0 { assert.Equal(t, tc.opts.mutable.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni, true) - } else { - assert.Equal(t, tc.opts.mutable.httpProtocolOptions.UpstreamHttpProtocolOptions.AutoSni, false) + } else if tc.opts.mutable.httpProtocolOptions != nil { + t.Errorf("expecting nil httpProtocolOptions but got %v", tc.opts.mutable.httpProtocolOptions) } } }) diff --git a/releasenotes/notes/auto-sni-support.yaml b/releasenotes/notes/auto-sni-support.yaml index 820ae7340add..0782cdc409bc 100644 --- a/releasenotes/notes/auto-sni-support.yaml +++ b/releasenotes/notes/auto-sni-support.yaml @@ -3,6 +3,7 @@ kind: feature area: traffic-management releaseNotes: - | - **Added** Auto-Sni Support. + **Added** the ability to configure automatically set SNI when `DestinationRules` + do not specify it and `VERIFY_CLIENT_AT_CERT` is enabled. docs: - https://docs.google.com/document/d/1pTUl-Ng3nXAWJb7UGJtalftznpxQEfID/