From f4cf4f050f16dba853986d7ac14c4b00235bae6c Mon Sep 17 00:00:00 2001 From: John David White Date: Thu, 17 Apr 2025 14:17:21 +0100 Subject: [PATCH 1/2] Add config parser tests for SSLCerts --- .../instance/nginx_config_parser_test.go | 141 ++++++++++++++++++ .../nginx/nginx-with-multiple-ssl-certs.conf | 49 ++++++ test/config/nginx/nginx-with-ssl-certs.conf | 48 ++++++ test/config/nginx_config.go | 14 ++ 4 files changed, 252 insertions(+) create mode 100644 test/config/nginx/nginx-with-multiple-ssl-certs.conf create mode 100644 test/config/nginx/nginx-with-ssl-certs.conf diff --git a/internal/watcher/instance/nginx_config_parser_test.go b/internal/watcher/instance/nginx_config_parser_test.go index 0444b8749..a10bcea11 100644 --- a/internal/watcher/instance/nginx_config_parser_test.go +++ b/internal/watcher/instance/nginx_config_parser_test.go @@ -12,6 +12,7 @@ import ( "net/http" "net/http/httptest" "os" + "sort" "testing" "google.golang.org/protobuf/types/known/timestamppb" @@ -260,6 +261,7 @@ var ( ` ) +// nolint: maintidx func TestNginxConfigParser_Parse(t *testing.T) { ctx := context.Background() dir := t.TempDir() @@ -288,6 +290,20 @@ func TestNginxConfigParser_Parse(t *testing.T) { fileMetaAllowedFiles, err := files.FileMeta(allowedFile.Name()) require.NoError(t, err) + _, cert := helpers.GenerateSelfSignedCert(t) + certContents := helpers.Cert{Name: "nginx.cert", Type: "CERTIFICATE", Contents: cert} + certFile := helpers.WriteCertFiles(t, dir, certContents) + require.NotNil(t, certFile) + fileMetaCertFiles, err := files.FileMetaWithCertificate(certFile) + require.NoError(t, err) + + _, diffCert := helpers.GenerateSelfSignedCert(t) + diffCertContents := helpers.Cert{Name: "nginx1.cert", Type: "CERTIFICATE", Contents: diffCert} + diffCertFile := helpers.WriteCertFiles(t, dir, diffCertContents) + require.NotNil(t, diffCertFile) + diffFileMetaCertFiles, err := files.FileMetaWithCertificate(diffCertFile) + require.NoError(t, err) + tests := []struct { instance *mpi.Instance name string @@ -368,6 +384,125 @@ func TestNginxConfigParser_Parse(t *testing.T) { }, allowedDirectories: []string{dir}, }, + { + name: "Test 4: Check Parser for SSL Certs", + instance: protos.GetNginxPlusInstance([]string{}), + content: testconfig.GetNginxConfigWithSSLCerts( + errorLog.Name(), + accessLog.Name(), + certFile, + ), + expectedConfigContext: &model.NginxConfigContext{ + StubStatus: &model.APIDetails{}, + PlusAPI: &model.APIDetails{}, + InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + Files: []*mpi.File{ + { + FileMeta: fileMetaCertFiles, + }, + }, + AccessLogs: []*model.AccessLog{ + { + Name: accessLog.Name(), + Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + + "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + + "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", + Permissions: "0600", + Readable: true, + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: errorLog.Name(), + Permissions: "0600", + Readable: true, + }, + }, + NAPSysLogServers: nil, + }, + allowedDirectories: []string{dir}, + }, + { + name: "Test 5: Check for multiple different SSL Certs", + instance: protos.GetNginxPlusInstance([]string{}), + content: testconfig.GetNginxConfigWithMultipleSSLCerts( + errorLog.Name(), + accessLog.Name(), + certFile, + diffCertFile, + ), + expectedConfigContext: &model.NginxConfigContext{ + StubStatus: &model.APIDetails{}, + PlusAPI: &model.APIDetails{}, + InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + Files: []*mpi.File{ + { + FileMeta: fileMetaCertFiles, + }, + { + FileMeta: diffFileMetaCertFiles, + }, + }, + AccessLogs: []*model.AccessLog{ + { + Name: accessLog.Name(), + Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + + "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + + "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", + Permissions: "0600", + Readable: true, + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: errorLog.Name(), + Permissions: "0600", + Readable: true, + }, + }, + NAPSysLogServers: nil, + }, + allowedDirectories: []string{dir}, + }, + { + name: "Test 6: Check for multiple same SSL Certs", + instance: protos.GetNginxPlusInstance([]string{}), + content: testconfig.GetNginxConfigWithMultipleSSLCerts( + errorLog.Name(), + accessLog.Name(), + certFile, + certFile, + ), + expectedConfigContext: &model.NginxConfigContext{ + StubStatus: &model.APIDetails{}, + PlusAPI: &model.APIDetails{}, + InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + Files: []*mpi.File{ + { + FileMeta: fileMetaCertFiles, + }, + }, + AccessLogs: []*model.AccessLog{ + { + Name: accessLog.Name(), + Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + + "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + + "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", + Permissions: "0600", + Readable: true, + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: errorLog.Name(), + Permissions: "0600", + Readable: true, + }, + }, + NAPSysLogServers: nil, + }, + allowedDirectories: []string{dir}, + }, } for _, test := range tests { @@ -391,6 +526,11 @@ func TestNginxConfigParser_Parse(t *testing.T) { result, parseError := nginxConfig.Parse(ctx, test.instance) require.NoError(t, parseError) + sort.Slice(test.expectedConfigContext.Files, func(i, j int) bool { + return test.expectedConfigContext.Files[i].GetFileMeta().GetName() > + test.expectedConfigContext.Files[j].GetFileMeta().GetName() + }) + assert.ElementsMatch(t, test.expectedConfigContext.Files, result.Files) assert.Equal(t, test.expectedConfigContext.NAPSysLogServers, result.NAPSysLogServers) assert.Equal(t, test.expectedConfigContext.PlusAPI, result.PlusAPI) @@ -398,6 +538,7 @@ func TestNginxConfigParser_Parse(t *testing.T) { assert.ElementsMatch(t, test.expectedConfigContext.ErrorLogs, result.ErrorLogs) assert.Equal(t, test.expectedConfigContext.StubStatus, result.StubStatus) assert.Equal(t, test.expectedConfigContext.InstanceID, result.InstanceID) + assert.Equal(t, len(test.expectedConfigContext.Files), len(result.Files)) }) } } diff --git a/test/config/nginx/nginx-with-multiple-ssl-certs.conf b/test/config/nginx/nginx-with-multiple-ssl-certs.conf new file mode 100644 index 000000000..b74ca2f6a --- /dev/null +++ b/test/config/nginx/nginx-with-multiple-ssl-certs.conf @@ -0,0 +1,49 @@ +worker_processes 1; +error_log %s; +events { + worker_connections 1024; +} + +http { + default_type application/octet-stream; + + log_format main '$remote_addr - $remote_user [$time_local] "$request" ' + '$status $body_bytes_sent "$http_referer" ' + '"$http_user_agent" "$http_x_forwarded_for" ' + '"$bytes_sent" "$request_length" "$request_time" ' + '"$gzip_ratio" $server_protocol '; + + access_log %s main; + + sendfile on; + keepalive_timeout 65; + + server { + listen 8080; + server_name localhost; + + location / { + root /usr/share/nginx/html; + index index.html index.htm; + } + + ssl_certificate %s; + ssl_certificate %s; + + ## + # Enable Metrics + ## + location /api { + stub_status; + allow 127.0.0.1; + deny all; + } + + # redirect server error pages to the static page /50x.html + # + error_page 500 502 503 504 /50x.html; + location = /50x.html { + root /usr/share/nginx/html; + } + } +} diff --git a/test/config/nginx/nginx-with-ssl-certs.conf b/test/config/nginx/nginx-with-ssl-certs.conf new file mode 100644 index 000000000..4d74fbba5 --- /dev/null +++ b/test/config/nginx/nginx-with-ssl-certs.conf @@ -0,0 +1,48 @@ +worker_processes 1; +error_log %s; +events { + worker_connections 1024; +} + +http { + default_type application/octet-stream; + + log_format main '$remote_addr - $remote_user [$time_local] "$request" ' + '$status $body_bytes_sent "$http_referer" ' + '"$http_user_agent" "$http_x_forwarded_for" ' + '"$bytes_sent" "$request_length" "$request_time" ' + '"$gzip_ratio" $server_protocol '; + + access_log %s main; + + sendfile on; + keepalive_timeout 65; + + server { + listen 8080; + server_name localhost; + + location / { + root /usr/share/nginx/html; + index index.html index.htm; + } + + ssl_certificate %s; + + ## + # Enable Metrics + ## + location /api { + stub_status; + allow 127.0.0.1; + deny all; + } + + # redirect server error pages to the static page /50x.html + # + error_page 500 502 503 504 /50x.html; + location = /50x.html { + root /usr/share/nginx/html; + } + } +} diff --git a/test/config/nginx_config.go b/test/config/nginx_config.go index 272cd4a7c..cb168f7c8 100644 --- a/test/config/nginx_config.go +++ b/test/config/nginx_config.go @@ -16,6 +16,12 @@ var embedNginxConfWithMultipleAccessLogs string //go:embed nginx/nginx-not-allowed-dir.conf var embedNginxConfWithNotAllowedDir string +//go:embed nginx/nginx-with-ssl-certs.conf +var embedNginxConfWithSSLCerts string + +//go:embed nginx/nginx-with-multiple-ssl-certs.conf +var embedNginxConfWithMultipleSSLCerts string + func GetNginxConfigWithMultipleAccessLogs( errorLogName, accessLogName, @@ -34,3 +40,11 @@ func GetNginxConfigWithMultipleAccessLogs( func GetNginxConfigWithNotAllowedDir(errorLogFile, notAllowedFile, allowedFileDir, accessLogFile string) string { return fmt.Sprintf(embedNginxConfWithNotAllowedDir, errorLogFile, notAllowedFile, allowedFileDir, accessLogFile) } + +func GetNginxConfigWithSSLCerts(errorLogFile, accessLogFile, certFile string) string { + return fmt.Sprintf(embedNginxConfWithSSLCerts, errorLogFile, accessLogFile, certFile) +} + +func GetNginxConfigWithMultipleSSLCerts(errorLogFile, accessLogFile, certFile1, certFile2 string) string { + return fmt.Sprintf(embedNginxConfWithMultipleSSLCerts, errorLogFile, accessLogFile, certFile1, certFile2) +} From eec0457a9dfab19773eedb65af5907c7b6b2090c Mon Sep 17 00:00:00 2001 From: John David White Date: Wed, 23 Apr 2025 13:46:13 +0100 Subject: [PATCH 2/2] PR feedback --- .../instance/nginx_config_parser_test.go | 146 ++++-------------- test/model/config.go | 46 +++++- 2 files changed, 76 insertions(+), 116 deletions(-) diff --git a/internal/watcher/instance/nginx_config_parser_test.go b/internal/watcher/instance/nginx_config_parser_test.go index 759bd8981..648369edb 100644 --- a/internal/watcher/instance/nginx_config_parser_test.go +++ b/internal/watcher/instance/nginx_config_parser_test.go @@ -289,6 +289,7 @@ func TestNginxConfigParser_Parse(t *testing.T) { defer helpers.RemoveFileWithErrorCheck(t, allowedFile.Name()) fileMetaAllowedFiles, err := files.FileMeta(allowedFile.Name()) require.NoError(t, err) + allowedFileWithMetas := mpi.File{FileMeta: fileMetaAllowedFiles} _, cert := helpers.GenerateSelfSignedCert(t) certContents := helpers.Cert{Name: "nginx.cert", Type: "CERTIFICATE", Contents: cert} @@ -296,6 +297,7 @@ func TestNginxConfigParser_Parse(t *testing.T) { require.NotNil(t, certFile) fileMetaCertFiles, err := files.FileMetaWithCertificate(certFile) require.NoError(t, err) + certFileWithMetas := mpi.File{FileMeta: fileMetaCertFiles} _, diffCert := helpers.GenerateSelfSignedCert(t) diffCertContents := helpers.Cert{Name: "nginx1.cert", Type: "CERTIFICATE", Contents: diffCert} @@ -303,6 +305,7 @@ func TestNginxConfigParser_Parse(t *testing.T) { require.NotNil(t, diffCertFile) diffFileMetaCertFiles, err := files.FileMetaWithCertificate(diffCertFile) require.NoError(t, err) + diffCertFileWithMetas := mpi.File{FileMeta: diffFileMetaCertFiles} tests := []struct { instance *mpi.Instance @@ -354,34 +357,13 @@ func TestNginxConfigParser_Parse(t *testing.T) { instance: protos.GetNginxPlusInstance([]string{}), content: testconfig.GetNginxConfigWithNotAllowedDir(errorLog.Name(), allowedFile.Name(), notAllowedFile.Name(), accessLog.Name()), - expectedConfigContext: &model.NginxConfigContext{ - StubStatus: &model.APIDetails{}, - PlusAPI: &model.APIDetails{}, - InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), - Files: []*mpi.File{ - { - FileMeta: fileMetaAllowedFiles, - }, - }, - AccessLogs: []*model.AccessLog{ - { - Name: accessLog.Name(), - Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + - "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + - "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", - Permissions: "0600", - Readable: true, - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: errorLog.Name(), - Permissions: "0600", - Readable: true, - }, - }, - NAPSysLogServers: nil, - }, + expectedConfigContext: modelHelpers.GetConfigContextWithFiles( + accessLog.Name(), + errorLog.Name(), + []*mpi.File{&allowedFileWithMetas}, + protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + nil, + ), allowedDirectories: []string{dir}, }, { @@ -407,34 +389,13 @@ func TestNginxConfigParser_Parse(t *testing.T) { accessLog.Name(), certFile, ), - expectedConfigContext: &model.NginxConfigContext{ - StubStatus: &model.APIDetails{}, - PlusAPI: &model.APIDetails{}, - InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), - Files: []*mpi.File{ - { - FileMeta: fileMetaCertFiles, - }, - }, - AccessLogs: []*model.AccessLog{ - { - Name: accessLog.Name(), - Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + - "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + - "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", - Permissions: "0600", - Readable: true, - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: errorLog.Name(), - Permissions: "0600", - Readable: true, - }, - }, - NAPSysLogServers: nil, - }, + expectedConfigContext: modelHelpers.GetConfigContextWithFiles( + accessLog.Name(), + errorLog.Name(), + []*mpi.File{&certFileWithMetas}, + protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + nil, + ), allowedDirectories: []string{dir}, }, { @@ -446,37 +407,13 @@ func TestNginxConfigParser_Parse(t *testing.T) { certFile, diffCertFile, ), - expectedConfigContext: &model.NginxConfigContext{ - StubStatus: &model.APIDetails{}, - PlusAPI: &model.APIDetails{}, - InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), - Files: []*mpi.File{ - { - FileMeta: fileMetaCertFiles, - }, - { - FileMeta: diffFileMetaCertFiles, - }, - }, - AccessLogs: []*model.AccessLog{ - { - Name: accessLog.Name(), - Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + - "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + - "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", - Permissions: "0600", - Readable: true, - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: errorLog.Name(), - Permissions: "0600", - Readable: true, - }, - }, - NAPSysLogServers: nil, - }, + expectedConfigContext: modelHelpers.GetConfigContextWithFiles( + accessLog.Name(), + errorLog.Name(), + []*mpi.File{&certFileWithMetas, &diffCertFileWithMetas}, + protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + nil, + ), allowedDirectories: []string{dir}, }, { @@ -488,34 +425,13 @@ func TestNginxConfigParser_Parse(t *testing.T) { certFile, certFile, ), - expectedConfigContext: &model.NginxConfigContext{ - StubStatus: &model.APIDetails{}, - PlusAPI: &model.APIDetails{}, - InstanceID: protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), - Files: []*mpi.File{ - { - FileMeta: fileMetaCertFiles, - }, - }, - AccessLogs: []*model.AccessLog{ - { - Name: accessLog.Name(), - Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + - "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + - "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", - Permissions: "0600", - Readable: true, - }, - }, - ErrorLogs: []*model.ErrorLog{ - { - Name: errorLog.Name(), - Permissions: "0600", - Readable: true, - }, - }, - NAPSysLogServers: nil, - }, + expectedConfigContext: modelHelpers.GetConfigContextWithFiles( + accessLog.Name(), + errorLog.Name(), + []*mpi.File{&certFileWithMetas}, + protos.GetNginxPlusInstance([]string{}).GetInstanceMeta().GetInstanceId(), + nil, + ), allowedDirectories: []string{dir}, }, } diff --git a/test/model/config.go b/test/model/config.go index 5893c445f..dc48f043d 100644 --- a/test/model/config.go +++ b/test/model/config.go @@ -5,7 +5,10 @@ package model -import "github.com/nginx/agent/v3/internal/model" +import ( + mpi "github.com/nginx/agent/v3/api/grpc/mpi/v1" + "github.com/nginx/agent/v3/internal/model" +) func GetConfigContext() *model.NginxConfigContext { return &model.NginxConfigContext{ @@ -72,3 +75,44 @@ func GetConfigContextWithNames( NAPSysLogServers: syslogServers, } } + +func GetConfigContextWithFiles( + accessLogName, + errorLogName string, + files []*mpi.File, + instanceID string, + syslogServers []string, +) *model.NginxConfigContext { + return &model.NginxConfigContext{ + StubStatus: &model.APIDetails{ + URL: "", + Listen: "", + Location: "", + }, + PlusAPI: &model.APIDetails{ + URL: "", + Listen: "", + Location: "", + }, + Files: files, + AccessLogs: []*model.AccessLog{ + { + Name: accessLogName, + Format: "$remote_addr - $remote_user [$time_local] \"$request\" $status $body_bytes_sent " + + "\"$http_referer\" \"$http_user_agent\" \"$http_x_forwarded_for\" \"$bytes_sent\" " + + "\"$request_length\" \"$request_time\" \"$gzip_ratio\" $server_protocol ", + Readable: true, + Permissions: "0600", + }, + }, + ErrorLogs: []*model.ErrorLog{ + { + Name: errorLogName, + Readable: true, + Permissions: "0600", + }, + }, + InstanceID: instanceID, + NAPSysLogServers: syslogServers, + } +}