fix: Use specified CA cert for grpc

This had been skipping out of the function early if a client key
wasn't specified.

I don't believe that's correct. If I[User] have specified specified a
CA cert because the MPI server I'm trying to talk to is signed by a
non-standard CA (e.g. N1 devenv) then it should be respected
regardless of whether I've configured mTLS.

Silently skipping the CA is really confusing and leads to

> Failed to create connection" error="rpc error: code = Unavailable desc = connection error: desc = \"transport: authentication handshake failed: tls: failed to verify certificate: x509: certificate signed by unknown authority\"

I've split out the getTLSConfigForCredentials to make it easier to
test this translation. Once it is wrapped into a TransportCredential
or a DialOption it's opaque and hard to verify.

Author: UnwashedMeme

internal/grpc/grpc.go

@@ -363,30 +363,32 @@ func getTransportCredentials(agentConfig *config.Config) (credentials.TransportC
 	if agentConfig.Command.TLS == nil {
 		return defaultCredentials, nil
 	}
+	tlsConfig, err := getTLSConfigForCredentials(agentConfig.Command.TLS)
+	if err != nil {
+		return nil, err
+	}
+	return credentials.NewTLS(tlsConfig), nil
+}
 
-	if agentConfig.Command.TLS.SkipVerify {
+func getTLSConfigForCredentials(c *config.TLSConfig) (*tls.Config, error) {
+	if c.SkipVerify {
 		slog.Warn("Verification of the server's certificate chain and host name is disabled")
 	}
 
 	tlsConfig := &tls.Config{
 		MinVersion:         tls.VersionTLS12,
-		ServerName:         agentConfig.Command.TLS.ServerName,
-		InsecureSkipVerify: agentConfig.Command.TLS.SkipVerify,
-	}
-
-	if agentConfig.Command.TLS.Key == "" {
-		return credentials.NewTLS(tlsConfig), nil
+		ServerName:         c.ServerName,
+		InsecureSkipVerify: c.SkipVerify,
 	}
 
-	err := appendCertKeyPair(tlsConfig, agentConfig.Command.TLS.Cert, agentConfig.Command.TLS.Key)
+	err := appendRootCAs(tlsConfig, c.Ca)
 	if err != nil {
-		return nil, fmt.Errorf("append cert and key pair failed: %w", err)
+		slog.Debug("Unable to append root CA", "error", err)
 	}
 
-	err = appendRootCAs(tlsConfig, agentConfig.Command.TLS.Ca)
+	err = appendCertKeyPair(tlsConfig, c.Cert, c.Key)
 	if err != nil {
-		slog.Debug("Unable to append root CA", "error", err)
+		return nil, fmt.Errorf("append cert and key pair failed: %w", err)
 	}
-
-	return credentials.NewTLS(tlsConfig), nil
+	return tlsConfig, nil
 }

internal/grpc/grpc_test.go

@@ -7,11 +7,10 @@ package grpc
 
 import (
 	"context"
+	"crypto/tls"
 	"fmt"
 	"testing"
 
-	"google.golang.org/grpc/credentials"
-
 	"github.com/cenkalti/backoff/v4"
 	"github.com/nginx/agent/v3/test/helpers"
 	"github.com/nginx/agent/v3/test/protos"
@@ -356,28 +355,131 @@ func Test_ValidateGrpcError(t *testing.T) {
 }
 
 func Test_getTransportCredentials(t *testing.T) {
-	tests := []struct {
-		want    credentials.TransportCredentials
-		conf    *config.Config
-		wantErr assert.ErrorAssertionFunc
-		name    string
+	tests := map[string]struct {
+		conf                *config.Config
+		wantErr             bool
+		wantSecurityProfile string
+		wantServerName      string
 	}{
-		{
-			name: "No TLS config returns default credentials",
+		"Test 1: No TLS config returns default credentials": {
 			conf: &config.Config{
 				Command: &config.Command{},
 			},
-			want:    defaultCredentials,
-			wantErr: assert.NoError,
+			wantErr:             false,
+			wantSecurityProfile: "insecure",
+		},
+		"Test 2: With tls config returns secure credentials": {
+			conf: &config.Config{
+				Command: &config.Command{
+					TLS: &config.TLSConfig{
+						ServerName: "foobar",
+						SkipVerify: true,
+					},
+				},
+			},
+			wantErr:             false,
+			wantSecurityProfile: "tls",
+		},
+		"Test 3: With invalid tls config should error": {
+			conf:    types.AgentConfig(), // references non-existant certs
+			wantErr: true,
 		},
 	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
 			got, err := getTransportCredentials(tt.conf)
-			if !tt.wantErr(t, err, fmt.Sprintf("getTransportCredentials(%v)", tt.conf)) {
+			if tt.wantErr {
+				require.Error(t, err, "getTransportCredentials(%v)", tt.conf)
+				return
+			}
+			require.NoError(t, err, "getTransportCredentials(%v)", tt.conf)
+			require.Equal(t, tt.wantSecurityProfile, got.Info().SecurityProtocol, "incorrect SecurityProtocol")
+		})
+	}
+}
+
+func Test_getTLSConfig(t *testing.T) {
+	tmpDir := t.TempDir()
+	// not mTLS scripts
+	key, cert := helpers.GenerateSelfSignedCert(t)
+	_, ca := helpers.GenerateSelfSignedCert(t)
+
+	keyContents := helpers.Cert{Name: keyFileName, Type: privateKeyType, Contents: key}
+	certContents := helpers.Cert{Name: certFileName, Type: certificateType, Contents: cert}
+	caContents := helpers.Cert{Name: caFileName, Type: certificateType, Contents: ca}
+
+	keyPath := helpers.WriteCertFiles(t, tmpDir, keyContents)
+	certPath := helpers.WriteCertFiles(t, tmpDir, certContents)
+	caPath := helpers.WriteCertFiles(t, tmpDir, caContents)
+
+	tests := map[string]struct {
+		conf    *config.TLSConfig
+		wantErr bool
+		verify  func(*testing.T, *tls.Config)
+	}{
+		"Test 1: all config should be translated": {
+			conf: &config.TLSConfig{
+				Cert:       certPath,
+				Key:        keyPath,
+				Ca:         caPath,
+				ServerName: "foobar",
+				SkipVerify: true,
+			},
+			wantErr: false,
+			verify: func(t *testing.T, c *tls.Config) {
+				require.NotEmpty(t, c.Certificates)
+				require.Equal(t, "foobar", c.ServerName, "wrong servername")
+				require.True(t, c.InsecureSkipVerify, "InsecureSkipVerify not set")
+			},
+		},
+		"Test 2: CA only config should use CA": {
+			conf: &config.TLSConfig{
+				Ca: caPath,
+			},
+			wantErr: false,
+			verify: func(t *testing.T, c *tls.Config) {
+				require.NotNil(t, c.RootCAs, "RootCAs should be initialized")
+				require.Len(t, c.RootCAs.Subjects(), 1, "RootCAs pool should contain at least one subject")
+				require.False(t, c.InsecureSkipVerify, "InsecureSkipVerify should not be set")
+			},
+		},
+		"Test 3: incorrect CA should not error": { // REALLY ?!
+			conf: &config.TLSConfig{
+				Ca: "customca.pem",
+			},
+			wantErr: false,
+			verify: func(t *testing.T, c *tls.Config) {
+				require.Nil(t, c.RootCAs, "RootCAs should be nil to use system")
+			},
+		},
+		"Test 4: incorrect key path should error": {
+			conf: &config.TLSConfig{
+				Ca:   caPath,
+				Cert: certPath,
+				Key:  "badkey",
+			},
+			wantErr: true,
+		},
+		"Test 5: incorrect cert path should error": {
+			conf: &config.TLSConfig{
+				Ca:   caPath,
+				Cert: "badcert",
+				Key:  keyPath,
+			},
+			wantErr: true,
+		},
+	}
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+			got, err := getTLSConfigForCredentials(tt.conf)
+			if tt.wantErr {
+				require.Error(t, err, "getTLSConfigForCredentials(%v)", tt.conf)
 				return
 			}
-			assert.Equalf(t, tt.want, got, "getTransportCredentials(%v)", tt.conf)
+			require.NoError(t, err, "getTLSConfigForCredentials(%v)", tt.conf)
+			if tt.verify != nil {
+				tt.verify(t, got)
+			}
 		})
 	}
 }

test/helpers/cert_utils.go

@@ -11,10 +11,9 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
-	"fmt"
 	"math/big"
 	"os"
-	"strings"
+	"path"
 	"testing"
 	"time"
 
@@ -73,12 +72,7 @@ func WriteCertFiles(t *testing.T, location string, cert Cert) string {
 		Bytes: cert.Contents,
 	})
 
-	var certFile string
-	if strings.HasSuffix(location, string(os.PathSeparator)) {
-		certFile = fmt.Sprintf("%s%s", location, cert.Name)
-	} else {
-		certFile = fmt.Sprintf("%s%s%s", location, string(os.PathSeparator), cert.Name)
-	}
+	certFile := path.Join(location, cert.Name)
 
 	err := os.WriteFile(certFile, pemContents, permission)
 	require.NoError(t, err)