From a3594cdeaa17340d681b7a4315df98d12d5dad34 Mon Sep 17 00:00:00 2001 From: Nathan Bird Date: Wed, 14 May 2025 11:03:09 -0400 Subject: [PATCH 1/4] test: use smaller selfsigned certs for testing When generating a self-signed cert for _unit-tests_ it doesn't need to have high security. Using a smaller length makes the tests run faster. The difference is ~0.1s vs ~2.0s to run one test that generates a cert. --- test/helpers/cert_utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/cert_utils.go b/test/helpers/cert_utils.go index 04cacfa77b..6d8dd0b4ec 100644 --- a/test/helpers/cert_utils.go +++ b/test/helpers/cert_utils.go @@ -31,7 +31,7 @@ const ( permission = 0o600 serialNumber = 123123 years, months, days = 5, 0, 0 - bits = 4096 + bits = 1024 ) func GenerateSelfSignedCert(t testing.TB) (keyBytes, certBytes []byte) { From 8d5f8a2d451db01a3c13195f5495ffc77e3b1adf Mon Sep 17 00:00:00 2001 From: Nathan Bird Date: Wed, 14 May 2025 11:36:30 -0400 Subject: [PATCH 2/4] test: fix grpc mTLS dialoptions test This was incorrectly passing in the cert and key backwards which resulted in not actually getting mTLS credentials, but instead insecure credentials. When insecure credentials are used, skipToken means we don't add a addPerRPCCredentials. When it is a secure TLS credential then addPerRPCCredentials increases the dialoption count by one. --- internal/grpc/grpc_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/grpc/grpc_test.go b/internal/grpc/grpc_test.go index 4aea9ed6b6..47cd6bc131 100644 --- a/internal/grpc/grpc_test.go +++ b/internal/grpc/grpc_test.go @@ -103,7 +103,7 @@ func Test_GetDialOptions(t *testing.T) { { name: "Test 2: DialOptions mTLS", agentConfig: types.AgentConfig(), - expected: 7, + expected: 8, createCerts: true, }, { @@ -171,8 +171,8 @@ func Test_GetDialOptions(t *testing.T) { key, cert := helpers.GenerateSelfSignedCert(t) _, ca := helpers.GenerateSelfSignedCert(t) - keyContents := helpers.Cert{Name: keyFileName, Type: certificateType, Contents: key} - certContents := helpers.Cert{Name: certFileName, Type: privateKeyType, Contents: cert} + 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} helpers.WriteCertFiles(t, tmpDir, keyContents) From 50e82edf84569c28a0f02c8e5f2a242271715131 Mon Sep 17 00:00:00 2001 From: Nathan Bird Date: Tue, 13 May 2025 16:17:32 -0400 Subject: [PATCH 3/4] 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. --- internal/grpc/grpc.go | 28 ++++--- internal/grpc/grpc_test.go | 146 ++++++++++++++++++++++++++++++++----- test/helpers/cert_utils.go | 10 +-- 3 files changed, 147 insertions(+), 37 deletions(-) diff --git a/internal/grpc/grpc.go b/internal/grpc/grpc.go index 34f6f35037..0c77a2ad98 100644 --- a/internal/grpc/grpc.go +++ b/internal/grpc/grpc.go @@ -363,30 +363,34 @@ 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 + } - if agentConfig.Command.TLS.SkipVerify { + return credentials.NewTLS(tlsConfig), nil +} + +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, + ServerName: c.ServerName, + InsecureSkipVerify: c.SkipVerify, } - if agentConfig.Command.TLS.Key == "" { - return credentials.NewTLS(tlsConfig), nil - } - - 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 } diff --git a/internal/grpc/grpc_test.go b/internal/grpc/grpc_test.go index 47cd6bc131..034573250c 100644 --- a/internal/grpc/grpc_test.go +++ b/internal/grpc/grpc_test.go @@ -7,19 +7,20 @@ package grpc import ( "context" + "crypto/tls" + "crypto/x509" "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" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" + "github.com/nginx/agent/v3/test/helpers" + "github.com/nginx/agent/v3/test/protos" + "github.com/nginx/agent/v3/internal/config" "github.com/nginx/agent/v3/test/types" @@ -356,28 +357,139 @@ 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 + wantSecurityProfile string + wantServerName string + wantErr bool }{ - { - 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-existent 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 + verify func(require.TestingT, *tls.Config) + wantErr bool + }{ + "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 require.TestingT, 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 require.TestingT, c *tls.Config) { + require.NotNil(t, c.RootCAs, "RootCAs should be initialized") + require.False(t, x509.NewCertPool().Equal(c.RootCAs), + "CertPool shouldn't be empty, valid CA cert was specified") + 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 require.TestingT, 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, + }, + "Test 6: incomplete cert info should error": { + conf: &config.TLSConfig{ + 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) + } }) } } diff --git a/test/helpers/cert_utils.go b/test/helpers/cert_utils.go index 6d8dd0b4ec..c292e97a38 100644 --- a/test/helpers/cert_utils.go +++ b/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) From 1859885f4e47772f7e5ddfeebc26d6740922895b Mon Sep 17 00:00:00 2001 From: Nathan Bird Date: Wed, 21 May 2025 11:14:28 -0400 Subject: [PATCH 4/4] fix: invalid TLS CA cert should error immediately Previously if a consumer specified the CA cert to verify the command connection but that CA wasn't valid then system would log at Debug (default hidden) and proceed anyways. I don't believe this is good behavior. If the consumer is directly specifying a CA cert then that is the CA that should be used, not silently ignored. This patch returns the error up, which is now caught and swallowed at a higher level, but at least it is more visible: > time=2025-05-21T15:41:33.547Z level=ERROR msg="Unable to add transport credentials to gRPC dial options, adding default transport credentials" error="invalid CA cert while building transport credentials: read CA file (/etc/nginx-agent/bad.crt): open /etc/nginx-agent/bad.crt: no such file or directory" --- internal/grpc/grpc.go | 10 ++++------ internal/grpc/grpc_test.go | 7 ++----- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/internal/grpc/grpc.go b/internal/grpc/grpc.go index 0c77a2ad98..a93c6f9c28 100644 --- a/internal/grpc/grpc.go +++ b/internal/grpc/grpc.go @@ -382,14 +382,12 @@ func getTLSConfigForCredentials(c *config.TLSConfig) (*tls.Config, error) { InsecureSkipVerify: c.SkipVerify, } - err := appendRootCAs(tlsConfig, c.Ca) - if err != nil { - slog.Debug("Unable to append root CA", "error", err) + if err := appendRootCAs(tlsConfig, c.Ca); err != nil { + return nil, fmt.Errorf("invalid CA cert while building transport credentials: %w", err) } - err = appendCertKeyPair(tlsConfig, c.Cert, c.Key) - if err != nil { - return nil, fmt.Errorf("append cert and key pair failed: %w", err) + if err := appendCertKeyPair(tlsConfig, c.Cert, c.Key); err != nil { + return nil, fmt.Errorf("invalid client cert while building transport credentials: %w", err) } return tlsConfig, nil diff --git a/internal/grpc/grpc_test.go b/internal/grpc/grpc_test.go index 034573250c..87828ceb33 100644 --- a/internal/grpc/grpc_test.go +++ b/internal/grpc/grpc_test.go @@ -447,14 +447,11 @@ func Test_getTLSConfig(t *testing.T) { require.False(t, c.InsecureSkipVerify, "InsecureSkipVerify should not be set") }, }, - "Test 3: incorrect CA should not error": { // REALLY ?! + "Test 3: incorrect CA should not error": { conf: &config.TLSConfig{ Ca: "customca.pem", }, - wantErr: false, - verify: func(t require.TestingT, c *tls.Config) { - require.Nil(t, c.RootCAs, "RootCAs should be nil to use system") - }, + wantErr: true, }, "Test 4: incorrect key path should error": { conf: &config.TLSConfig{