-
Notifications
You must be signed in to change notification settings - Fork 14
fix(login): prevent panic with --ignore-ssl-errors on subsequent runs #549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
0fc648c
958d271
2b51718
316f0b2
dc91df9
4847d32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,39 @@ | ||||||||||||||||||||||||||||||||||||||
| package apiclient | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||
| "crypto/tls" | ||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // ApplySSLIgnoreConfiguration configures the HTTP client to ignore SSL errors | ||||||||||||||||||||||||||||||||||||||
| // by setting InsecureSkipVerify on the underlying transport. This function | ||||||||||||||||||||||||||||||||||||||
| // handles multiple transport types: | ||||||||||||||||||||||||||||||||||||||
| // - Direct *http.Transport | ||||||||||||||||||||||||||||||||||||||
| // - *SpinnerRoundTripper wrapping *http.Transport | ||||||||||||||||||||||||||||||||||||||
| // - Any other transport type (fallback replacement) | ||||||||||||||||||||||||||||||||||||||
| func ApplySSLIgnoreConfiguration(httpClient *http.Client) { | ||||||||||||||||||||||||||||||||||||||
| if httpClient.Transport == nil { | ||||||||||||||||||||||||||||||||||||||
| httpClient.Transport = &http.Transport{} | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Handle both direct http.Transport and SpinnerRoundTripper wrapping http.Transport | ||||||||||||||||||||||||||||||||||||||
| switch transport := httpClient.Transport.(type) { | ||||||||||||||||||||||||||||||||||||||
| case *http.Transport: | ||||||||||||||||||||||||||||||||||||||
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | ||||||||||||||||||||||||||||||||||||||
| case *SpinnerRoundTripper: | ||||||||||||||||||||||||||||||||||||||
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | ||||||||||||||||||||||||||||||||||||||
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | ||||||||||||||||||||||||||||||||||||||
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| case *SpinnerRoundTripper: | |
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | |
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | |
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| if transport.TLSClientConfig != nil { | |
| transport.TLSClientConfig.InsecureSkipVerify = true | |
| } else { | |
| transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| } | |
| case *SpinnerRoundTripper: | |
| // If the SpinnerRoundTripper's Next is an http.Transport, configure it | |
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | |
| if httpTransport.TLSClientConfig != nil { | |
| httpTransport.TLSClientConfig.InsecureSkipVerify = true | |
| } else { | |
| httpTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified and tested
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package login_test | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "testing" | ||
|
|
||
| "github.com/OctopusDeploy/cli/pkg/apiclient" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // TestSSLIgnoreHandling tests that our SSL ignore logic works with both | ||
| // direct http.Transport and SpinnerRoundTripper scenarios | ||
| func TestSSLIgnoreHandling(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| transport http.RoundTripper | ||
| expectPanic bool | ||
| }{ | ||
| { | ||
| name: "Direct http.Transport should work", | ||
| transport: &http.Transport{}, | ||
| expectPanic: false, | ||
| }, | ||
| { | ||
| name: "SpinnerRoundTripper with http.Transport should work", | ||
| transport: &apiclient.SpinnerRoundTripper{Next: &http.Transport{}}, | ||
| expectPanic: false, | ||
| }, | ||
| { | ||
| name: "SpinnerRoundTripper with default transport should work", | ||
| transport: apiclient.NewSpinnerRoundTripper(), | ||
| expectPanic: false, | ||
| }, | ||
| { | ||
| name: "nil transport should work", | ||
| transport: nil, | ||
| expectPanic: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| client := &http.Client{Transport: tt.transport} | ||
|
|
||
| // This simulates the SSL ignore logic from loginRun function | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| if !tt.expectPanic { | ||
| t.Errorf("Unexpected panic: %v", r) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| // Apply the SSL ignore logic using the shared utility | ||
| apiclient.ApplySSLIgnoreConfiguration(client) | ||
|
|
||
| // Verify the SSL configuration was applied correctly | ||
| verifySSLConfig(t, client) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // verifySSLConfig checks that the SSL configuration was applied correctly | ||
| func verifySSLConfig(t *testing.T, httpClient *http.Client) { | ||
| assert.NotNil(t, httpClient.Transport, "Transport should not be nil") | ||
|
|
||
| switch transport := httpClient.Transport.(type) { | ||
| case *http.Transport: | ||
| assert.NotNil(t, transport.TLSClientConfig, "TLS config should be set") | ||
| assert.True(t, transport.TLSClientConfig.InsecureSkipVerify, "InsecureSkipVerify should be true") | ||
|
|
||
| case *apiclient.SpinnerRoundTripper: | ||
| assert.NotNil(t, transport.Next, "SpinnerRoundTripper.Next should not be nil") | ||
|
|
||
| if httpTransport, ok := transport.Next.(*http.Transport); ok { | ||
| assert.NotNil(t, httpTransport.TLSClientConfig, "Underlying TLS config should be set") | ||
| assert.True(t, httpTransport.TLSClientConfig.InsecureSkipVerify, "Underlying InsecureSkipVerify should be true") | ||
| } else { | ||
| t.Errorf("SpinnerRoundTripper.Next should be *http.Transport, got %T", transport.Next) | ||
| } | ||
|
|
||
| default: | ||
| t.Errorf("Unexpected transport type: %T", transport) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.