Merge pull request #13535 from helm/refactor_tlsutil

refactor: tlsutil use options pattern

cmd/helm/root.go

@@ -297,7 +297,12 @@ func newDefaultRegistryClient(plainHTTP bool, username, password string) (*regis
 func newRegistryClientWithTLS(
 	certFile, keyFile, caFile string, insecureSkipTLSverify bool, username, password string,
 ) (*registry.Client, error) {
-	tlsConf, err := tlsutil.NewClientTLS(certFile, keyFile, caFile, insecureSkipTLSverify)
+	tlsConf, err := tlsutil.NewTLSConfig(
+		tlsutil.WithInsecureSkipVerify(insecureSkipTLSverify),
+		tlsutil.WithCertKeyPairFiles(certFile, keyFile),
+		tlsutil.WithCAFile(caFile),
+	)
+
 	if err != nil {
 		return nil, fmt.Errorf("can't create TLS config for client: %w", err)
 	}

internal/tlsutil/cfg.go

@@ -1,58 +0,0 @@
-/*
-Copyright The Helm 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 tlsutil
-
-import (
-	"crypto/tls"
-	"crypto/x509"
-	"os"
-
-	"github.com/pkg/errors"
-)
-
-// Options represents configurable options used to create client and server TLS configurations.
-type Options struct {
-	CaCertFile string
-	// If either the KeyFile or CertFile is empty, ClientConfig() will not load them.
-	KeyFile  string
-	CertFile string
-	// Client-only options
-	InsecureSkipVerify bool
-}
-
-// ClientConfig returns a TLS configuration for use by a Helm client.
-func ClientConfig(opts Options) (cfg *tls.Config, err error) {
-	var cert *tls.Certificate
-	var pool *x509.CertPool
-
-	if opts.CertFile != "" || opts.KeyFile != "" {
-		if cert, err = CertFromFilePair(opts.CertFile, opts.KeyFile); err != nil {
-			if os.IsNotExist(err) {
-				return nil, errors.Wrapf(err, "could not load x509 key pair (cert: %q, key: %q)", opts.CertFile, opts.KeyFile)
-			}
-			return nil, errors.Wrapf(err, "could not read x509 key pair (cert: %q, key: %q)", opts.CertFile, opts.KeyFile)
-		}
-	}
-	if !opts.InsecureSkipVerify && opts.CaCertFile != "" {
-		if pool, err = CertPoolFromFile(opts.CaCertFile); err != nil {
-			return nil, err
-		}
-	}
-
-	cfg = &tls.Config{InsecureSkipVerify: opts.InsecureSkipVerify, Certificates: []tls.Certificate{*cert}, RootCAs: pool}
-	return cfg, nil
-}

internal/tlsutil/tls.go

@@ -19,60 +19,104 @@ package tlsutil
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"fmt"
 	"os"
 
-	"github.com/pkg/errors"
+	"errors"
 )
 
-// NewClientTLS returns tls.Config appropriate for client auth.
+type TLSConfigOptions struct {
-func NewClientTLS(certFile, keyFile, caFile string, insecureSkipTLSverify bool) (*tls.Config, error) {
+	insecureSkipTLSverify     bool
-	config := tls.Config{
+	certPEMBlock, keyPEMBlock []byte
-		InsecureSkipVerify: insecureSkipTLSverify,
+	caPEMBlock                []byte
+}
+
+type TLSConfigOption func(options *TLSConfigOptions) error
+
+func WithInsecureSkipVerify(insecureSkipTLSverify bool) TLSConfigOption {
+	return func(options *TLSConfigOptions) error {
+		options.insecureSkipTLSverify = insecureSkipTLSverify
+
+		return nil
 	}
+}
+
+func WithCertKeyPairFiles(certFile, keyFile string) TLSConfigOption {
+	return func(options *TLSConfigOptions) error {
+		if certFile == "" && keyFile == "" {
+			return nil
+		}
 
-	if certFile != "" && keyFile != "" {
+		certPEMBlock, err := os.ReadFile(certFile)
-		cert, err := CertFromFilePair(certFile, keyFile)
 		if err != nil {
-			return nil, err
+			return fmt.Errorf("unable to read cert file: %q: %w", certFile, err)
 		}
-		config.Certificates = []tls.Certificate{*cert}
-	}
 
-	if caFile != "" {
+		keyPEMBlock, err := os.ReadFile(keyFile)
-		cp, err := CertPoolFromFile(caFile)
 		if err != nil {
-			return nil, err
+			return fmt.Errorf("unable to read key file: %q: %w", keyFile, err)
 		}
-		config.RootCAs = cp
+
+		options.certPEMBlock = certPEMBlock
+		options.keyPEMBlock = keyPEMBlock
+
+		return nil
 	}
+}
 
-	return &config, nil
+func WithCAFile(caFile string) TLSConfigOption {
+	return func(options *TLSConfigOptions) error {
+		if caFile == "" {
+			return nil
+		}
+
+		caPEMBlock, err := os.ReadFile(caFile)
+		if err != nil {
+			return fmt.Errorf("can't read CA file: %q: %w", caFile, err)
+		}
+
+		options.caPEMBlock = caPEMBlock
+
+		return nil
+	}
 }
 
-// CertPoolFromFile returns an x509.CertPool containing the certificates
+func NewTLSConfig(options ...TLSConfigOption) (*tls.Config, error) {
-// in the given PEM-encoded file.
+	to := TLSConfigOptions{}
-// Returns an error if the file could not be read, a certificate could not
+
-// be parsed, or if the file does not contain any certificates
+	errs := []error{}
-func CertPoolFromFile(filename string) (*x509.CertPool, error) {
+	for _, option := range options {
-	b, err := os.ReadFile(filename)
+		err := option(&to)
-	if err != nil {
+		if err != nil {
-		return nil, errors.Errorf("can't read CA file: %v", filename)
+			errs = append(errs, err)
+		}
 	}
-	cp := x509.NewCertPool()
+
-	if !cp.AppendCertsFromPEM(b) {
+	if len(errs) > 0 {
-		return nil, errors.Errorf("failed to append certificates from file: %s", filename)
+		return nil, errors.Join(errs...)
 	}
-	return cp, nil
-}
 
-// CertFromFilePair returns a tls.Certificate containing the
+	config := tls.Config{
-// certificates public/private key pair from a pair of given PEM-encoded files.
+		InsecureSkipVerify: to.insecureSkipTLSverify,
-// Returns an error if the file could not be read, a certificate could not
-// be parsed, or if the file does not contain any certificates
-func CertFromFilePair(certFile, keyFile string) (*tls.Certificate, error) {
-	cert, err := tls.LoadX509KeyPair(certFile, keyFile)
-	if err != nil {
-		return nil, errors.Wrapf(err, "can't load key pair from cert %s and key %s", certFile, keyFile)
 	}
-	return &cert, err
+
+	if len(to.certPEMBlock) > 0 && len(to.keyPEMBlock) > 0 {
+		cert, err := tls.X509KeyPair(to.certPEMBlock, to.keyPEMBlock)
+		if err != nil {
+			return nil, fmt.Errorf("unable to load cert from key pair: %w", err)
+		}
+
+		config.Certificates = []tls.Certificate{cert}
+	}
+
+	if len(to.caPEMBlock) > 0 {
+		cp := x509.NewCertPool()
+		if !cp.AppendCertsFromPEM(to.caPEMBlock) {
+			return nil, fmt.Errorf("failed to append certificates from pem block")
+		}
+
+		config.RootCAs = cp
+	}
+
+	return &config, nil
 }

internal/tlsutil/tls_test.go

@@ -0,0 +1,105 @@
+/*
+Copyright The Helm 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 tlsutil
+
+import (
+	"path/filepath"
+	"testing"
+)
+
+const tlsTestDir = "../../testdata"
+
+const (
+	testCaCertFile = "rootca.crt"
+	testCertFile   = "crt.pem"
+	testKeyFile    = "key.pem"
+)
+
+func testfile(t *testing.T, file string) (path string) {
+	var err error
+	if path, err = filepath.Abs(filepath.Join(tlsTestDir, file)); err != nil {
+		t.Fatalf("error getting absolute path to test file %q: %v", file, err)
+	}
+	return path
+}
+
+func TestNewTLSConfig(t *testing.T) {
+	certFile := testfile(t, testCertFile)
+	keyFile := testfile(t, testKeyFile)
+	caCertFile := testfile(t, testCaCertFile)
+	insecureSkipTLSverify := false
+
+	{
+		cfg, err := NewTLSConfig(
+			WithInsecureSkipVerify(insecureSkipTLSverify),
+			WithCertKeyPairFiles(certFile, keyFile),
+			WithCAFile(caCertFile),
+		)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if got := len(cfg.Certificates); got != 1 {
+			t.Fatalf("expecting 1 client certificates, got %d", got)
+		}
+		if cfg.InsecureSkipVerify {
+			t.Fatalf("insecure skip verify mismatch, expecting false")
+		}
+		if cfg.RootCAs == nil {
+			t.Fatalf("mismatch tls RootCAs, expecting non-nil")
+		}
+	}
+	{
+		cfg, err := NewTLSConfig(
+			WithInsecureSkipVerify(insecureSkipTLSverify),
+			WithCAFile(caCertFile),
+		)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if got := len(cfg.Certificates); got != 0 {
+			t.Fatalf("expecting 0 client certificates, got %d", got)
+		}
+		if cfg.InsecureSkipVerify {
+			t.Fatalf("insecure skip verify mismatch, expecting false")
+		}
+		if cfg.RootCAs == nil {
+			t.Fatalf("mismatch tls RootCAs, expecting non-nil")
+		}
+	}
+
+	{
+		cfg, err := NewTLSConfig(
+			WithInsecureSkipVerify(insecureSkipTLSverify),
+			WithCertKeyPairFiles(certFile, keyFile),
+		)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if got := len(cfg.Certificates); got != 1 {
+			t.Fatalf("expecting 1 client certificates, got %d", got)
+		}
+		if cfg.InsecureSkipVerify {
+			t.Fatalf("insecure skip verify mismatch, expecting false")
+		}
+		if cfg.RootCAs != nil {
+			t.Fatalf("mismatch tls RootCAs, expecting nil")
+		}
+	}
+}

internal/tlsutil/tlsutil_test.go

@@ -1,114 +0,0 @@
-/*
-Copyright The Helm 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 tlsutil
-
-import (
-	"path/filepath"
-	"testing"
-)
-
-const tlsTestDir = "../../testdata"
-
-const (
-	testCaCertFile = "rootca.crt"
-	testCertFile   = "crt.pem"
-	testKeyFile    = "key.pem"
-)
-
-func TestClientConfig(t *testing.T) {
-	opts := Options{
-		CaCertFile:         testfile(t, testCaCertFile),
-		CertFile:           testfile(t, testCertFile),
-		KeyFile:            testfile(t, testKeyFile),
-		InsecureSkipVerify: false,
-	}
-
-	cfg, err := ClientConfig(opts)
-	if err != nil {
-		t.Fatalf("error building tls client config: %v", err)
-	}
-
-	if got := len(cfg.Certificates); got != 1 {
-		t.Fatalf("expecting 1 client certificates, got %d", got)
-	}
-	if cfg.InsecureSkipVerify {
-		t.Fatalf("insecure skip verify mismatch, expecting false")
-	}
-	if cfg.RootCAs == nil {
-		t.Fatalf("mismatch tls RootCAs, expecting non-nil")
-	}
-}
-
-func testfile(t *testing.T, file string) (path string) {
-	var err error
-	if path, err = filepath.Abs(filepath.Join(tlsTestDir, file)); err != nil {
-		t.Fatalf("error getting absolute path to test file %q: %v", file, err)
-	}
-	return path
-}
-
-func TestNewClientTLS(t *testing.T) {
-	certFile := testfile(t, testCertFile)
-	keyFile := testfile(t, testKeyFile)
-	caCertFile := testfile(t, testCaCertFile)
-	insecureSkipTLSverify := false
-
-	cfg, err := NewClientTLS(certFile, keyFile, caCertFile, insecureSkipTLSverify)
-	if err != nil {
-		t.Error(err)
-	}
-
-	if got := len(cfg.Certificates); got != 1 {
-		t.Fatalf("expecting 1 client certificates, got %d", got)
-	}
-	if cfg.InsecureSkipVerify {
-		t.Fatalf("insecure skip verify mismatch, expecting false")
-	}
-	if cfg.RootCAs == nil {
-		t.Fatalf("mismatch tls RootCAs, expecting non-nil")
-	}
-
-	cfg, err = NewClientTLS("", "", caCertFile, insecureSkipTLSverify)
-	if err != nil {
-		t.Error(err)
-	}
-
-	if got := len(cfg.Certificates); got != 0 {
-		t.Fatalf("expecting 0 client certificates, got %d", got)
-	}
-	if cfg.InsecureSkipVerify {
-		t.Fatalf("insecure skip verify mismatch, expecting false")
-	}
-	if cfg.RootCAs == nil {
-		t.Fatalf("mismatch tls RootCAs, expecting non-nil")
-	}
-
-	cfg, err = NewClientTLS(certFile, keyFile, "", insecureSkipTLSverify)
-	if err != nil {
-		t.Error(err)
-	}
-
-	if got := len(cfg.Certificates); got != 1 {
-		t.Fatalf("expecting 1 client certificates, got %d", got)
-	}
-	if cfg.InsecureSkipVerify {
-		t.Fatalf("insecure skip verify mismatch, expecting false")
-	}
-	if cfg.RootCAs != nil {
-		t.Fatalf("mismatch tls RootCAs, expecting nil")
-	}
-}

pkg/getter/httpgetter.go

@@ -128,7 +128,11 @@ func (g *HTTPGetter) httpClient() (*http.Client, error) {
 	})
 
 	if (g.opts.certFile != "" && g.opts.keyFile != "") || g.opts.caFile != "" || g.opts.insecureSkipVerifyTLS {
-		tlsConf, err := tlsutil.NewClientTLS(g.opts.certFile, g.opts.keyFile, g.opts.caFile, g.opts.insecureSkipVerifyTLS)
+		tlsConf, err := tlsutil.NewTLSConfig(
+			tlsutil.WithInsecureSkipVerify(g.opts.insecureSkipVerifyTLS),
+			tlsutil.WithCertKeyPairFiles(g.opts.certFile, g.opts.keyFile),
+			tlsutil.WithCAFile(g.opts.caFile),
+		)
 		if err != nil {
 			return nil, errors.Wrap(err, "can't create TLS config for client")
 		}

pkg/getter/httpgetter_test.go

@@ -311,7 +311,11 @@ func TestDownloadTLS(t *testing.T) {
 	insecureSkipTLSverify := false
 
 	tlsSrv := httptest.NewUnstartedServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
-	tlsConf, err := tlsutil.NewClientTLS(pub, priv, ca, insecureSkipTLSverify)
+	tlsConf, err := tlsutil.NewTLSConfig(
+		tlsutil.WithInsecureSkipVerify(insecureSkipTLSverify),
+		tlsutil.WithCertKeyPairFiles(pub, priv),
+		tlsutil.WithCAFile(ca),
+	)
 	if err != nil {
 		t.Fatal(errors.Wrap(err, "can't create TLS config for client"))
 	}

pkg/getter/ocigetter.go

@@ -128,7 +128,11 @@ func (g *OCIGetter) newRegistryClient() (*registry.Client, error) {
 	})
 
 	if (g.opts.certFile != "" && g.opts.keyFile != "") || g.opts.caFile != "" || g.opts.insecureSkipVerifyTLS {
-		tlsConf, err := tlsutil.NewClientTLS(g.opts.certFile, g.opts.keyFile, g.opts.caFile, g.opts.insecureSkipVerifyTLS)
+		tlsConf, err := tlsutil.NewTLSConfig(
+			tlsutil.WithInsecureSkipVerify(g.opts.insecureSkipVerifyTLS),
+			tlsutil.WithCertKeyPairFiles(g.opts.certFile, g.opts.keyFile),
+			tlsutil.WithCAFile(g.opts.caFile),
+		)
 		if err != nil {
 			return nil, fmt.Errorf("can't create TLS config for client: %w", err)
 		}

pkg/pusher/ocipusher.go

@@ -111,7 +111,11 @@ func NewOCIPusher(ops ...Option) (Pusher, error) {
 
 func (pusher *OCIPusher) newRegistryClient() (*registry.Client, error) {
 	if (pusher.opts.certFile != "" && pusher.opts.keyFile != "") || pusher.opts.caFile != "" || pusher.opts.insecureSkipTLSverify {
-		tlsConf, err := tlsutil.NewClientTLS(pusher.opts.certFile, pusher.opts.keyFile, pusher.opts.caFile, pusher.opts.insecureSkipTLSverify)
+		tlsConf, err := tlsutil.NewTLSConfig(
+			tlsutil.WithInsecureSkipVerify(pusher.opts.insecureSkipTLSverify),
+			tlsutil.WithCertKeyPairFiles(pusher.opts.certFile, pusher.opts.keyFile),
+			tlsutil.WithCAFile(pusher.opts.caFile),
+		)
 		if err != nil {
 			return nil, errors.Wrap(err, "can't create TLS config for client")
 		}

pkg/registry/util.go

@@ -101,7 +101,11 @@ func extractChartMeta(chartData []byte) (*chart.Metadata, error) {
 
 // NewRegistryClientWithTLS is a helper function to create a new registry client with TLS enabled.
 func NewRegistryClientWithTLS(out io.Writer, certFile, keyFile, caFile string, insecureSkipTLSverify bool, registryConfig string, debug bool) (*Client, error) {
-	tlsConf, err := tlsutil.NewClientTLS(certFile, keyFile, caFile, insecureSkipTLSverify)
+	tlsConf, err := tlsutil.NewTLSConfig(
+		tlsutil.WithInsecureSkipVerify(insecureSkipTLSverify),
+		tlsutil.WithCertKeyPairFiles(certFile, keyFile),
+		tlsutil.WithCAFile(caFile),
+	)
 	if err != nil {
 		return nil, fmt.Errorf("can't create TLS config for client: %s", err)
 	}

pkg/registry/utils_test.go

@@ -94,9 +94,14 @@ func setup(suite *TestSuite, tlsEnabled, insecure bool) *registry.Registry {
 	if tlsEnabled {
 		var tlsConf *tls.Config
 		if insecure {
-			tlsConf, err = tlsutil.NewClientTLS("", "", "", true)
+			tlsConf, err = tlsutil.NewTLSConfig(
+				tlsutil.WithInsecureSkipVerify(true),
+			)
 		} else {
-			tlsConf, err = tlsutil.NewClientTLS(tlsCert, tlsKey, tlsCA, false)
+			tlsConf, err = tlsutil.NewTLSConfig(
+				tlsutil.WithCertKeyPairFiles(tlsCert, tlsKey),
+				tlsutil.WithCAFile(tlsCA),
+			)
 		}
 		httpClient := &http.Client{
 			Transport: &http.Transport{

pkg/repo/repotest/server.go

@@ -368,7 +368,11 @@ func (s *Server) StartTLS() {
 		}
 		http.FileServer(http.Dir(s.Root())).ServeHTTP(w, r)
 	}))
-	tlsConf, err := tlsutil.NewClientTLS(pub, priv, ca, insecure)
+	tlsConf, err := tlsutil.NewTLSConfig(
+		tlsutil.WithInsecureSkipVerify(insecure),
+		tlsutil.WithCertKeyPairFiles(pub, priv),
+		tlsutil.WithCAFile(ca),
+	)
 	if err != nil {
 		panic(err)
 	}