Merge pull request #270 from newrelic/add_client_unsecure_skip_verify

HTTP Client: Add possibility to skip server verification and refactor client to work with options pattern
newrelic · Aug 2, 2021 · 1f48f6c · 1f48f6c
2 parents f0f6bb8 + 916feb0
commit 1f48f6c
Show file tree

Hide file tree

Showing 2 changed files with 245 additions and 120 deletions.
diff --git a/http/client.go b/http/client.go
@@ -6,116 +6,175 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"net/http"
-	"os"
 	"path/filepath"
 	"strings"
 	"time"
 )
 
-// New creates a new http.Client with a custom certificate, which can be loaded from the passed CA Bundle file and/or
+var (
+	// ErrInvalidTransportType defines the error to be returned if the transport defined for the client is incorrect
+	ErrInvalidTransportType = errors.New("roundTripper transport invalid, should be http type")
+	// ErrEmptyArg defines the error to be returned if an option is defined with an empty argument
+	ErrEmptyArg = errors.New("argument is empty")
+)
+
+// ClientOption defines the format of the client option functions
+// Note that many options defined in this package rely on the Transport for the http.Client being of type *http.Transport.
+// Using a different Transport will cause these options to fail.
+type ClientOption func(*http.Client) error
+
+// New creates a new http.Client with the Passed Client Options
+// that will have a custom certificate if it is loaded from the passed CA Bundle file and/or
 // directory. If both CABundleFile and CABundleDir are empty arguments, it creates an unsecure HTTP client.
-func New(CABundleFile, CABundleDir string, httpTimeout time.Duration) (*http.Client, error) {
-	return _new(CABundleFile, CABundleDir, httpTimeout, "")
-}
+func New(opts ...ClientOption) (*http.Client, error) {
+	t := &http.Transport{
+		TLSClientConfig: &tls.Config{},
+	}
 
-// NewAcceptInvalidHostname new http.Client with ability to accept HTTPS certificates that don't
-// match the hostname of the server they are connecting to.
-func NewAcceptInvalidHostname(CABundleFile, CABundleDir string, httpTimeout time.Duration, hostname string) (*http.Client, error) {
-	return _new(CABundleFile, CABundleDir, httpTimeout, hostname)
+	client := &http.Client{
+		Transport: t,
+	}
+
+	for _, opt := range opts {
+		if err := opt(client); err != nil {
+			return nil, err
+		}
+	}
+
+	return client, nil
 }
 
-func _new(CABundleFile, CABundleDir string, httpTimeout time.Duration, acceptInvalidHostname string) (*http.Client, error) {
-	// go default http transport settings
-	t := &http.Transport{}
+// WithCABundleFile adds the CABundleFile cert to the the client's certPool
+func WithCABundleFile(CABundleFile string) ClientOption {
+	return func(c *http.Client) error {
+		if CABundleFile == "" {
+			return fmt.Errorf("applying CABundleFile: %w", ErrEmptyArg)
+		}
 
-	if CABundleFile != "" || CABundleDir != "" {
-		certs, err := getCertPool(CABundleFile, CABundleDir)
+		certPool, err := clientCertPool(c)
 		if err != nil {
-			return nil, err
+			return err
 		}
+		return addCert(CABundleFile, certPool)
+	}
+}
 
-		t.TLSClientConfig = &tls.Config{
-			RootCAs: certs,
+// WithCABundleDir adds the CABundleDir looks for pem certs in the
+// CABundleDir and adds them to the the client's certPool.
+func WithCABundleDir(CABundleDir string) ClientOption {
+	return func(c *http.Client) error {
+		if CABundleDir == "" {
+			return fmt.Errorf("applying CABundleDir: %w", ErrEmptyArg)
 		}
 
-		if acceptInvalidHostname != "" {
-			// Default validation is replaced with VerifyPeerCertificate.
-			// Note that when InsecureSkipVerify and VerifyPeerCertificate are in use,
-			// ConnectionState.VerifiedChains will be nil.
-			t.TLSClientConfig.InsecureSkipVerify = true
-			// While packages like net/http will implicitly set ServerName, the
-			// VerifyPeerCertificate callback can't access that value, so it has to be set
-			// explicitly here or in VerifyPeerCertificate on the client side. If in
-			// an http.Transport DialTLS callback, this can be obtained by passing
-			// the addr argument to net.SplitHostPort.
-			t.TLSClientConfig.ServerName = acceptInvalidHostname
-			// Approximately equivalent to what crypto/tls does normally:
-			// https://github.com/golang/go/commit/29cfb4d3c3a97b6f426d1b899234da905be699aa
-			t.TLSClientConfig.VerifyPeerCertificate = func(certificates [][]byte, _ [][]*x509.Certificate) error {
-				certs := make([]*x509.Certificate, len(certificates))
-				for i, asn1Data := range certificates {
-					cert, err := x509.ParseCertificate(asn1Data)
-					if err != nil {
-						return errors.New("tls: failed to parse certificate from server: " + err.Error())
-					}
-					certs[i] = cert
-				}
+		certPool, err := clientCertPool(c)
+		if err != nil {
+			return err
+		}
 
-				opts := x509.VerifyOptions{
-					Roots:         t.TLSClientConfig.RootCAs, // On the server side, use config.ClientCAs.
-					DNSName:       acceptInvalidHostname,
-					Intermediates: x509.NewCertPool(),
-					// On the server side, set KeyUsages to ExtKeyUsageClientAuth. The
-					// default value is appropriate for clients side verification.
-					// KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
-				}
-				for _, cert := range certs[1:] {
-					opts.Intermediates.AddCert(cert)
+		files, err := ioutil.ReadDir(CABundleDir)
+		if err != nil {
+			return err
+		}
+
+		for _, f := range files {
+			if strings.Contains(f.Name(), ".pem") {
+				caCertFilePath := filepath.Join(CABundleDir, f.Name())
+				if err := addCert(caCertFilePath, certPool); err != nil {
+					return err
 				}
-				_, err := certs[0].Verify(opts)
-				return err
 			}
-
 		}
+		return nil
 	}
-
-	return &http.Client{
-		Timeout:   httpTimeout,
-		Transport: t,
-	}, nil
 }
 
-func getCertPool(certFile string, certDirectory string) (*x509.CertPool, error) {
-	caCertPool := x509.NewCertPool()
+// WithAcceptInvalidHostname allows the client to call the acceptInvalidHostname host
+// instead of the host from the certificates.
+func WithAcceptInvalidHostname(acceptInvalidHostname string) ClientOption {
+	return func(c *http.Client) error {
+		if acceptInvalidHostname == "" {
+			return fmt.Errorf("applying AcceptInvalidHostname: %w", ErrEmptyArg)
+		}
 
-	if certFile != "" {
-		if err := addCert(filepath.Join(certDirectory, certFile), caCertPool); err != nil {
-			if os.IsNotExist(err) {
-				if err = addCert(certFile, caCertPool); err != nil {
-					return nil, err
+		transport, ok := c.Transport.(*http.Transport)
+		if !ok {
+			return ErrInvalidTransportType
+		}
+		// Default validation is replaced with VerifyPeerCertificate.
+		// Note that when InsecureSkipVerify and VerifyPeerCertificate are in use,
+		// ConnectionState.VerifiedChains will be nil.
+		transport.TLSClientConfig.InsecureSkipVerify = true
+		// While packages like net/http will implicitly set ServerName, the
+		// VerifyPeerCertificate callback can't access that value, so it has to be set
+		// explicitly here or in VerifyPeerCertificate on the client side. If in
+		// an http.Transport DialTLS callback, this can be obtained by passing
+		// the addr argument to net.SplitHostPort.
+		transport.TLSClientConfig.ServerName = acceptInvalidHostname
+		// Approximately equivalent to what crypto/tls does normally:
+		// https://github.com/golang/go/commit/29cfb4d3c3a97b6f426d1b899234da905be699aa
+		transport.TLSClientConfig.VerifyPeerCertificate = func(certificates [][]byte, _ [][]*x509.Certificate) error {
+			certs := make([]*x509.Certificate, len(certificates))
+			for i, asn1Data := range certificates {
+				cert, err := x509.ParseCertificate(asn1Data)
+				if err != nil {
+					return errors.New("tls: failed to parse certificate from server: " + err.Error())
 				}
+				certs[i] = cert
+			}
+
+			opts := x509.VerifyOptions{
+				Roots:         transport.TLSClientConfig.RootCAs, // On the server side, use config.ClientCAs.
+				DNSName:       acceptInvalidHostname,
+				Intermediates: x509.NewCertPool(),
+				// On the server side, set KeyUsages to ExtKeyUsageClientAuth. The
+				// default value is appropriate for clients side verification.
+				// KeyUsages:     []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
 			}
+			for _, cert := range certs[1:] {
+				opts.Intermediates.AddCert(cert)
+			}
+			_, err := certs[0].Verify(opts)
+			return err
 		}
+		return nil
 	}
+}
 
-	if certDirectory != "" {
-		files, err := ioutil.ReadDir(certDirectory)
-		if err != nil {
-			return nil, err
+// WithTLSInsecureSkipVerify allows the client to call any host without checking the certificates.
+func WithTLSInsecureSkipVerify() ClientOption {
+	return func(c *http.Client) error {
+		transport, ok := c.Transport.(*http.Transport)
+		if !ok {
+			return ErrInvalidTransportType
 		}
 
-		for _, f := range files {
-			if strings.Contains(f.Name(), ".pem") {
-				caCertFilePath := filepath.Join(certDirectory, f.Name())
-				if err := addCert(caCertFilePath, caCertPool); err != nil {
-					return nil, err
-				}
-			}
-		}
+		transport.TLSClientConfig.InsecureSkipVerify = true
+		return nil
+	}
+}
+
+// WithTimeout sets the timeout for the client, if not called the timeout will be 0 (no timeout).
+func WithTimeout(httpTimeout time.Duration) ClientOption {
+	return func(c *http.Client) error {
+		c.Timeout = httpTimeout
+		return nil
+	}
+}
+
+func clientCertPool(c *http.Client) (*x509.CertPool, error) {
+	transport, ok := c.Transport.(*http.Transport)
+	if !ok {
+		return nil, ErrInvalidTransportType
+	}
+
+	if transport.TLSClientConfig.RootCAs == nil {
+		transport.TLSClientConfig.RootCAs = x509.NewCertPool()
 	}
-	return caCertPool, nil
+	return transport.TLSClientConfig.RootCAs, nil
 }
 
 func addCert(certFile string, caCertPool *x509.CertPool) error {