From bd7d09255beb8726e0c0bfbea37670ff3d5175f8 Mon Sep 17 00:00:00 2001 From: EXEC Date: Sat, 19 Mar 2022 17:48:30 +0800 Subject: [PATCH 1/7] Fix panic in etcd validate secure endpoints #13810 `ValidateSecureEndpoints()` should call `t.DialContext()` instead of `t.Dial()`, because `t.Dial` is `nil` --- client/pkg/transport/tls.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/pkg/transport/tls.go b/client/pkg/transport/tls.go index 62fe0d385..d99b772e3 100644 --- a/client/pkg/transport/tls.go +++ b/client/pkg/transport/tls.go @@ -15,6 +15,7 @@ package transport import ( + "context" "fmt" "strings" "time" @@ -34,7 +35,7 @@ func ValidateSecureEndpoints(tlsInfo TLSInfo, eps []string) ([]string, error) { errs = append(errs, fmt.Sprintf("%q is insecure", ep)) continue } - conn, cerr := t.Dial("tcp", ep[len("https://"):]) + conn, cerr := t.DialContext(context.Background(), "tcp", ep[len("https://"):]) if cerr != nil { errs = append(errs, fmt.Sprintf("%q failed to dial (%v)", ep, cerr)) continue From 983ee82c98570069f94287d11494cfb26aac90fc Mon Sep 17 00:00:00 2001 From: eval-exec Date: Sun, 20 Mar 2022 09:38:24 +0800 Subject: [PATCH 2/7] add test for transport/tls.go:ValidateSecureEndpoints() --- client/pkg/transport/tls_test.go | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 client/pkg/transport/tls_test.go diff --git a/client/pkg/transport/tls_test.go b/client/pkg/transport/tls_test.go new file mode 100644 index 000000000..4e7b4a21b --- /dev/null +++ b/client/pkg/transport/tls_test.go @@ -0,0 +1,36 @@ +package transport + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestValidateSecureEndpoints(t *testing.T) { + tlsInfo, err := createSelfCert(t) + if err != nil { + t.Fatalf("unable to create cert: %v", err) + } + + remoteAddr := func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(r.RemoteAddr)) + } + srv := httptest.NewServer(http.HandlerFunc(remoteAddr)) + defer srv.Close() + + insecureEps := []string{ + "http://" + srv.Listener.Addr().String(), + "invalid remote address", + } + if _, err := ValidateSecureEndpoints(*tlsInfo, insecureEps); err == nil || !strings.Contains(err.Error(), "is insecure") { + t.Error("validate secure endpoints should fail") + } + + secureEps := []string{ + "https://" + srv.Listener.Addr().String(), + } + if _, err := ValidateSecureEndpoints(*tlsInfo, secureEps); err != nil { + t.Error("validate secure endpoints should succeed") + } +} From df71f59c0e4bc5f2f9b03cd1f28c98010501de9b Mon Sep 17 00:00:00 2001 From: eval-exec Date: Sun, 20 Mar 2022 21:23:20 +0800 Subject: [PATCH 3/7] close idle connections --- client/pkg/transport/tls.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/pkg/transport/tls.go b/client/pkg/transport/tls.go index d99b772e3..8c3a35b14 100644 --- a/client/pkg/transport/tls.go +++ b/client/pkg/transport/tls.go @@ -28,6 +28,8 @@ func ValidateSecureEndpoints(tlsInfo TLSInfo, eps []string) ([]string, error) { if err != nil { return nil, err } + defer t.CloseIdleConnections() + var errs []string var endpoints []string for _, ep := range eps { From 4786a72cfc075fb7440636e65a996fca367cd394 Mon Sep 17 00:00:00 2001 From: EXEC Date: Mon, 21 Mar 2022 21:59:28 +0800 Subject: [PATCH 4/7] Update client/pkg/transport/tls_test.go Co-authored-by: Marek Siarkowicz --- client/pkg/transport/tls_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/client/pkg/transport/tls_test.go b/client/pkg/transport/tls_test.go index 4e7b4a21b..e2747f87b 100644 --- a/client/pkg/transport/tls_test.go +++ b/client/pkg/transport/tls_test.go @@ -1,3 +1,17 @@ +// Copyright 2022 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package transport import ( From 88e1d6b126ad38809503893ad31b5bd64501078a Mon Sep 17 00:00:00 2001 From: eval-exec Date: Mon, 21 Mar 2022 22:36:39 +0800 Subject: [PATCH 5/7] using subtests for TestValidateSecureEndpoints() --- client/pkg/transport/tls_test.go | 72 ++++++++++++++++++++++++++------ 1 file changed, 59 insertions(+), 13 deletions(-) diff --git a/client/pkg/transport/tls_test.go b/client/pkg/transport/tls_test.go index e2747f87b..aa2615257 100644 --- a/client/pkg/transport/tls_test.go +++ b/client/pkg/transport/tls_test.go @@ -17,7 +17,7 @@ package transport import ( "net/http" "net/http/httptest" - "strings" + "reflect" "testing" ) @@ -33,18 +33,64 @@ func TestValidateSecureEndpoints(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(remoteAddr)) defer srv.Close() - insecureEps := []string{ - "http://" + srv.Listener.Addr().String(), - "invalid remote address", + tests := map[string]struct { + endPoints []string + expectedEndpoints []string + expectedErr bool + }{ + "invalidEndPoints": { + endPoints: []string{ + "invalid endpoint", + }, + expectedEndpoints: nil, + expectedErr: true, + }, + "insecureEndpoints": { + endPoints: []string{ + "http://127.0.0.1:8000", + "http://" + srv.Listener.Addr().String(), + }, + expectedEndpoints: nil, + expectedErr: true, + }, + "secureEndPoints": { + endPoints: []string{ + "https://" + srv.Listener.Addr().String(), + }, + expectedEndpoints: []string{ + "https://" + srv.Listener.Addr().String(), + }, + expectedErr: false, + }, + "mixEndPoints": { + endPoints: []string{ + "https://" + srv.Listener.Addr().String(), + "http://" + srv.Listener.Addr().String(), + "invalid end points", + }, + expectedEndpoints: []string{ + "https://" + srv.Listener.Addr().String(), + }, + expectedErr: true, + }, } - if _, err := ValidateSecureEndpoints(*tlsInfo, insecureEps); err == nil || !strings.Contains(err.Error(), "is insecure") { - t.Error("validate secure endpoints should fail") - } - - secureEps := []string{ - "https://" + srv.Listener.Addr().String(), - } - if _, err := ValidateSecureEndpoints(*tlsInfo, secureEps); err != nil { - t.Error("validate secure endpoints should succeed") + for name, test := range tests { + t.Run(name, func(t *testing.T) { + secureEps, err := ValidateSecureEndpoints(*tlsInfo, test.endPoints) + if test.expectedErr && err == nil { + t.Errorf("expected error") + } + if !test.expectedErr && err != nil { + t.Errorf("unexpected error: %v", err) + } + if err == nil && !test.expectedErr { + if len(secureEps) != len(test.expectedEndpoints) { + t.Errorf("expected %v endpoints, got %v", len(test.expectedEndpoints), len(secureEps)) + } + if !reflect.DeepEqual(test.expectedEndpoints, secureEps) { + t.Errorf("expected endpoints %v, got %v", test.expectedEndpoints, secureEps) + } + } + }) } } From ad78a74c42a27c9fc0a410487adbad1ac7c3f3e3 Mon Sep 17 00:00:00 2001 From: EXEC Date: Mon, 21 Mar 2022 23:06:29 +0800 Subject: [PATCH 6/7] Update client/pkg/transport/tls_test.go Co-authored-by: Marek Siarkowicz --- client/pkg/transport/tls_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/client/pkg/transport/tls_test.go b/client/pkg/transport/tls_test.go index aa2615257..50e864f22 100644 --- a/client/pkg/transport/tls_test.go +++ b/client/pkg/transport/tls_test.go @@ -77,12 +77,10 @@ func TestValidateSecureEndpoints(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { secureEps, err := ValidateSecureEndpoints(*tlsInfo, test.endPoints) - if test.expectedErr && err == nil { - t.Errorf("expected error") - } - if !test.expectedErr && err != nil { - t.Errorf("unexpected error: %v", err) + if test.expectedErr != (err != nil) { + t.Errorf("Unexpected error, got: %v, want: %v", err, test.expectedError) } + if err == nil && !test.expectedErr { if len(secureEps) != len(test.expectedEndpoints) { t.Errorf("expected %v endpoints, got %v", len(test.expectedEndpoints), len(secureEps)) From 8d01ac28162d79fd11ce71edad093370b190b0e4 Mon Sep 17 00:00:00 2001 From: eval-exec Date: Mon, 21 Mar 2022 23:08:35 +0800 Subject: [PATCH 7/7] remove endpoints length check in TestValidateSecureEndpoints() --- client/pkg/transport/tls_test.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/client/pkg/transport/tls_test.go b/client/pkg/transport/tls_test.go index 50e864f22..46af1db67 100644 --- a/client/pkg/transport/tls_test.go +++ b/client/pkg/transport/tls_test.go @@ -78,16 +78,11 @@ func TestValidateSecureEndpoints(t *testing.T) { t.Run(name, func(t *testing.T) { secureEps, err := ValidateSecureEndpoints(*tlsInfo, test.endPoints) if test.expectedErr != (err != nil) { - t.Errorf("Unexpected error, got: %v, want: %v", err, test.expectedError) + t.Errorf("Unexpected error, got: %v, want: %v", err, test.expectedErr) } - if err == nil && !test.expectedErr { - if len(secureEps) != len(test.expectedEndpoints) { - t.Errorf("expected %v endpoints, got %v", len(test.expectedEndpoints), len(secureEps)) - } - if !reflect.DeepEqual(test.expectedEndpoints, secureEps) { - t.Errorf("expected endpoints %v, got %v", test.expectedEndpoints, secureEps) - } + if !reflect.DeepEqual(test.expectedEndpoints, secureEps) { + t.Errorf("expected endpoints %v, got %v", test.expectedEndpoints, secureEps) } }) }