From 13d2e194850626c91df571a194c9fc49554ea334 Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Wed, 21 Aug 2024 18:02:49 +0100 Subject: [PATCH 1/9] feat: WIP add gencert function --- Makefile.containers | 4 +- internal/collector/otel_collector_plugin.go | 8 + internal/collector/otelcol.tmpl | 27 ++- internal/config/config.go | 42 ++++ internal/config/config_test.go | 2 +- internal/config/defaults.go | 6 +- internal/config/testdata/nginx-agent.conf | 5 +- internal/config/types.go | 16 +- pkg/tls/self_signed_cert.go | 203 +++++++++++++++++ pkg/tls/self_signed_cert_test.go | 227 ++++++++++++++++++++ 10 files changed, 529 insertions(+), 11 deletions(-) create mode 100644 pkg/tls/self_signed_cert.go create mode 100644 pkg/tls/self_signed_cert_test.go diff --git a/Makefile.containers b/Makefile.containers index 6063fd10ca..4d1d52f1f1 100644 --- a/Makefile.containers +++ b/Makefile.containers @@ -10,8 +10,8 @@ endif ifeq ($(CONTAINER_CLITOOL), docker) CONTAINER_BUILDENV ?= DOCKER_BUILDKIT=1 BUILDKIT_PROGRESS=plain -ifeq ($(shell docker-compose -v >/dev/null 2>&1 || echo FAIL),) -CONTAINER_COMPOSE = docker-compose +ifeq ($(shell docker -v >/dev/null 2>&1 || echo FAIL),) +CONTAINER_COMPOSE = docker compose endif else ifeq ($(CONTAINER_CLITOOL), podman) ifeq ($(shell podman-compose -v >/dev/null 2>&1 || echo FAIL),) diff --git a/internal/collector/otel_collector_plugin.go b/internal/collector/otel_collector_plugin.go index b3358656a8..02724a827d 100644 --- a/internal/collector/otel_collector_plugin.go +++ b/internal/collector/otel_collector_plugin.go @@ -66,6 +66,14 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro return fmt.Errorf("write OTel Collector config: %w", err) } + if oc.config.Collector.Receivers.OtlpReceivers != nil { + for _, receiver := range oc.config.Collector.Receivers.OtlpReceivers { + if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { + slog.Warn("Self-signed certificate for OTEL receiver in use") + } + } + } + bootErr := oc.bootup(runCtx) if bootErr != nil { slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr) diff --git a/internal/collector/otelcol.tmpl b/internal/collector/otelcol.tmpl index 726d81bf44..b754a947db 100644 --- a/internal/collector/otelcol.tmpl +++ b/internal/collector/otelcol.tmpl @@ -19,10 +19,35 @@ receivers: protocols: {{- if eq .Server.Type 0 }} grpc: + endpoint: "{{ .Server.Host }}:{{ .Server.Port }}" + {{- if and .OtlpTLSConfig (or (gt (len .OtlpTLSConfig.Key) 0) (gt (len .OtlpTLSConfig.Cert) 0) (gt (len .OtlpTLSConfig.Key) 0)) }} + tls: + {{ if gt (len .OtlpTLSConfig.Cert) 0 -}} + cert_file: {{ .OtlpTLSConfig.Cert }} + {{- end }} + {{ if gt (len .OtlpTLSConfig.Key) 0 -}} + key_file: {{ .OtlpTLSConfig.Key }} + {{- end }} + {{ if gt (len .OtlpTLSConfig.Ca) 0 -}} + ca_file: {{ .OtlpTLSConfig.Ca }} + {{- end }} + {{- end }} {{- else }} http: - {{- end }} endpoint: "{{- .Server.Host -}}:{{- .Server.Port -}}" + {{- if .OtlpTLSConfig }} + tls: + {{ if gt (len .OtlpTLSConfig.Cert) 0 -}} + cert_file: {{ .OtlpTLSConfig.Cert }} + {{- end }} + {{ if gt (len .OtlpTLSConfig.Key) 0 -}} + key_file: {{ .OtlpTLSConfig.Key }} + {{- end }} + {{ if gt (len .OtlpTLSConfig.Ca) 0 -}} + ca_file: {{ .OtlpTLSConfig.Ca }} + {{- end }} + {{- end }} + {{- end }} {{- end }} {{- range .Receivers.NginxReceivers }} nginx/{{- .InstanceID -}}: diff --git a/internal/config/config.go b/internal/config/config.go index 60677235b6..a1b51df353 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,9 +12,11 @@ import ( "log/slog" "os" "path/filepath" + "slices" "strings" "time" + selfsignedcerts "github.com/nginx/agent/v3/pkg/tls" uuidLibrary "github.com/nginx/agent/v3/pkg/uuid" "github.com/spf13/cobra" flag "github.com/spf13/pflag" @@ -326,6 +328,11 @@ func resolveCollector(allowedDirs []string) (*Collector, error) { Health: &healthCheck, } + // Check for self-signed certificate true in Agent conf + if err = handleSelfSignedCertificates(col); err != nil { + return nil, err + } + err = col.Validate(allowedDirs) if err != nil { return nil, fmt.Errorf("collector config: %w", err) @@ -334,6 +341,41 @@ func resolveCollector(allowedDirs []string) (*Collector, error) { return col, nil } +// generate self-signed certificate for OTEL receiver +// nolint: revive +func handleSelfSignedCertificates(col *Collector) error { + sanNames := []string{"127.0.0.1", "::1", "localhost"} + + if col.Receivers.OtlpReceivers != nil { + for _, receiver := range col.Receivers.OtlpReceivers { + if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { + if !slices.Contains(sanNames, receiver.Server.Host) { + sanNames = append(sanNames, receiver.Server.Host) + } + + // Update viper's TLS paths with defaults + receiver.OtlpTLSConfig.Ca = DefCollectorTLSCAPath + receiver.OtlpTLSConfig.Cert = DefCollectorTLSCertPath + receiver.OtlpTLSConfig.Key = DefCollectorTLSKeyPath + } + } + } + + if len(sanNames) > 0 { + err := selfsignedcerts.GenerateServerCert( + sanNames, + DefCollectorTLSCAPath, + DefCollectorTLSCertPath, + DefCollectorTLSKeyPath, + ) + if err != nil { + return fmt.Errorf("failed to generate self-signed certificate: %w", err) + } + } + + return nil +} + func resolveCommand() *Command { if !viperInstance.IsSet(CommandRootKey) { return nil diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 6995ff783a..ee0447f42a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -333,7 +333,7 @@ func getAgentConfig() *Config { Auth: &AuthConfig{ Token: "even-secreter-token", }, - TLS: &TLSConfig{ + OtlpTLSConfig: &OtlpTLSConfig{ Cert: "/path/to/server-cert.pem", Key: "/path/to/server-cert.pem", Ca: "/path/to/server-cert.pem", diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 649fde8812..83a134780f 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -13,7 +13,11 @@ const ( DefNginxReloadMonitoringPeriod = 10 * time.Second DefTreatErrorsAsWarnings = true - DefCollectorConfigPath = "/var/run/nginx-agent/otelcol.yaml" + DefCollectorConfigPath = "/var/run/nginx-agent/otelcol.yaml" + DefCollectorTLSGenSelfSignedCert = false + DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" + DefCollectorTLSKeyPath = "/var/lib/nginx-agent/key.pem" + DefCollectorTLSCAPath = "/var/lib/nginx-agent/ca.pem" DefCommandServerHostKey = "" DefCommandServerPortKey = 0 diff --git a/internal/config/testdata/nginx-agent.conf b/internal/config/testdata/nginx-agent.conf index 9aea099a21..41c92e7337 100644 --- a/internal/config/testdata/nginx-agent.conf +++ b/internal/config/testdata/nginx-agent.conf @@ -27,10 +27,9 @@ collector: Token: "secret-receiver-token" tls: server_name: "test-local-server" - skip_verify: false - cert: /path/to/server-cert.pem - key: /path/to/server-key.pem ca: /path/to/server-cert.pem + cert: /var/lib/nginx-agent/cert.pem + key: /var/lib/nginx-agent/key.pem processors: - type: batch exporters: diff --git a/internal/config/types.go b/internal/config/types.go index c5b60c2a26..3657808e3c 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -105,9 +105,9 @@ type ( } OtlpReceiver struct { - Server *ServerConfig `yaml:"-" mapstructure:"server"` - Auth *AuthConfig `yaml:"-" mapstructure:"auth"` - TLS *TLSConfig `yaml:"-" mapstructure:"tls"` + Server *ServerConfig `yaml:"-" mapstructure:"server"` + Auth *AuthConfig `yaml:"-" mapstructure:"auth"` + OtlpTLSConfig *OtlpTLSConfig `yaml:"-" mapstructure:"tls"` } NginxReceiver struct { @@ -156,6 +156,16 @@ type ( SkipVerify bool `yaml:"-" mapstructure:"skip_verify"` } + // Specialized TLS configuration for OtlpReceiver with self-signed cert generation. + OtlpTLSConfig struct { + Cert string `yaml:"-" mapstructure:"cert"` + Key string `yaml:"-" mapstructure:"key"` + Ca string `yaml:"-" mapstructure:"ca"` + ServerName string `yaml:"-" mapstructure:"server_name"` + SkipVerify bool `yaml:"-" mapstructure:"skip_verify"` + GenerateSelfSignedCert bool `yaml:"-" mapstructure:"generate_self_signed_cert"` + } + File struct { Location string `yaml:"-" mapstructure:"location"` } diff --git a/pkg/tls/self_signed_cert.go b/pkg/tls/self_signed_cert.go new file mode 100644 index 0000000000..5855f041f5 --- /dev/null +++ b/pkg/tls/self_signed_cert.go @@ -0,0 +1,203 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +// Package gencert generates self-signed TLS certificates. +package tls + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "errors" + "fmt" + "log/slog" + "math/big" + "os" + "time" +) + +const ( + caOrganization = "F5 Inc. CA" + certOrganization = "F5 Inc." + certFilePermissions = 0o600 + keyFilePermissions = 0o600 +) + +type certReq struct { + template *x509.Certificate + parent *x509.Certificate + publicKey *ecdsa.PublicKey + privateKey *ecdsa.PrivateKey +} + +func genCert(req *certReq) (*x509.Certificate, []byte) { + certBytes, createCertErr := x509.CreateCertificate( + rand.Reader, + req.template, + req.parent, + req.publicKey, + req.privateKey, + ) + + if createCertErr != nil { + slog.Error("Failed to generate certificate", "error", createCertErr) + return &x509.Certificate{}, []byte{} + } + + cert, parseCertErr := x509.ParseCertificate(certBytes) + if parseCertErr != nil { + slog.Error("Failed to parse certificate") + return &x509.Certificate{}, []byte{} + } + + b := pem.Block{Type: "CERTIFICATE", Bytes: certBytes} + certPEM := pem.EncodeToMemory(&b) + + return cert, certPEM +} + +func GenerateCA(now time.Time, caCertPath string) (*x509.Certificate, *ecdsa.PrivateKey, error) { + // Generate key pair for the CA + caKeyPair, caKeyErr := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if caKeyErr != nil { + return &x509.Certificate{}, &ecdsa.PrivateKey{}, fmt.Errorf("failed to generate CA private key: %w", caKeyErr) + } + + // Create CA certificate template + caTemplate := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{Organization: []string{certOrganization}}, + NotBefore: now.Add(-time.Minute), + NotAfter: now.AddDate(1, 0, 0), // 1 year + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + IsCA: true, + MaxPathLen: 1, + } + + // CA is self signed + caRequest := certReq{ + template: &caTemplate, + parent: &caTemplate, + publicKey: &caKeyPair.PublicKey, + privateKey: caKeyPair, + } + + caCert, caCertPEM := genCert(&caRequest) + if len(caCertPEM) == 0 { + slog.Error("Error generating certificate authority") + } + + // Write the CA certificate to a file + slog.Debug("About to write CA file", "path", caCertPath) + writeCAErr := os.WriteFile(caCertPath, caCertPEM, certFilePermissions) + if writeCAErr != nil { + return &x509.Certificate{}, &ecdsa.PrivateKey{}, fmt.Errorf( + "failed to write ca file: %w", + writeCAErr, + ) + } + + return caCert, caKeyPair, nil +} + +// nolint: revive +func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) error { + // Check for and return existing cert if it already exists + existingCertErr := ReturnExistingCert(certPath) + if existingCertErr != nil { + return fmt.Errorf("error reading existing certificate data: %w", existingCertErr) + } + + // Get the local time zone + location_currentzone, locErr := time.LoadLocation("Local") + if locErr != nil { + return fmt.Errorf("error detecting local timezone: %w", locErr) + } + now := time.Now().In(location_currentzone) + + // Create CA first + caCert, caKeyPair, caErr := GenerateCA(now, caPath) + if caErr != nil { + return fmt.Errorf("error generating certificate authority: %w", caErr) + } + + // Generate key pair for the server certficate + servKeyPair, servKeyErr := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if servKeyErr != nil { + return fmt.Errorf("failed to generate server keypair: %w", servKeyErr) + } + + servTemplate := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{ + Organization: []string{caOrganization}, + }, + NotBefore: now.Add(-time.Minute), + NotAfter: now.AddDate(1, 0, 0), // 1 year + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + DNSNames: hostnames, + } + + servRequest := certReq{ + template: &servTemplate, + parent: caCert, + publicKey: &servKeyPair.PublicKey, + privateKey: caKeyPair, + } + + // Generate server certficated signed by the CA + _, servCertPEM := genCert(&servRequest) + if len(servCertPEM) == 0 { + return errors.New("error generating server certificate") + } + + // Write the certificate to a file + writeCertErr := os.WriteFile(certPath, servCertPEM, certFilePermissions) + if writeCertErr != nil { + return fmt.Errorf("failed to write certificate file: %w", writeCertErr) + } + + // Write the private key to a file + servKeyBytes, marshalErr := x509.MarshalECPrivateKey(servKeyPair) + if marshalErr != nil { + return fmt.Errorf("failed to marshal private key file: %w", marshalErr) + } + b := pem.Block{Type: "EC PRIVATE KEY", Bytes: servKeyBytes} + servKeyPEM := pem.EncodeToMemory(&b) + writeKeyErr := os.WriteFile(keyPath, servKeyPEM, keyFilePermissions) + if writeKeyErr != nil { + return fmt.Errorf("failed to write key file: %w", writeKeyErr) + } + + return nil +} + +func ReturnExistingCert(certPath string) error { + if _, certErr := os.Stat(certPath); certErr == nil { + certBytes, certReadErr := os.ReadFile(certPath) + if certReadErr != nil { + return fmt.Errorf("error reading existing certificate file") + } + certPEM, _ := pem.Decode(certBytes) + if certPEM == nil { + return errors.New("error decoding certificate PEM block") + } + _, parseErr := x509.ParseCertificate(certPEM.Bytes) + if parseErr == nil { + slog.Warn("Certificate file already exists, skipping self-signed certificate generation") + return nil + } + + return fmt.Errorf("error parsing existing certificate: %w", parseErr) + } + + return nil +} diff --git a/pkg/tls/self_signed_cert_test.go b/pkg/tls/self_signed_cert_test.go new file mode 100644 index 0000000000..e22d8df532 --- /dev/null +++ b/pkg/tls/self_signed_cert_test.go @@ -0,0 +1,227 @@ +// Copyright (c) F5, Inc. +// +// This source code is licensed under the Apache License, Version 2.0 license found in the +// LICENSE file in the root directory of this source tree. + +// Package gencert generates self-signed TLS certificates. +package tls + +import ( + "encoding/pem" + "os" + "testing" + + "github.com/nginx/agent/v3/test/helpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// nolint: revive,gocognit +func TestGenerateSelfSignedCert(t *testing.T) { + // Setup temp file paths + caPath := "/tmp/test_ca.pem" + certPath := "/tmp/test_cert.pem" + keyPath := "/tmp/test_key.pem" + hostNames := []string{"localhost", "::1", "127.0.0.1"} + + // Cleanup any pre-existing files from previous tests + defer os.Remove(caPath) + defer os.Remove(certPath) + defer os.Remove(keyPath) + + // Define a struct for test cases + type testCase struct { + name string + caPath string + certPath string + keyPath string + expectedError string + setup func() error + hostNames []string + } + + tests := []testCase{ + { + name: "Test 1: CA, Cert and key file exist", + setup: func() error { + // Ensure no cert files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + // Create valid PEM files + keyBytes, certBytes := helpers.GenerateSelfSignedCert(t) + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certBytes}) + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certBytes}) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: keyBytes}) + if caErr := os.WriteFile(caPath, caPEM, 0o600); caErr != nil { + return caErr + } + if certErr := os.WriteFile(certPath, certPEM, 0o600); certErr != nil { + return certErr + } + + return os.WriteFile(keyPath, keyPEM, 0o600) + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "", + }, + { + name: "Test 2: Invalid cert data", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + // Create dummy cert files + if caErr := os.WriteFile(caPath, []byte("dummy ca"), 0o600); caErr != nil { + return caErr + } + if certErr := os.WriteFile(certPath, []byte("dummy cert"), 0o600); certErr != nil { + return certErr + } + + return os.WriteFile(keyPath, []byte("dummy key"), 0o600) + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "error decoding certificate PEM block", + }, + { + name: "Test 3: Error writing certificate file", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + return nil + }, + caPath: caPath, + certPath: "/dev/null/cert.pem", // Path that is guaranteed to fail + keyPath: keyPath, + hostNames: hostNames, + expectedError: "failed to write certificate file", + }, + { + name: "Test 4: Error writing key file", + setup: func() error { + return nil + }, + caPath: caPath, + certPath: certPath, + keyPath: "/dev/null/key/pem", // Path that is guaranteed to fail + hostNames: hostNames, + expectedError: "failed to write key file", + }, + { + name: "Test 5: Successful certificate generation", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + return nil + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "", + }, + { + name: "Test case 6: Error reading existing certificate file", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + // No read/write permissions + if certErr := os.WriteFile(certPath, []byte("dummy cert"), 0o000); certErr != nil { + return certErr + } + + return os.WriteFile(keyPath, []byte("dummy key"), 0o600) + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "error reading existing certificate file", + }, + { + name: "Test case 7: Error reading existing key file", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + if certErr := os.WriteFile(certPath, []byte("dummy cert"), 0o600); certErr != nil { + return certErr + } + + return os.WriteFile(keyPath, []byte("dummy key"), 0o000) + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "error decoding certificate PEM block", + }, + { + name: "Test case 8: Error parsing TLS key pair", + setup: func() error { + // Ensure no cert or key files exist + os.Remove(caPath) + os.Remove(certPath) + os.Remove(keyPath) + + // Write invalid PEM data to simulate parsing error + err := os.WriteFile(certPath, []byte("invalid cert data"), 0o600) + if err != nil { + return err + } + err = os.WriteFile(keyPath, []byte("invalid key data"), 0o600) + if err != nil { + return err + } + + return nil + }, + caPath: caPath, + certPath: certPath, + keyPath: keyPath, + hostNames: hostNames, + expectedError: "error decoding certificate PEM block", + }, + } + // Iterate over the test cases + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.setup() + require.NoError(t, err) + + genCertErr := GenerateServerCert(tc.hostNames, tc.caPath, tc.certPath, tc.keyPath) + + // Check the results + if tc.expectedError != "" { + require.Error(t, genCertErr) + assert.Contains(t, genCertErr.Error(), tc.expectedError) + } else { + require.NoError(t, genCertErr) + _, err = os.Stat(tc.certPath) + require.NoError(t, err) + _, err = os.Stat(tc.keyPath) + require.NoError(t, err) + } + }) + } +} From b6f83fd93d5219e94e7dcdcf21309c899eac232f Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Wed, 18 Sep 2024 13:47:41 +0100 Subject: [PATCH 2/9] add template test --- internal/collector/settings_test.go | 14 ++++++++++++++ internal/config/defaults.go | 6 +++--- pkg/tls/self_signed_cert_test.go | 2 +- .../test-opentelemetry-collector-agent.yaml | 6 +++++- test/types/config.go | 2 +- 5 files changed, 24 insertions(+), 6 deletions(-) diff --git a/internal/collector/settings_test.go b/internal/collector/settings_test.go index caa84f9e1c..1cf282af63 100644 --- a/internal/collector/settings_test.go +++ b/internal/collector/settings_test.go @@ -90,6 +90,20 @@ func TestTemplateWrite(t *testing.T) { }, }, }) + // Clear default config and test collector with TLS enabled + cfg.Collector.Receivers.OtlpReceivers = []config.OtlpReceiver{} + cfg.Collector.Receivers.OtlpReceivers = append(cfg.Collector.Receivers.OtlpReceivers, config.OtlpReceiver{ + Server: &config.ServerConfig{ + Host: "localhost", + Port: 4317, + Type: 0, + }, + OtlpTLSConfig: &config.OtlpTLSConfig{ + Cert: "/tmp/cert.pem", + Key: "/tmp/key.pem", + Ca: "/tmp/ca.pem", + }, + }) require.NotNil(t, cfg) diff --git a/internal/config/defaults.go b/internal/config/defaults.go index de16a32ffc..1be57d62cb 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -16,9 +16,9 @@ const ( DefCollectorConfigPath = "/var/run/nginx-agent/opentelemetry-collector-agent.yaml" DefCollectorTLSGenSelfSignedCert = false - DefCollectorLogLevel = "INFO" - DefCollectorLogPath = "/var/log/nginx-agent/opentelemetry-collector-agent.log" - DefConfigDirectories = "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules" + DefCollectorLogLevel = "INFO" + DefCollectorLogPath = "/var/log/nginx-agent/opentelemetry-collector-agent.log" + DefConfigDirectories = "/etc/nginx:/usr/local/etc/nginx:/usr/share/nginx/modules" DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" DefCollectorTLSKeyPath = "/var/lib/nginx-agent/key.pem" DefCollectorTLSCAPath = "/var/lib/nginx-agent/ca.pem" diff --git a/pkg/tls/self_signed_cert_test.go b/pkg/tls/self_signed_cert_test.go index e22d8df532..307d956984 100644 --- a/pkg/tls/self_signed_cert_test.go +++ b/pkg/tls/self_signed_cert_test.go @@ -154,7 +154,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, - expectedError: "error reading existing certificate file", + expectedError: "error decoding certificate PEM block", }, { name: "Test case 7: Error reading existing key file", diff --git a/test/config/collector/test-opentelemetry-collector-agent.yaml b/test/config/collector/test-opentelemetry-collector-agent.yaml index be6aacfa1a..9deb5be556 100644 --- a/test/config/collector/test-opentelemetry-collector-agent.yaml +++ b/test/config/collector/test-opentelemetry-collector-agent.yaml @@ -11,7 +11,11 @@ receivers: otlp/0: protocols: grpc: - endpoint: "localhost:4321" + endpoint: "localhost:4317" + tls: + cert_file: /tmp/cert.pem + key_file: /tmp/key.pem + ca_file: /tmp/ca.pem nginx/123: endpoint: "http://localhost:80/status" collection_interval: 10s diff --git a/test/types/config.go b/test/types/config.go index fefaa1964b..68f8830cf4 100644 --- a/test/types/config.go +++ b/test/types/config.go @@ -31,7 +31,7 @@ const ( reloadMonitoringPeriod = 400 * time.Millisecond randomPort1 = 1234 - randomPort2 = 4321 + randomPort2 = 4317 randomPort3 = 1337 ) From a7a0045cdef13bcbb4d5bcc0fca1d9fcbbf29d04 Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Wed, 18 Sep 2024 23:34:49 +0100 Subject: [PATCH 3/9] Update test data --- internal/config/config.go | 37 +++++++++++++++-------- internal/config/config_test.go | 13 ++++---- internal/config/testdata/nginx-agent.conf | 7 +++-- pkg/tls/self_signed_cert.go | 4 +-- pkg/tls/self_signed_cert_test.go | 2 +- 5 files changed, 39 insertions(+), 24 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 86f5df9d38..88624b1644 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -407,29 +407,42 @@ func resolveCollector(allowedDirs []string) (*Collector, error) { // generate self-signed certificate for OTEL receiver // nolint: revive func handleSelfSignedCertificates(col *Collector) error { - sanNames := []string{"127.0.0.1", "::1", "localhost"} - if col.Receivers.OtlpReceivers != nil { for _, receiver := range col.Receivers.OtlpReceivers { if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { - if !slices.Contains(sanNames, receiver.Server.Host) { - sanNames = append(sanNames, receiver.Server.Host) + err := processOtlpReceivers(receiver.OtlpTLSConfig) + if err != nil { + return fmt.Errorf("failed to generate self-signed certificate: %w", err) } - - // Update viper's TLS paths with defaults - receiver.OtlpTLSConfig.Ca = DefCollectorTLSCAPath - receiver.OtlpTLSConfig.Cert = DefCollectorTLSCertPath - receiver.OtlpTLSConfig.Key = DefCollectorTLSKeyPath } } } + return nil +} + +func processOtlpReceivers(tlsConfig *OtlpTLSConfig) error { + sanNames := []string{"127.0.0.1", "::1", "localhost"} + + if tlsConfig.Ca == "" { + tlsConfig.Ca = DefCollectorTLSCAPath + } + if tlsConfig.Cert == "" { + tlsConfig.Cert = DefCollectorTLSCertPath + } + if tlsConfig.Key == "" { + tlsConfig.Key = DefCollectorTLSKeyPath + } + + if !slices.Contains(sanNames, tlsConfig.ServerName) { + sanNames = append(sanNames, tlsConfig.ServerName) + } if len(sanNames) > 0 { err := selfsignedcerts.GenerateServerCert( sanNames, - DefCollectorTLSCAPath, - DefCollectorTLSCertPath, - DefCollectorTLSKeyPath, + tlsConfig.Ca, + tlsConfig.Cert, + tlsConfig.Key, ) if err != nil { return fmt.Errorf("failed to generate self-signed certificate: %w", err) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ff067fea54..3daec1c292 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -367,18 +367,19 @@ func getAgentConfig() *Config { { Server: &ServerConfig{ Host: "localhost", - Port: 4321, + Port: 4317, Type: 0, }, Auth: &AuthConfig{ Token: "even-secreter-token", }, OtlpTLSConfig: &OtlpTLSConfig{ - Cert: "/path/to/server-cert.pem", - Key: "/path/to/server-cert.pem", - Ca: "/path/to/server-cert.pem", - SkipVerify: true, - ServerName: "local-dataa-plane-server", + GenerateSelfSignedCert: false, + Cert: "/path/to/server-cert.pem", + Key: "/path/to/server-cert.pem", + Ca: "/path/to/server-cert.pem", + SkipVerify: true, + ServerName: "local-data-plane-server", }, }, }, diff --git a/internal/config/testdata/nginx-agent.conf b/internal/config/testdata/nginx-agent.conf index c0e3d51e8b..81ed647b37 100644 --- a/internal/config/testdata/nginx-agent.conf +++ b/internal/config/testdata/nginx-agent.conf @@ -29,10 +29,11 @@ collector: auth: Token: "secret-receiver-token" tls: + generate_self_signed_cert: false server_name: "test-local-server" - ca: /path/to/server-cert.pem - cert: /var/lib/nginx-agent/cert.pem - key: /var/lib/nginx-agent/key.pem + ca: /tmp/ca.pem + cert: /tmp/cert.pem + key: /tmp/key.pem nginx_receivers: - instance_id: cd7b8911-c2c5-4daf-b311-dbead151d938 stub_status: "http://localhost:4321/status" diff --git a/pkg/tls/self_signed_cert.go b/pkg/tls/self_signed_cert.go index 5855f041f5..35ef72c741 100644 --- a/pkg/tls/self_signed_cert.go +++ b/pkg/tls/self_signed_cert.go @@ -116,11 +116,11 @@ func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) er } // Get the local time zone - location_currentzone, locErr := time.LoadLocation("Local") + locationCurrentzone, locErr := time.LoadLocation("Local") if locErr != nil { return fmt.Errorf("error detecting local timezone: %w", locErr) } - now := time.Now().In(location_currentzone) + now := time.Now().In(locationCurrentzone) // Create CA first caCert, caKeyPair, caErr := GenerateCA(now, caPath) diff --git a/pkg/tls/self_signed_cert_test.go b/pkg/tls/self_signed_cert_test.go index 307d956984..f9a43eb445 100644 --- a/pkg/tls/self_signed_cert_test.go +++ b/pkg/tls/self_signed_cert_test.go @@ -154,7 +154,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, - expectedError: "error decoding certificate PEM block", + expectedError: "error reading existing certificate data", }, { name: "Test case 7: Error reading existing key file", From 986dcf076c54aaa7f05549ea0842197f23c0b55b Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Thu, 19 Sep 2024 16:10:13 +0100 Subject: [PATCH 4/9] address PR test comments --- internal/collector/otel_collector_plugin.go | 9 ++++- internal/config/config.go | 5 ++- internal/config/types.go | 1 + pkg/tls/self_signed_cert.go | 38 +++++++++++---------- pkg/tls/self_signed_cert_test.go | 16 ++++++++- 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/internal/collector/otel_collector_plugin.go b/internal/collector/otel_collector_plugin.go index c635af82d4..628547abc3 100644 --- a/internal/collector/otel_collector_plugin.go +++ b/internal/collector/otel_collector_plugin.go @@ -67,6 +67,7 @@ func New(conf *config.Config) (*Collector, error) { } // Init initializes and starts the plugin +// nolint: revive func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) error { slog.InfoContext(ctx, "Starting OTel Collector plugin") @@ -81,7 +82,13 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro if oc.config.Collector.Receivers.OtlpReceivers != nil { for _, receiver := range oc.config.Collector.Receivers.OtlpReceivers { if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { - slog.Warn("Self-signed certificate for OTEL receiver in use") + slog.WarnContext(ctx, "Self-signed certificate for OTEL receiver requested") + if receiver.OtlpTLSConfig.ExistingCert { + slog.WarnContext( + ctx, + "Certificate file already exists, skipping self-signed certificate generation", + ) + } } } } diff --git a/internal/config/config.go b/internal/config/config.go index 88624b1644..2f7c0979fe 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -438,7 +438,7 @@ func processOtlpReceivers(tlsConfig *OtlpTLSConfig) error { sanNames = append(sanNames, tlsConfig.ServerName) } if len(sanNames) > 0 { - err := selfsignedcerts.GenerateServerCert( + existingCert, err := selfsignedcerts.GenerateServerCert( sanNames, tlsConfig.Ca, tlsConfig.Cert, @@ -447,6 +447,9 @@ func processOtlpReceivers(tlsConfig *OtlpTLSConfig) error { if err != nil { return fmt.Errorf("failed to generate self-signed certificate: %w", err) } + if existingCert { + tlsConfig.ExistingCert = true + } } return nil diff --git a/internal/config/types.go b/internal/config/types.go index 0d7c19329a..7c29e24ca0 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -192,6 +192,7 @@ type ( Key string `yaml:"-" mapstructure:"key"` Ca string `yaml:"-" mapstructure:"ca"` ServerName string `yaml:"-" mapstructure:"server_name"` + ExistingCert bool `yaml:"-"` SkipVerify bool `yaml:"-" mapstructure:"skip_verify"` GenerateSelfSignedCert bool `yaml:"-" mapstructure:"generate_self_signed_cert"` } diff --git a/pkg/tls/self_signed_cert.go b/pkg/tls/self_signed_cert.go index 35ef72c741..5befc9685f 100644 --- a/pkg/tls/self_signed_cert.go +++ b/pkg/tls/self_signed_cert.go @@ -108,30 +108,33 @@ func GenerateCA(now time.Time, caCertPath string) (*x509.Certificate, *ecdsa.Pri } // nolint: revive -func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) error { +func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) (existingCert bool, err error) { // Check for and return existing cert if it already exists - existingCertErr := ReturnExistingCert(certPath) + existingCert, existingCertErr := ReturnExistingCert(certPath) if existingCertErr != nil { - return fmt.Errorf("error reading existing certificate data: %w", existingCertErr) + return false, fmt.Errorf("error reading existing certificate data: %w", existingCertErr) + } + if existingCert { + return true, nil } // Get the local time zone locationCurrentzone, locErr := time.LoadLocation("Local") if locErr != nil { - return fmt.Errorf("error detecting local timezone: %w", locErr) + return false, fmt.Errorf("error detecting local timezone: %w", locErr) } now := time.Now().In(locationCurrentzone) // Create CA first caCert, caKeyPair, caErr := GenerateCA(now, caPath) if caErr != nil { - return fmt.Errorf("error generating certificate authority: %w", caErr) + return false, fmt.Errorf("error generating certificate authority: %w", caErr) } // Generate key pair for the server certficate servKeyPair, servKeyErr := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if servKeyErr != nil { - return fmt.Errorf("failed to generate server keypair: %w", servKeyErr) + return false, fmt.Errorf("failed to generate server keypair: %w", servKeyErr) } servTemplate := x509.Certificate{ @@ -156,48 +159,47 @@ func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) er // Generate server certficated signed by the CA _, servCertPEM := genCert(&servRequest) if len(servCertPEM) == 0 { - return errors.New("error generating server certificate") + return false, errors.New("error generating server certificate") } // Write the certificate to a file writeCertErr := os.WriteFile(certPath, servCertPEM, certFilePermissions) if writeCertErr != nil { - return fmt.Errorf("failed to write certificate file: %w", writeCertErr) + return false, fmt.Errorf("failed to write certificate file: %w", writeCertErr) } // Write the private key to a file servKeyBytes, marshalErr := x509.MarshalECPrivateKey(servKeyPair) if marshalErr != nil { - return fmt.Errorf("failed to marshal private key file: %w", marshalErr) + return false, fmt.Errorf("failed to marshal private key file: %w", marshalErr) } b := pem.Block{Type: "EC PRIVATE KEY", Bytes: servKeyBytes} servKeyPEM := pem.EncodeToMemory(&b) writeKeyErr := os.WriteFile(keyPath, servKeyPEM, keyFilePermissions) if writeKeyErr != nil { - return fmt.Errorf("failed to write key file: %w", writeKeyErr) + return false, fmt.Errorf("failed to write key file: %w", writeKeyErr) } - return nil + return false, nil } -func ReturnExistingCert(certPath string) error { +func ReturnExistingCert(certPath string) (bool, error) { if _, certErr := os.Stat(certPath); certErr == nil { certBytes, certReadErr := os.ReadFile(certPath) if certReadErr != nil { - return fmt.Errorf("error reading existing certificate file") + return false, fmt.Errorf("error reading existing certificate file") } certPEM, _ := pem.Decode(certBytes) if certPEM == nil { - return errors.New("error decoding certificate PEM block") + return false, errors.New("error decoding certificate PEM block") } _, parseErr := x509.ParseCertificate(certPEM.Bytes) if parseErr == nil { - slog.Warn("Certificate file already exists, skipping self-signed certificate generation") - return nil + return true, nil } - return fmt.Errorf("error parsing existing certificate: %w", parseErr) + return false, fmt.Errorf("error parsing existing certificate: %w", parseErr) } - return nil + return false, nil } diff --git a/pkg/tls/self_signed_cert_test.go b/pkg/tls/self_signed_cert_test.go index f9a43eb445..f5e4ae82a2 100644 --- a/pkg/tls/self_signed_cert_test.go +++ b/pkg/tls/self_signed_cert_test.go @@ -8,6 +8,7 @@ package tls import ( "encoding/pem" + "fmt" "os" "testing" @@ -38,6 +39,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { expectedError string setup func() error hostNames []string + existingCert bool } tests := []testCase{ @@ -67,6 +69,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: true, expectedError: "", }, { @@ -90,6 +93,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "error decoding certificate PEM block", }, { @@ -106,6 +110,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: "/dev/null/cert.pem", // Path that is guaranteed to fail keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "failed to write certificate file", }, { @@ -117,6 +122,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: "/dev/null/key/pem", // Path that is guaranteed to fail hostNames: hostNames, + existingCert: false, expectedError: "failed to write key file", }, { @@ -133,6 +139,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "", }, { @@ -154,6 +161,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "error reading existing certificate data", }, { @@ -174,6 +182,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "error decoding certificate PEM block", }, { @@ -200,6 +209,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { certPath: certPath, keyPath: keyPath, hostNames: hostNames, + existingCert: false, expectedError: "error decoding certificate PEM block", }, } @@ -209,7 +219,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { err := tc.setup() require.NoError(t, err) - genCertErr := GenerateServerCert(tc.hostNames, tc.caPath, tc.certPath, tc.keyPath) + existingCert, genCertErr := GenerateServerCert(tc.hostNames, tc.caPath, tc.certPath, tc.keyPath) // Check the results if tc.expectedError != "" { @@ -222,6 +232,10 @@ func TestGenerateSelfSignedCert(t *testing.T) { _, err = os.Stat(tc.keyPath) require.NoError(t, err) } + fmt.Printf("tc.ExistingCert is %t\n", tc.existingCert) + if tc.existingCert { + require.True(t, existingCert) + } }) } } From ba506e50a91832fce05d7028a4de3a9643b498eb Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Thu, 19 Sep 2024 16:23:34 +0100 Subject: [PATCH 5/9] Enable otel collector in mock server --- test/mock/collector/nginx-agent.conf | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/mock/collector/nginx-agent.conf b/test/mock/collector/nginx-agent.conf index 4b8c58e696..de55b87475 100644 --- a/test/mock/collector/nginx-agent.conf +++ b/test/mock/collector/nginx-agent.conf @@ -37,6 +37,19 @@ collector: disk: {} network: {} filesystem: {} + otlp_receivers: + - server: + host: "127.0.0.1" + port: 4317 + type: 0 + auth: + Token: "secret-receiver-token" + tls: + server_name: "test-local-server" + ca: /tmp/ca.pem + cert: /tmp/cert.pem + key: /tmp/key.pem + generate_self_signed_cert: true processors: - type: batch exporters: From dfd1fcd9543f5628a170be92c2caeb5e3e96c840 Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Thu, 19 Sep 2024 17:14:05 +0100 Subject: [PATCH 6/9] Address comments in PR --- internal/collector/otel_collector_plugin.go | 5 ++- internal/config/config.go | 4 +-- internal/config/defaults.go | 1 + pkg/tls/self_signed_cert.go | 36 +++++++++++---------- pkg/tls/self_signed_cert_test.go | 2 +- 5 files changed, 27 insertions(+), 21 deletions(-) diff --git a/internal/collector/otel_collector_plugin.go b/internal/collector/otel_collector_plugin.go index 628547abc3..b0ae1a8240 100644 --- a/internal/collector/otel_collector_plugin.go +++ b/internal/collector/otel_collector_plugin.go @@ -82,7 +82,10 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro if oc.config.Collector.Receivers.OtlpReceivers != nil { for _, receiver := range oc.config.Collector.Receivers.OtlpReceivers { if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { - slog.WarnContext(ctx, "Self-signed certificate for OTEL receiver requested") + slog.WarnContext(ctx, + "Self-signed certificate for OTEL receiver requested, "+ + "this is not recommended for production environments.", + ) if receiver.OtlpTLSConfig.ExistingCert { slog.WarnContext( ctx, diff --git a/internal/config/config.go b/internal/config/config.go index 2f7c0979fe..faddd11d3a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -422,7 +422,7 @@ func handleSelfSignedCertificates(col *Collector) error { } func processOtlpReceivers(tlsConfig *OtlpTLSConfig) error { - sanNames := []string{"127.0.0.1", "::1", "localhost"} + sanNames := strings.Split(DefCollectorTLSSANNames, ",") if tlsConfig.Ca == "" { tlsConfig.Ca = DefCollectorTLSCAPath @@ -438,7 +438,7 @@ func processOtlpReceivers(tlsConfig *OtlpTLSConfig) error { sanNames = append(sanNames, tlsConfig.ServerName) } if len(sanNames) > 0 { - existingCert, err := selfsignedcerts.GenerateServerCert( + existingCert, err := selfsignedcerts.GenerateServerCerts( sanNames, tlsConfig.Ca, tlsConfig.Cert, diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 1be57d62cb..40030c5020 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -22,6 +22,7 @@ const ( DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" DefCollectorTLSKeyPath = "/var/lib/nginx-agent/key.pem" DefCollectorTLSCAPath = "/var/lib/nginx-agent/ca.pem" + DefCollectorTLSSANNames = "127.0.0.1, ::1, localhost" DefCommandServerHostKey = "" DefCommandServerPortKey = 0 diff --git a/pkg/tls/self_signed_cert.go b/pkg/tls/self_signed_cert.go index 5befc9685f..65e57c4402 100644 --- a/pkg/tls/self_signed_cert.go +++ b/pkg/tls/self_signed_cert.go @@ -15,7 +15,6 @@ import ( "encoding/pem" "errors" "fmt" - "log/slog" "math/big" "os" "time" @@ -35,7 +34,8 @@ type certReq struct { privateKey *ecdsa.PrivateKey } -func genCert(req *certReq) (*x509.Certificate, []byte) { +// Returns x509 Certificate object and bytes in PEM format +func genCert(req *certReq) (*x509.Certificate, []byte, error) { certBytes, createCertErr := x509.CreateCertificate( rand.Reader, req.template, @@ -45,22 +45,21 @@ func genCert(req *certReq) (*x509.Certificate, []byte) { ) if createCertErr != nil { - slog.Error("Failed to generate certificate", "error", createCertErr) - return &x509.Certificate{}, []byte{} + return &x509.Certificate{}, []byte{}, fmt.Errorf("error generating certificate: %w", createCertErr) } cert, parseCertErr := x509.ParseCertificate(certBytes) if parseCertErr != nil { - slog.Error("Failed to parse certificate") - return &x509.Certificate{}, []byte{} + return &x509.Certificate{}, []byte{}, fmt.Errorf("error parsing certificate: %w", parseCertErr) } b := pem.Block{Type: "CERTIFICATE", Bytes: certBytes} certPEM := pem.EncodeToMemory(&b) - return cert, certPEM + return cert, certPEM, nil } +// Generates a CA, returns x509 Certificate and private key for signing server certificates func GenerateCA(now time.Time, caCertPath string) (*x509.Certificate, *ecdsa.PrivateKey, error) { // Generate key pair for the CA caKeyPair, caKeyErr := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -89,13 +88,14 @@ func GenerateCA(now time.Time, caCertPath string) (*x509.Certificate, *ecdsa.Pri privateKey: caKeyPair, } - caCert, caCertPEM := genCert(&caRequest) - if len(caCertPEM) == 0 { - slog.Error("Error generating certificate authority") + caCert, caCertPEM, caErr := genCert(&caRequest) + if caErr != nil { + return &x509.Certificate{}, &ecdsa.PrivateKey{}, fmt.Errorf( + "error generating certificate authority: %w", + caErr) } // Write the CA certificate to a file - slog.Debug("About to write CA file", "path", caCertPath) writeCAErr := os.WriteFile(caCertPath, caCertPEM, certFilePermissions) if writeCAErr != nil { return &x509.Certificate{}, &ecdsa.PrivateKey{}, fmt.Errorf( @@ -107,10 +107,11 @@ func GenerateCA(now time.Time, caCertPath string) (*x509.Certificate, *ecdsa.Pri return caCert, caKeyPair, nil } +// Writes CA, Cert, Key to specified destinations. If cert files are already present, does nothing, returns true // nolint: revive -func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) (existingCert bool, err error) { +func GenerateServerCerts(hostnames []string, caPath, certPath, keyPath string) (existingCert bool, err error) { // Check for and return existing cert if it already exists - existingCert, existingCertErr := ReturnExistingCert(certPath) + existingCert, existingCertErr := DoesCertAlreadyExist(certPath) if existingCertErr != nil { return false, fmt.Errorf("error reading existing certificate data: %w", existingCertErr) } @@ -157,9 +158,9 @@ func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) (e } // Generate server certficated signed by the CA - _, servCertPEM := genCert(&servRequest) - if len(servCertPEM) == 0 { - return false, errors.New("error generating server certificate") + _, servCertPEM, servCertErr := genCert(&servRequest) + if servCertErr != nil { + return false, fmt.Errorf("error generating server certificate: %w", servCertErr) } // Write the certificate to a file @@ -183,7 +184,8 @@ func GenerateServerCert(hostnames []string, caPath, certPath, keyPath string) (e return false, nil } -func ReturnExistingCert(certPath string) (bool, error) { +// Returns true if a valid certificate is found at certPath +func DoesCertAlreadyExist(certPath string) (bool, error) { if _, certErr := os.Stat(certPath); certErr == nil { certBytes, certReadErr := os.ReadFile(certPath) if certReadErr != nil { diff --git a/pkg/tls/self_signed_cert_test.go b/pkg/tls/self_signed_cert_test.go index f5e4ae82a2..64911d77aa 100644 --- a/pkg/tls/self_signed_cert_test.go +++ b/pkg/tls/self_signed_cert_test.go @@ -219,7 +219,7 @@ func TestGenerateSelfSignedCert(t *testing.T) { err := tc.setup() require.NoError(t, err) - existingCert, genCertErr := GenerateServerCert(tc.hostNames, tc.caPath, tc.certPath, tc.keyPath) + existingCert, genCertErr := GenerateServerCerts(tc.hostNames, tc.caPath, tc.certPath, tc.keyPath) // Check the results if tc.expectedError != "" { From 1b22853485057bbe0c27d68ef20128d9272e7f9c Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Fri, 20 Sep 2024 12:17:07 +0100 Subject: [PATCH 7/9] fix mock config syntax --- test/mock/collector/nginx-agent.conf | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/mock/collector/nginx-agent.conf b/test/mock/collector/nginx-agent.conf index de55b87475..b2e16388d1 100644 --- a/test/mock/collector/nginx-agent.conf +++ b/test/mock/collector/nginx-agent.conf @@ -42,14 +42,14 @@ collector: host: "127.0.0.1" port: 4317 type: 0 - auth: - Token: "secret-receiver-token" - tls: - server_name: "test-local-server" - ca: /tmp/ca.pem - cert: /tmp/cert.pem - key: /tmp/key.pem - generate_self_signed_cert: true + auth: + Token: secret-receiver-token + tls: + server_name: test-local-server + ca: /tmp/ca.pem + cert: /tmp/cert.pem + key: /tmp/key.pem + generate_self_signed_cert: true processors: - type: batch exporters: From 1bdedf5762a3e3c867a16336208ee0a638c4fb7a Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Fri, 20 Sep 2024 13:06:15 +0100 Subject: [PATCH 8/9] address test comments --- internal/collector/otel_collector_plugin.go | 29 ++++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/internal/collector/otel_collector_plugin.go b/internal/collector/otel_collector_plugin.go index b0ae1a8240..7286c9ae3f 100644 --- a/internal/collector/otel_collector_plugin.go +++ b/internal/collector/otel_collector_plugin.go @@ -67,7 +67,6 @@ func New(conf *config.Config) (*Collector, error) { } // Init initializes and starts the plugin -// nolint: revive func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) error { slog.InfoContext(ctx, "Starting OTel Collector plugin") @@ -80,8 +79,23 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro } if oc.config.Collector.Receivers.OtlpReceivers != nil { - for _, receiver := range oc.config.Collector.Receivers.OtlpReceivers { - if receiver.OtlpTLSConfig != nil && receiver.OtlpTLSConfig.GenerateSelfSignedCert { + oc.processReceivers(ctx, oc.config.Collector.Receivers.OtlpReceivers) + } + + bootErr := oc.bootup(runCtx) + if bootErr != nil { + slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr) + } + + return nil +} + +// Process receivers and log warning for sub-optimal configurations +// nolint: revive +func (oc *Collector) processReceivers(ctx context.Context, receivers []config.OtlpReceiver) { + for _, receiver := range receivers { + if receiver.OtlpTLSConfig != nil { + if receiver.OtlpTLSConfig.GenerateSelfSignedCert { slog.WarnContext(ctx, "Self-signed certificate for OTEL receiver requested, "+ "this is not recommended for production environments.", @@ -93,15 +107,10 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro ) } } + } else { + slog.WarnContext(ctx, "OTEL receiver is configured without TLS. Connections are unencrypted.") } } - - bootErr := oc.bootup(runCtx) - if bootErr != nil { - slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr) - } - - return nil } func (oc *Collector) bootup(ctx context.Context) error { From 3e13ace0a9f52c51cf3c3c155cbeccbc6839f097 Mon Sep 17 00:00:00 2001 From: Christopher van de Sande Date: Fri, 20 Sep 2024 18:49:40 +0100 Subject: [PATCH 9/9] remove spaces from names --- internal/collector/otel_collector_plugin.go | 24 +++++++++++---------- internal/config/defaults.go | 2 +- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/internal/collector/otel_collector_plugin.go b/internal/collector/otel_collector_plugin.go index 7286c9ae3f..160ec3e113 100644 --- a/internal/collector/otel_collector_plugin.go +++ b/internal/collector/otel_collector_plugin.go @@ -91,21 +91,23 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro } // Process receivers and log warning for sub-optimal configurations -// nolint: revive func (oc *Collector) processReceivers(ctx context.Context, receivers []config.OtlpReceiver) { for _, receiver := range receivers { - if receiver.OtlpTLSConfig != nil { - if receiver.OtlpTLSConfig.GenerateSelfSignedCert { + if receiver.OtlpTLSConfig == nil { + slog.WarnContext(ctx, "OTEL receiver is configured without TLS. Connections are unencrypted.") + continue + } + + if receiver.OtlpTLSConfig.GenerateSelfSignedCert { + slog.WarnContext(ctx, + "Self-signed certificate for OTEL receiver requested, "+ + "this is not recommended for production environments.", + ) + + if receiver.OtlpTLSConfig.ExistingCert { slog.WarnContext(ctx, - "Self-signed certificate for OTEL receiver requested, "+ - "this is not recommended for production environments.", + "Certificate file already exists, skipping self-signed certificate generation", ) - if receiver.OtlpTLSConfig.ExistingCert { - slog.WarnContext( - ctx, - "Certificate file already exists, skipping self-signed certificate generation", - ) - } } } else { slog.WarnContext(ctx, "OTEL receiver is configured without TLS. Connections are unencrypted.") diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 40030c5020..5a2b6b7f4b 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -22,7 +22,7 @@ const ( DefCollectorTLSCertPath = "/var/lib/nginx-agent/cert.pem" DefCollectorTLSKeyPath = "/var/lib/nginx-agent/key.pem" DefCollectorTLSCAPath = "/var/lib/nginx-agent/ca.pem" - DefCollectorTLSSANNames = "127.0.0.1, ::1, localhost" + DefCollectorTLSSANNames = "127.0.0.1,::1,localhost" DefCommandServerHostKey = "" DefCommandServerPortKey = 0