Fix panic in etcd validate secure endpoints #13810 #13824

eval-exec · 2022-03-19T09:50:36Z

This pr fix #13810

Lines 33 to 43 in 1b208aa

    
           t := &http.Transport{ 
        
           	Proxy: http.ProxyFromEnvironment, 
        
           	DialContext: (&net.Dialer{ 
        
           		Timeout: dialtimeoutd, 
        
           		// value taken from http.DefaultTransport 
        
           		KeepAlive: 30 * time.Second, 
        
           	}).DialContext, 
        
           	// value taken from http.DefaultTransport 
        
           	TLSHandshakeTimeout: 10 * time.Second, 
        
           	TLSClientConfig:     cfg, 
        
           }

In etcd/client/pkg/transport/tls.go:
ValidateSecureEndpoints() should call t.DialContext() instead of t.Dial(), because t.Dial is nil

serathius · 2022-03-19T19:58:50Z

Can you add a test?

codecov-commenter · 2022-03-19T20:30:37Z

Codecov Report

Merging #13824 (e2e38bf) into main (1b208aa) will decrease coverage by 0.32%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13824      +/-   ##
==========================================
- Coverage   72.72%   72.39%   -0.33%     
==========================================
  Files         467      467              
  Lines       38280    38282       +2     
==========================================
- Hits        27839    27716     -123     
- Misses       8662     8766     +104     
- Partials     1779     1800      +21

Flag	Coverage Δ
all	`72.39% <ø> (-0.33%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/compaction_command.go	`30.43% <0.00%> (-43.48%)`	⬇️
etcdctl/ctlv3/command/alarm_command.go	`51.35% <0.00%> (-27.03%)`	⬇️
server/storage/quota.go	`57.89% <0.00%> (-9.22%)`	⬇️
server/auth/simple_token.go	`80.00% <0.00%> (-8.47%)`	⬇️
server/storage/mvcc/watchable_store.go	`85.14% <0.00%> (-7.61%)`	⬇️
client/pkg/v3/fileutil/purge.go	`66.03% <0.00%> (-7.55%)`	⬇️
server/etcdserver/api/v3rpc/lease.go	`77.21% <0.00%> (-5.07%)`	⬇️
client/v3/concurrency/session.go	`88.63% <0.00%> (-4.55%)`	⬇️
client/pkg/v3/testutil/leak.go	`62.83% <0.00%> (-4.43%)`	⬇️
client/v3/leasing/cache.go	`87.77% <0.00%> (-3.89%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b208aa...e2e38bf. Read the comment docs.

eval-exec · 2022-03-19T23:42:36Z

Can you add a test?

Yes, I will add a test today.

eval-exec · 2022-03-20T01:49:01Z

Can you add a test?

Test for ValidateSecureEndpoints() added.

serathius

LGTM, confirmed that Dial is deprecated and not used by NewTransport

serathius · 2022-03-21T10:02:08Z

cc @ahrtr @ptabor @spzala

client/pkg/transport/tls_test.go

serathius · 2022-03-21T10:07:47Z

client/pkg/transport/tls_test.go

+		t.Error("validate secure endpoints should fail")
+	}
+
+	secureEps := []string{


Looks like you sequentially test different cases, fist insecure later secure, also you don't have you considered using subtests?

OK, I will using subtests.

serathius · 2022-03-21T10:08:23Z

client/pkg/transport/tls_test.go

+		"http://" + srv.Listener.Addr().String(),
+		"invalid remote address",
+	}
+	if _, err := ValidateSecureEndpoints(*tlsInfo, insecureEps); err == nil || !strings.Contains(err.Error(), "is insecure") {


Can you test case with insecure endpoint that passes?

Test for ValidateSecureEndpoints() updated.

ahrtr

LGTM

The only minor concern is the unit test. subtest is better, but since there is only two cases, and they share the same http server, so I am OK.

serathius · 2022-03-21T14:47:36Z

client/pkg/transport/tls_test.go

+			if !test.expectedErr && err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if err == nil && !test.expectedErr {


This if is not needed, you can still check endpoints. in case mixEndPoints you specified both expectedEndpoints and expectErr true, which is misleading as this if will result in endpoints not being verified.

client/pkg/transport/tls_test.go

serathius · 2022-03-21T14:50:27Z

client/pkg/transport/tls_test.go

+				t.Errorf("unexpected error: %v", err)
+			}
+			if err == nil && !test.expectedErr {
+				if len(secureEps) != len(test.expectedEndpoints) {


DeepEqual should already test if length matches.

Thank you, I removed secureEps's length check.

spzala

lgtm
Running workflow. Thanks @eval-exec

eval-exec · 2022-03-22T12:37:20Z

@serathius I rebased branch (eval-exec:patch-1), please approve running workflows.

`ValidateSecureEndpoints()` should call `t.DialContext()` instead of `t.Dial()`, because `t.Dial` is `nil`

Co-authored-by: Marek Siarkowicz <[email protected]>

Backport: etcd-io#13824 Signed-off-by: Wei Fu <[email protected]>

eval-exec force-pushed the patch-1 branch from 3dc2f7f to 652ed9d Compare March 19, 2022 09:54

serathius approved these changes Mar 21, 2022

View reviewed changes

serathius reviewed Mar 21, 2022

View reviewed changes

client/pkg/transport/tls_test.go Show resolved Hide resolved

serathius reviewed Mar 21, 2022

View reviewed changes

ahrtr approved these changes Mar 21, 2022

View reviewed changes

serathius reviewed Mar 21, 2022

View reviewed changes

client/pkg/transport/tls_test.go Outdated Show resolved Hide resolved

serathius reviewed Mar 21, 2022

View reviewed changes

eval-exec force-pushed the patch-1 branch from 90c128e to e2e38bf Compare March 21, 2022 15:11

spzala approved these changes Mar 21, 2022

View reviewed changes

serathius approved these changes Mar 22, 2022

View reviewed changes

serathius added the stage/merge-when-tests-green label Mar 22, 2022

eval-exec force-pushed the patch-1 branch from f3399b7 to 95b6a86 Compare March 22, 2022 12:25

eval-exec force-pushed the patch-1 branch from 95b6a86 to d5875ac Compare March 23, 2022 14:34

eval-exec and others added 7 commits March 23, 2022 23:37

Fix panic in etcd validate secure endpoints etcd-io#13810

bd7d092

`ValidateSecureEndpoints()` should call `t.DialContext()` instead of `t.Dial()`, because `t.Dial` is `nil`

add test for transport/tls.go:ValidateSecureEndpoints()

983ee82

close idle connections

df71f59

Update client/pkg/transport/tls_test.go

4786a72

Co-authored-by: Marek Siarkowicz <[email protected]>

using subtests for TestValidateSecureEndpoints()

88e1d6b

Update client/pkg/transport/tls_test.go

ad78a74

Co-authored-by: Marek Siarkowicz <[email protected]>

remove endpoints length check in TestValidateSecureEndpoints()

8d01ac2

eval-exec force-pushed the patch-1 branch from d5875ac to 8d01ac2 Compare March 23, 2022 15:37

serathius merged commit cc33b7c into etcd-io:main Mar 24, 2022

vivekpatani mentioned this pull request Aug 3, 2022

Plans for v3.4.20 release #14232

Closed

25 tasks

fuweid added a commit to fuweid/etcd that referenced this pull request Sep 9, 2023

Fix panic in etcd validate secure endpoints

7816df7

Backport: etcd-io#13824 Signed-off-by: Wei Fu <[email protected]>

fuweid mentioned this pull request Sep 9, 2023

[3.5] Fix panic in etcd validate secure endpoints #16565

Merged

ahrtr mentioned this pull request Oct 10, 2023

etcd (3.5.9) gateway with discovery-srv crashes with "invalid memory address or nil pointer dereference" #16736

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panic in etcd validate secure endpoints #13810 #13824

Fix panic in etcd validate secure endpoints #13810 #13824

eval-exec commented Mar 19, 2022 •

edited

Loading

serathius commented Mar 19, 2022

codecov-commenter commented Mar 19, 2022 •

edited

Loading

eval-exec commented Mar 19, 2022

eval-exec commented Mar 20, 2022

serathius left a comment

serathius commented Mar 21, 2022

serathius Mar 21, 2022

eval-exec Mar 21, 2022

serathius Mar 21, 2022

eval-exec Mar 21, 2022

ahrtr left a comment •

edited

Loading

serathius Mar 21, 2022

serathius Mar 21, 2022

eval-exec Mar 21, 2022

spzala left a comment

eval-exec commented Mar 22, 2022

	t := &http.Transport{
	Proxy: http.ProxyFromEnvironment,
	DialContext: (&net.Dialer{
	Timeout: dialtimeoutd,
	// value taken from http.DefaultTransport
	KeepAlive: 30 * time.Second,
	}).DialContext,
	// value taken from http.DefaultTransport
	TLSHandshakeTimeout: 10 * time.Second,
	TLSClientConfig: cfg,
	}

Fix panic in etcd validate secure endpoints #13810 #13824

Fix panic in etcd validate secure endpoints #13810 #13824

Conversation

eval-exec commented Mar 19, 2022 • edited Loading

serathius commented Mar 19, 2022

codecov-commenter commented Mar 19, 2022 • edited Loading

Codecov Report

eval-exec commented Mar 19, 2022

eval-exec commented Mar 20, 2022

serathius left a comment

Choose a reason for hiding this comment

serathius commented Mar 21, 2022

serathius Mar 21, 2022

Choose a reason for hiding this comment

eval-exec Mar 21, 2022

Choose a reason for hiding this comment

serathius Mar 21, 2022

Choose a reason for hiding this comment

eval-exec Mar 21, 2022

Choose a reason for hiding this comment

ahrtr left a comment • edited Loading

Choose a reason for hiding this comment

serathius Mar 21, 2022

Choose a reason for hiding this comment

serathius Mar 21, 2022

Choose a reason for hiding this comment

eval-exec Mar 21, 2022

Choose a reason for hiding this comment

spzala left a comment

Choose a reason for hiding this comment

eval-exec commented Mar 22, 2022

eval-exec commented Mar 19, 2022 •

edited

Loading

codecov-commenter commented Mar 19, 2022 •

edited

Loading

ahrtr left a comment •

edited

Loading