fixup! Add HTTP client to certificate manager

pkg/security/certManager/general/certManager.go

@@ -85,26 +85,7 @@ func tryToWatchFile(file urischeme.URIScheme, fileWatcher *fsnotify.Watcher, rem
 	return nil
 }
 
-// New creates a new certificate manager which watches for certs in a filesystem
-func New(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, tracerProvider trace.TracerProvider) (*CertManager, error) {
-	verifyClientCertificate := tls.RequireAndVerifyClientCert
-	if !config.ClientCertificateRequired {
-		verifyClientCertificate = tls.NoClientCert
-	}
-
-	var cleanUpOnError fn.FuncList
-	var httpClient *pkgHttpClient.Client
-	if config.CRL.Enabled {
-		var err error
-		httpClient, err = NewHTTPClient(config.CRL.HTTP, fileWatcher, logger, tracerProvider)
-		if err != nil {
-			return nil, err
-		}
-		cleanUpOnError.AddFunc(func() {
-			httpClient.Close()
-		})
-	}
-
+func newCertManager(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, verifyClientCertificate tls.ClientAuthType, httpClient *pkgHttpClient.Client, cleanUpOnError fn.FuncList) (*CertManager, error) {
 	c := &CertManager{
 		fileWatcher:             fileWatcher,
 		config:                  config,
@@ -114,33 +95,53 @@ func New(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, tracer
 	}
 	_, err := c.loadCAs()
 	if err != nil {
-		cleanUpOnError.Execute()
 		return nil, err
 	}
 	_, err = c.loadCerts()
 	if err != nil {
-		cleanUpOnError.Execute()
 		return nil, err
 	}
 
-	for _, ca := range config.CAPool {
-		if err = tryToWatchFile(ca, fileWatcher, cleanUpOnError); err != nil {
-			cleanUpOnError.Execute()
+	for _, ca := range c.config.CAPool {
+		if err = tryToWatchFile(ca, c.fileWatcher, cleanUpOnError); err != nil {
 			return nil, fmt.Errorf("cannot watch CAPool: %w", err)
 		}
 	}
-	if err = tryToWatchFile(config.CertFile, fileWatcher, cleanUpOnError); err != nil {
-		cleanUpOnError.Execute()
+	if err = tryToWatchFile(c.config.CertFile, c.fileWatcher, cleanUpOnError); err != nil {
 		return nil, fmt.Errorf("cannot watch CertFile: %w", err)
 	}
-	if err = tryToWatchFile(config.KeyFile, fileWatcher, cleanUpOnError); err != nil {
-		cleanUpOnError.Execute()
+	if err = tryToWatchFile(c.config.KeyFile, c.fileWatcher, cleanUpOnError); err != nil {
 		return nil, fmt.Errorf("cannot watch KeyFile: %w", err)
 	}
 
 	c.onFileChangeFunc = c.onFileChange
 	c.fileWatcher.AddOnEventHandler(&c.onFileChangeFunc)
+	return c, nil
+}
+
+// New creates a new certificate manager which watches for certs in a filesystem
+func New(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, tracerProvider trace.TracerProvider) (*CertManager, error) {
+	verifyClientCertificate := tls.RequireAndVerifyClientCert
+	if !config.ClientCertificateRequired {
+		verifyClientCertificate = tls.NoClientCert
+	}
+
+	var cleanUpOnError fn.FuncList
+	var httpClient *pkgHttpClient.Client
+	if config.CRL.Enabled {
+		var err error
+		httpClient, err = NewHTTPClient(config.CRL.HTTP, fileWatcher, logger, tracerProvider)
+		if err != nil {
+			return nil, err
+		}
+		cleanUpOnError.AddFunc(httpClient.Close)
+	}
 
+	c, err := newCertManager(config, fileWatcher, logger, verifyClientCertificate, httpClient, cleanUpOnError)
+	if err != nil {
+		cleanUpOnError.Execute()
+		return nil, err
+	}
 	return c, nil
 }
 

pkg/security/certManager/general/clientCertManager.go

@@ -49,7 +49,11 @@ func NewClientCertManager(config pkgTls.ClientConfig, fileWatcher *fsnotify.Watc
 	}
 	caPoolArray, _ := config.CAPoolArray()
 
-	c, err := New(ClientConfig(caPoolArray, config.KeyFile, config.CertFile, config.UseSystemCAPool, config.CRL), fileWatcher, ClientLogger(logger), tracerProvider)
+	cfg := ClientConfig(caPoolArray, config.KeyFile, config.CertFile, config.UseSystemCAPool, config.CRL)
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
+	c, err := New(cfg, fileWatcher, ClientLogger(logger), tracerProvider)
 	if err != nil {
 		return nil, err
 	}

pkg/security/certManager/server/certManager.go

@@ -81,14 +81,18 @@ func New(config Config, fileWatcher *fsnotify.Watcher, logger log.Logger, tracer
 			return nil, err
 		}
 	}
-	c, err := general.New(general.Config{
+	cfg := general.Config{
 		CAPool:                    config.caPoolArray,
 		KeyFile:                   config.KeyFile,
 		CertFile:                  config.CertFile,
 		ClientCertificateRequired: config.ClientCertificateRequired,
 		UseSystemCAPool:           false,
 		CRL:                       config.CRL,
-	}, fileWatcher, logger.With(log.CertManagerKey, "server"), tracerProvider)
+	}
+	if err := cfg.Validate(); err != nil {
+		return nil, err
+	}
+	c, err := general.New(cfg, fileWatcher, logger.With(log.CertManagerKey, "server"), tracerProvider)
 	if err != nil {
 		return nil, err
 	}

pkg/security/tls/client_test.go

@@ -0,0 +1,187 @@
+package tls_test
+
+import (
+	"testing"
+
+	"github.com/plgd-dev/hub/v2/pkg/security/tls"
+	"github.com/stretchr/testify/require"
+)
+
+func TestClientConfigValidate(t *testing.T) {
+	validClientConfig := func() *tls.ClientConfig {
+		return &tls.ClientConfig{
+			CAPool:          []string{"file://path/to/ca1.pem", "file://path/to/ca2.pem"},
+			KeyFile:         "file://path/to/key.pem",
+			CertFile:        "file://path/to/cert.pem",
+			UseSystemCAPool: false,
+		}
+	}
+
+	type args struct {
+		cfg *tls.ClientConfig
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "valid",
+			args: args{
+				cfg: validClientConfig(),
+			},
+		},
+		{
+			name: "invalid ca pool type",
+			args: args{
+				cfg: func() *tls.ClientConfig {
+					c := validClientConfig()
+					c.CAPool = 42
+					return c
+				}(),
+			},
+			wantErr: true,
+		},
+		{
+			name: "empty ca pool",
+			args: args{
+				cfg: func() *tls.ClientConfig {
+					c := validClientConfig()
+					c.CAPool = []string{}
+					c.UseSystemCAPool = false
+					return c
+				}(),
+			},
+			wantErr: true,
+		},
+		{
+			name: "missing keyfile",
+			args: args{
+				cfg: func() *tls.ClientConfig {
+					c := validClientConfig()
+					c.KeyFile = ""
+					return c
+				}(),
+			},
+			wantErr: true,
+		},
+		{
+			name: "missing certfile",
+			args: args{
+				cfg: func() *tls.ClientConfig {
+					c := validClientConfig()
+					c.CertFile = ""
+					return c
+				}(),
+			},
+			wantErr: true,
+		},
+		{
+			name: "crl invalid - missing http",
+			args: args{
+				cfg: func() *tls.ClientConfig {
+					c := validClientConfig()
+					c.CRL.Enabled = true
+					return c
+				}(),
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := tt.args.cfg.Validate()
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
+}
+
+func TestClientConfigEquals(t *testing.T) {
+	cfg1 := tls.ClientConfig{
+		CAPool:          []string{"file://path/to/ca1.pem", "file://path/to/ca2.pem"},
+		KeyFile:         "file://path/to/key.pem",
+		CertFile:        "file://path/to/cert.pem",
+		UseSystemCAPool: false,
+	}
+
+	type args struct {
+		cfg1 tls.ClientConfig
+		cfg2 tls.ClientConfig
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "equal configurations",
+			args: args{
+				cfg1: cfg1,
+				cfg2: cfg1,
+			},
+			want: true,
+		},
+		{
+			name: "different ca pool",
+			args: args{
+				cfg1: cfg1,
+				cfg2: func() tls.ClientConfig {
+					c := cfg1
+					c.CAPool = []string{"file://path/to/ca2.pem"}
+					return c
+				}(),
+			},
+			want: false,
+		},
+		{
+			name: "different keyfile",
+			args: args{
+				cfg1: cfg1,
+				cfg2: func() tls.ClientConfig {
+					c := cfg1
+					c.KeyFile = "file://path/to/otherkey.pem"
+					return c
+				}(),
+			},
+			want: false,
+		},
+		{
+			name: "different certfile",
+			args: args{
+				cfg1: cfg1,
+				cfg2: func() tls.ClientConfig {
+					c := cfg1
+					c.CertFile = "file://path/to/othercert.pem"
+					return c
+				}(),
+			},
+			want: false,
+		},
+		{
+			name: "different crl",
+			args: args{
+				cfg1: cfg1,
+				cfg2: func() tls.ClientConfig {
+					c := cfg1
+					c.CRL = tls.CRLConfig{
+						Enabled: true,
+						HTTP:    &tls.HTTPConfig{},
+					}
+					return c
+				}(),
+			},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.Equal(t, tt.want, tt.args.cfg1.Equals(tt.args.cfg2))
+		})
+	}
+}

pkg/security/tls/crl.go

@@ -118,9 +118,7 @@ func (c *CRLConfig) Equals(c2 CRLConfig) bool {
 		return false
 	}
 	if !c.Enabled {
-		r1 := c.HTTP == nil
-		r2 := c2.HTTP == nil
-		return r1 && r2
+		return c.HTTP == nil && c2.HTTP == nil
 	}
 	if c.HTTP == nil {
 		return c2.HTTP == nil