fix: export ConfigureTLS helper for wrapped transports#73
Draft
fix: export ConfigureTLS helper for wrapped transports#73
Conversation
Export ConfigureTLS as a public helper so wrapped transports can reuse the TLS configuration logic. Harden the function against invalid input and resource leaks, skip dead root CAs when a fingerprint is set, and use a context-aware TLS dialer.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
Benchmark Resultsubuntu-latestwindows-latestmacos-latestCompared using benchstat. Benchmarks were run with |
There was a problem hiding this comment.
Pull request overview
This PR refactors TLS setup in elastictransport by extracting the TLS configuration logic from New() into a new exported helper, ConfigureTLS, intended for reuse when *http.Transport is wrapped by custom http.RoundTripper middleware.
Changes:
- Adds
ConfigureTLS(*http.Transport, caCert, certificateFingerprint)in a newtls.go. - Updates
New()to clone/configure*http.TransportviaConfigureTLS, and error when TLS options are used with a non-*http.Transport. - Adds unit tests covering the new helper and the updated
New()TLS integration paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
elastictransport/tls.go |
Introduces exported ConfigureTLS helper and fingerprint/CA configuration logic. |
elastictransport/elastictransport.go |
Replaces inline TLS configuration in New() with a call to ConfigureTLS and adds a non-*http.Transport error path. |
elastictransport/elastictransport_internal_test.go |
Adds tests for ConfigureTLS and TLS configuration behavior in New(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fingerprint, err := hex.DecodeString(certificateFingerprint) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid certificate fingerprint %q: %w", certificateFingerprint, err) | ||
| } |
There was a problem hiding this comment.
certificateFingerprint is decoded from hex but the decoded length is never validated. Since the comparison is bytes.Equal(sha256Sum[:], fingerprint), any decoded length other than 32 bytes will never match and will cause all TLS dials to fail later with a mismatch error. Consider validating len(fingerprint)==sha256.Size (and optionally rejecting odd lengths / whitespace) and returning a clear error early.
Suggested change
| } | |
| } | |
| if len(fingerprint) != sha256.Size { | |
| return fmt.Errorf("invalid certificate fingerprint %q: expected %d-byte SHA-256 digest, got %d bytes", certificateFingerprint, sha256.Size, len(fingerprint)) | |
| } |
Comment on lines
+54
to
+69
| transport.DialTLSContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| d := tls.Dialer{Config: &tls.Config{InsecureSkipVerify: true}} | ||
| conn, err := d.DialContext(ctx, network, addr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tlsConn := conn.(*tls.Conn) | ||
| for _, cert := range tlsConn.ConnectionState().PeerCertificates { | ||
| digest := sha256.Sum256(cert.Raw) | ||
| if bytes.Equal(digest[:], fingerprint) { | ||
| return tlsConn, nil | ||
| } | ||
| } | ||
| _ = tlsConn.Close() | ||
| return nil, fmt.Errorf("fingerprint mismatch, provided: %s", certificateFingerprint) |
There was a problem hiding this comment.
Using Transport.DialTLSContext for fingerprint pinning means the fingerprint verification only runs for non-proxied HTTPS connections, and it also bypasses transport.TLSClientConfig settings (e.g., ALPN NextProtos for HTTP/2, min TLS version, cipher suites). To ensure fingerprint pinning is consistently enforced (including proxied HTTPS) and to preserve TLS settings, consider implementing pinning via transport.TLSClientConfig (e.g., InsecureSkipVerify=true plus VerifyPeerCertificate/VerifyConnection) instead of a custom dialer.
Suggested change
| transport.DialTLSContext = func(ctx context.Context, network, addr string) (net.Conn, error) { | |
| d := tls.Dialer{Config: &tls.Config{InsecureSkipVerify: true}} | |
| conn, err := d.DialContext(ctx, network, addr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| tlsConn := conn.(*tls.Conn) | |
| for _, cert := range tlsConn.ConnectionState().PeerCertificates { | |
| digest := sha256.Sum256(cert.Raw) | |
| if bytes.Equal(digest[:], fingerprint) { | |
| return tlsConn, nil | |
| } | |
| } | |
| _ = tlsConn.Close() | |
| return nil, fmt.Errorf("fingerprint mismatch, provided: %s", certificateFingerprint) | |
| if transport.TLSClientConfig == nil { | |
| transport.TLSClientConfig = &tls.Config{} | |
| } | |
| // In fingerprint mode, we bypass standard CA verification and rely solely on | |
| // the fingerprint check. We therefore set InsecureSkipVerify and implement | |
| // verification via VerifyPeerCertificate so that it applies consistently, | |
| // including for proxied HTTPS connections. | |
| cfg := transport.TLSClientConfig | |
| cfg.InsecureSkipVerify = true | |
| prevVerify := cfg.VerifyPeerCertificate | |
| cfg.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | |
| for _, raw := range rawCerts { | |
| cert, err := x509.ParseCertificate(raw) | |
| if err != nil { | |
| return err | |
| } | |
| digest := sha256.Sum256(cert.Raw) | |
| if bytes.Equal(digest[:], fingerprint) { | |
| if prevVerify != nil { | |
| return prevVerify(rawCerts, verifiedChains) | |
| } | |
| return nil | |
| } | |
| } | |
| return fmt.Errorf("fingerprint mismatch, provided: %s", certificateFingerprint) |
Comment on lines
+104
to
+108
| t.Run("Sets DialTLSContext when fingerprint is configured", func(t *testing.T) { | ||
| transport := &http.Transport{} | ||
| if err := ConfigureTLS(transport, nil, "A98116BE"); err != nil { | ||
| t.Fatalf("Unexpected error: %s", err) | ||
| } |
There was a problem hiding this comment.
These subtests pass a very short fingerprint ("A98116BE"), which decodes to 4 bytes and can never match a SHA-256 certificate digest (32 bytes). This currently allows ConfigureTLS to succeed but guarantees runtime fingerprint mismatch errors. The test should use a full 32-byte (64 hex chars) fingerprint, and ideally exercise a real TLS handshake to verify that the correct fingerprint succeeds and a wrong one fails.
Comment on lines
181
to
185
| if transport, ok := cfg.Transport.(*http.Transport); ok { | ||
| if cfg.CertificateFingerprint != "" { | ||
| transport.DialTLSContext = func(_ context.Context, network, addr string) (net.Conn, error) { | ||
| fingerprint, _ := hex.DecodeString(cfg.CertificateFingerprint) | ||
|
|
||
| c, err := tls.Dial(network, addr, &tls.Config{InsecureSkipVerify: true}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Retrieve the connection state from the remote server. | ||
| cState := c.ConnectionState() | ||
| for _, cert := range cState.PeerCertificates { | ||
| // Compute digest for each certificate. | ||
| digest := sha256.Sum256(cert.Raw) | ||
|
|
||
| // Provided fingerprint should match at least one certificate from remote before we continue. | ||
| if bytes.Equal(digest[0:], fingerprint) { | ||
| return c, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("fingerprint mismatch, provided: %s", cfg.CertificateFingerprint) | ||
| } | ||
| if cfg.CACert != nil || cfg.CertificateFingerprint != "" { | ||
| transport = transport.Clone() | ||
| cfg.Transport = transport | ||
| } |
There was a problem hiding this comment.
When cfg.Transport is nil, the default transport is already cloned above. This block clones the transport again whenever TLS options are set, which creates an extra unused clone in the common case. Consider only cloning here when the transport came from the user (or track a boolean) to avoid the redundant clone.
Comment on lines
+60
to
+113
| func TestConfigureTLS(t *testing.T) { | ||
| t.Run("Returns error when transport is nil", func(t *testing.T) { | ||
| if err := ConfigureTLS(nil, nil, ""); err == nil { | ||
| t.Fatal("Expected error, got nil") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Returns error on invalid CA cert", func(t *testing.T) { | ||
| transport := &http.Transport{} | ||
| if err := ConfigureTLS(transport, []byte("invalid"), ""); err == nil { | ||
| t.Fatal("Expected error, got nil") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Initializes TLS config and RootCAs from PEM", func(t *testing.T) { | ||
| caCert, err := os.ReadFile("testdata/cert.pem") | ||
| if err != nil { | ||
| t.Fatalf("Unexpected error reading cert: %s", err) | ||
| } | ||
|
|
||
| transport := &http.Transport{} | ||
| if err := ConfigureTLS(transport, caCert, ""); err != nil { | ||
| t.Fatalf("Unexpected error: %s", err) | ||
| } | ||
|
|
||
| if transport.TLSClientConfig == nil { | ||
| t.Fatal("Expected TLSClientConfig to be initialized") | ||
| } | ||
| if transport.TLSClientConfig.RootCAs == nil { | ||
| t.Fatal("Expected RootCAs to be initialized") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Returns error on invalid fingerprint hex", func(t *testing.T) { | ||
| transport := &http.Transport{} | ||
| err := ConfigureTLS(transport, nil, "not-valid-hex") | ||
| if err == nil { | ||
| t.Fatal("Expected error, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid certificate fingerprint") { | ||
| t.Fatalf("Unexpected error message: %s", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Sets DialTLSContext when fingerprint is configured", func(t *testing.T) { | ||
| transport := &http.Transport{} | ||
| if err := ConfigureTLS(transport, nil, "A98116BE"); err != nil { | ||
| t.Fatalf("Unexpected error: %s", err) | ||
| } | ||
|
|
||
| if transport.DialTLSContext == nil { | ||
| t.Fatal("Expected DialTLSContext to be configured") | ||
| } | ||
| }) |
There was a problem hiding this comment.
PR description states TestConfigureTLS covers valid fingerprint / mismatch (including closing connections). In this test file, fingerprint scenarios only assert that DialTLSContext is set and that invalid hex errors; they don't validate successful/failed verification behavior. Either update the tests to cover the described behaviors (e.g., using a local TLS server with testdata/cert.pem) or adjust the PR description/test plan to match what's actually covered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New()into an exportedConfigureTLShelper (tls.go) so that wrapped transports (e.g. customhttp.RoundTrippermiddleware) can reuse it directly.tls.Dialwith a context-awaretls.Dialerfor fingerprint verification.DialTLSContextbypasses standard CA verification.*http.Transporttransport.ConfigureTLSand the updatedNew()integration paths.Test plan
TestConfigureTLScovers nil transport, invalid CA cert, valid CA cert, invalid fingerprint hex, valid fingerprint, and fingerprint-takes-precedence-over-CACert scenarios.TestTransportConfigcovers CACert on default transport and TLS options on non-http.Transport error path.go test ./...