From 83edca6e80510b62bfd3977d11a73057e4ed3e13 Mon Sep 17 00:00:00 2001 From: TwiN Date: Sat, 22 Apr 2023 15:22:09 -0400 Subject: [PATCH] refactor: Modify implementation of TLS (#457) * refactor: Don't generate certificates programmatically * build: Add testdata folder to .dockerignore --- .dockerignore | 3 +- config/web/web.go | 45 ++++++++++++---------- config/web/web_test.go | 37 +++++++++++------- controller/controller.go | 6 +-- controller/controller_test.go | 68 ++++++++++++++++++--------------- test/tls.go | 72 ----------------------------------- testdata/badcert.key | 3 ++ testdata/badcert.pem | 3 ++ testdata/cert.key | 5 +++ testdata/cert.pem | 10 +++++ 10 files changed, 109 insertions(+), 143 deletions(-) delete mode 100644 test/tls.go create mode 100644 testdata/badcert.key create mode 100644 testdata/badcert.pem create mode 100644 testdata/cert.key create mode 100644 testdata/cert.pem diff --git a/.dockerignore b/.dockerignore index 96af659d..706bbc44 100644 --- a/.dockerignore +++ b/.dockerignore @@ -4,4 +4,5 @@ Dockerfile .idea .git web/app -*.db \ No newline at end of file +*.db +testdata \ No newline at end of file diff --git a/config/web/web.go b/config/web/web.go index ae53eae3..2e9391fb 100644 --- a/config/web/web.go +++ b/config/web/web.go @@ -2,6 +2,7 @@ package web import ( "crypto/tls" + "errors" "fmt" "math" ) @@ -23,20 +24,18 @@ type Config struct { // Port to listen on (default to 8080 specified by DefaultPort) Port int `yaml:"port"` - // TLS configuration - Tls TLSConfig `yaml:"tls"` - - tlsConfig *tls.Config - tlsConfigError error + // TLS configuration (optional) + TLS *TLSConfig `yaml:"tls,omitempty"` } type TLSConfig struct { - - // Optional public certificate for TLS in PEM format. + // CertificateFile is the public certificate for TLS in PEM format. CertificateFile string `yaml:"certificate-file,omitempty"` - // Optional private key file for TLS in PEM format. + // PrivateKeyFile is the private key file for TLS in PEM format. PrivateKeyFile string `yaml:"private-key-file,omitempty"` + + tlsConfig *tls.Config } // GetDefaultConfig returns a Config struct with the default values @@ -57,9 +56,10 @@ func (web *Config) ValidateAndSetDefaults() error { return fmt.Errorf("invalid port: value should be between %d and %d", 0, math.MaxUint16) } // Try to load the TLS certificates - _, err := web.TLSConfig() - if err != nil { - return fmt.Errorf("invalid tls config: %w", err) + if web.TLS != nil { + if err := web.TLS.loadConfig(); err != nil { + return fmt.Errorf("invalid tls config: %w", err) + } } return nil } @@ -69,18 +69,21 @@ func (web *Config) SocketAddress() string { return fmt.Sprintf("%s:%d", web.Address, web.Port) } -// TLSConfig returns a tls.Config object for serving over an encrypted channel -func (web *Config) TLSConfig() (*tls.Config, error) { - if web.tlsConfig == nil && len(web.Tls.CertificateFile) > 0 && len(web.Tls.PrivateKeyFile) > 0 { - web.loadTLSConfig() +func (t *TLSConfig) loadConfig() error { + if len(t.CertificateFile) > 0 && len(t.PrivateKeyFile) > 0 { + certificate, err := tls.LoadX509KeyPair(t.CertificateFile, t.PrivateKeyFile) + if err != nil { + return err + } + t.tlsConfig = &tls.Config{Certificates: []tls.Certificate{certificate}} + return nil } - return web.tlsConfig, web.tlsConfigError + return errors.New("certificate-file and private-key-file must be specified") } -func (web *Config) loadTLSConfig() { - cer, err := tls.LoadX509KeyPair(web.Tls.CertificateFile, web.Tls.PrivateKeyFile) - if err != nil { - web.tlsConfigError = err +func (web *Config) TLSConfig() *tls.Config { + if web.TLS != nil { + return web.TLS.tlsConfig } - web.tlsConfig = &tls.Config{Certificates: []tls.Certificate{cer}} + return nil } diff --git a/config/web/web_test.go b/config/web/web_test.go index 3ac3b4a7..9a8f2a34 100644 --- a/config/web/web_test.go +++ b/config/web/web_test.go @@ -2,8 +2,6 @@ package web import ( "testing" - - "github.com/TwiN/gatus/v5/test" ) func TestGetDefaultConfig(t *testing.T) { @@ -14,7 +12,7 @@ func TestGetDefaultConfig(t *testing.T) { if defaultConfig.Address != DefaultAddress { t.Error("expected default config to have the default address") } - if defaultConfig.Tls != (TLSConfig{}) { + if defaultConfig.TLS != nil { t.Error("expected default config to have TLS disabled") } } @@ -70,38 +68,51 @@ func TestConfig_SocketAddress(t *testing.T) { } func TestConfig_TLSConfig(t *testing.T) { - privateKeyPath, publicKeyPath := test.UnsafeSelfSignedCertificates(t.TempDir()) - scenarios := []struct { name string cfg *Config expectedErr bool }{ { - name: "including TLS", - cfg: &Config{Tls: (TLSConfig{CertificateFile: publicKeyPath, PrivateKeyFile: privateKeyPath})}, + name: "good-tls-config", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "../../testdata/cert.pem", PrivateKeyFile: "../../testdata/cert.key"}}, expectedErr: false, }, { - name: "TLS with missing crt file", - cfg: &Config{Tls: (TLSConfig{CertificateFile: "doesnotexist", PrivateKeyFile: privateKeyPath})}, + name: "missing-crt-file", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "doesnotexist", PrivateKeyFile: "../../testdata/cert.key"}}, expectedErr: true, }, { - name: "TLS with missing key file", - cfg: &Config{Tls: (TLSConfig{CertificateFile: publicKeyPath, PrivateKeyFile: "doesnotexist"})}, + name: "bad-crt-file", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "../../testdata/badcert.pem", PrivateKeyFile: "../../testdata/cert.key"}}, + expectedErr: true, + }, + { + name: "missing-private-key-file", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "../../testdata/cert.pem", PrivateKeyFile: "doesnotexist"}}, + expectedErr: true, + }, + { + name: "bad-private-key-file", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "../../testdata/cert.pem", PrivateKeyFile: "../../testdata/badcert.key"}}, + expectedErr: true, + }, + { + name: "bad-cert-and-private-key-file", + cfg: &Config{TLS: &TLSConfig{CertificateFile: "../../testdata/badcert.pem", PrivateKeyFile: "../../testdata/badcert.key"}}, expectedErr: true, }, } for _, scenario := range scenarios { t.Run(scenario.name, func(t *testing.T) { - cfg, err := scenario.cfg.TLSConfig() + err := scenario.cfg.ValidateAndSetDefaults() if (err != nil) != scenario.expectedErr { t.Errorf("expected the existence of an error to be %v, got %v", scenario.expectedErr, err) return } if !scenario.expectedErr { - if cfg == nil { + if scenario.cfg.TLS.tlsConfig == nil { t.Error("TLS configuration was not correctly loaded although no error was returned") } } diff --git a/controller/controller.go b/controller/controller.go index d5827b1a..4d152ddc 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -24,11 +24,7 @@ func Handle(cfg *config.Config) { if os.Getenv("ENVIRONMENT") == "dev" { router = handler.DevelopmentCORS(router) } - tlsConfig, err := cfg.Web.TLSConfig() - if err != nil { - panic(err) // Should be unreachable, because the config is validated before - } - + tlsConfig := cfg.Web.TLSConfig() server = &http.Server{ Addr: fmt.Sprintf("%s:%d", cfg.Web.Address, cfg.Web.Port), TLSConfig: tlsConfig, diff --git a/controller/controller_test.go b/controller/controller_test.go index 81758116..ca398c20 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -10,7 +10,6 @@ import ( "github.com/TwiN/gatus/v5/config" "github.com/TwiN/gatus/v5/config/web" "github.com/TwiN/gatus/v5/core" - "github.com/TwiN/gatus/v5/test" ) func TestHandle(t *testing.T) { @@ -46,38 +45,45 @@ func TestHandle(t *testing.T) { } } -func TestHandleTls(t *testing.T) { - privateKeyPath, publicKeyPath := test.UnsafeSelfSignedCertificates(t.TempDir()) - cfg := &config.Config{ - Web: &web.Config{ - Address: "0.0.0.0", - Port: rand.Intn(65534), - Tls: (web.TLSConfig{CertificateFile: publicKeyPath, PrivateKeyFile: privateKeyPath}), - }, - Endpoints: []*core.Endpoint{ - { - Name: "frontend", - Group: "core", - }, - { - Name: "backend", - Group: "core", - }, +func TestHandleTLS(t *testing.T) { + scenarios := []struct { + name string + tls *web.TLSConfig + expectedStatusCode int + }{ + { + name: "good-tls-config", + tls: &web.TLSConfig{CertificateFile: "../testdata/cert.pem", PrivateKeyFile: "../testdata/cert.key"}, + expectedStatusCode: 200, }, } - _ = os.Setenv("ROUTER_TEST", "true") - _ = os.Setenv("ENVIRONMENT", "dev") - defer os.Clearenv() - Handle(cfg) - defer Shutdown() - request, _ := http.NewRequest("GET", "/health", http.NoBody) - responseRecorder := httptest.NewRecorder() - server.Handler.ServeHTTP(responseRecorder, request) - if responseRecorder.Code != http.StatusOK { - t.Error("expected GET /health to return status code 200") - } - if server == nil { - t.Fatal("server should've been set (but because we set ROUTER_TEST, it shouldn't have been started)") + for _, scenario := range scenarios { + t.Run(scenario.name, func(t *testing.T) { + cfg := &config.Config{ + Web: &web.Config{Address: "0.0.0.0", Port: rand.Intn(65534), TLS: scenario.tls}, + Endpoints: []*core.Endpoint{ + {Name: "frontend", Group: "core"}, + {Name: "backend", Group: "core"}, + }, + } + if err := cfg.Web.ValidateAndSetDefaults(); err != nil { + t.Error("expected no error from web (TLS) validation, got", err) + } + _ = os.Setenv("ROUTER_TEST", "true") + _ = os.Setenv("ENVIRONMENT", "dev") + defer os.Clearenv() + Handle(cfg) + defer Shutdown() + request, _ := http.NewRequest("GET", "/health", http.NoBody) + responseRecorder := httptest.NewRecorder() + server.Handler.ServeHTTP(responseRecorder, request) + if responseRecorder.Code != scenario.expectedStatusCode { + t.Errorf("expected GET /health to return status code %d, got %d", scenario.expectedStatusCode, responseRecorder.Code) + } + if server == nil { + t.Fatal("server should've been set (but because we set ROUTER_TEST, it shouldn't have been started)") + } + }) } } diff --git a/test/tls.go b/test/tls.go deleted file mode 100644 index 380fa7b8..00000000 --- a/test/tls.go +++ /dev/null @@ -1,72 +0,0 @@ -package test - -import ( - "crypto/ecdsa" - "crypto/elliptic" - "crypto/rand" - "crypto/x509" - "crypto/x509/pkix" - "encoding/pem" - "fmt" - "log" - "math/big" - "os" - "time" -) - -// UnsafeSelfSignedCertificates creates a pair of test certificates in the given test folder -func UnsafeSelfSignedCertificates(testfolder string) (privateKeyPath string, publicKeyPath string) { - privateKeyPath = fmt.Sprintf("%s/cert.key", testfolder) - publicKeyPath = fmt.Sprintf("%s/cert.pem", testfolder) - - key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - if err != nil { - log.Fatalf("Failed to generatekey: %v", err) - } - - template := x509.Certificate{ - SerialNumber: big.NewInt(1234), - Subject: pkix.Name{ - Organization: []string{"Gatus test"}, - }, - NotBefore: time.Now(), - NotAfter: time.Now().Add(time.Hour * 24), - KeyUsage: x509.KeyUsageDigitalSignature, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, - BasicConstraintsValid: true, - DNSNames: []string{"localhost"}, - } - derBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) - if err != nil { - log.Fatalf("Failed to create certificate: %v", err) - } - - certOut, err := os.Create(publicKeyPath) - if err != nil { - log.Fatalf("Failed to open cert.pem for writing: %v", err) - } - if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: derBytes}); err != nil { - log.Fatalf("Failed to write data to cert.pem: %v", err) - } - if err := certOut.Close(); err != nil { - log.Fatalf("Error closing cert.pem: %v", err) - } - - keyOut, err := os.OpenFile(privateKeyPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) - if err != nil { - log.Fatalf("Failed to open %s for writing: %v", privateKeyPath, err) - } - privBytes, err := x509.MarshalPKCS8PrivateKey(key) - if err != nil { - log.Fatalf("Unable to marshal private key: %v", err) - } - if err := pem.Encode(keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: privBytes}); err != nil { - log.Fatalf("Failed to write data to key.pem: %v", err) - } - if err := keyOut.Close(); err != nil { - log.Fatalf("Error closing key.pem: %v", err) - } - log.Print("wrote key.pem\n") - - return -} diff --git a/testdata/badcert.key b/testdata/badcert.key new file mode 100644 index 00000000..d7a9a79b --- /dev/null +++ b/testdata/badcert.key @@ -0,0 +1,3 @@ +-----BEGIN PRIVATE KEY----- +wat +-----END PRIVATE KEY----- \ No newline at end of file diff --git a/testdata/badcert.pem b/testdata/badcert.pem new file mode 100644 index 00000000..002a8474 --- /dev/null +++ b/testdata/badcert.pem @@ -0,0 +1,3 @@ +-----BEGIN CERTIFICATE----- +wat +-----END CERTIFICATE----- \ No newline at end of file diff --git a/testdata/cert.key b/testdata/cert.key new file mode 100644 index 00000000..20522ab5 --- /dev/null +++ b/testdata/cert.key @@ -0,0 +1,5 @@ +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgJh67FWpz8wrN1mM/ +CebkZN0zF83691ZVD83XlbNLRUqhRANCAAScfyPxScqz+Z/yNtAID/FOORy9J6LM +DUAJevGDvAZCMp/nh+Ps3nLrMoRlykcux3mq+N8HPlJ8R3eetB4S1tHY +-----END PRIVATE KEY----- \ No newline at end of file diff --git a/testdata/cert.pem b/testdata/cert.pem new file mode 100644 index 00000000..54c84fed --- /dev/null +++ b/testdata/cert.pem @@ -0,0 +1,10 @@ +-----BEGIN CERTIFICATE----- +MIIBaDCCAQ2gAwIBAgICBNIwCgYIKoZIzj0EAwIwFTETMBEGA1UEChMKR2F0dXMg +dGVzdDAgFw0yMzA0MjIxODUwMDVaGA8yMjk3MDIwNDE4NTAwNVowFTETMBEGA1UE +ChMKR2F0dXMgdGVzdDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABJx/I/FJyrP5 +n/I20AgP8U45HL0noswNQAl68YO8BkIyn+eH4+zecusyhGXKRy7Hear43wc+UnxH +d560HhLW0dijSzBJMA4GA1UdDwEB/wQEAwIHgDATBgNVHSUEDDAKBggrBgEFBQcD +ATAMBgNVHRMBAf8EAjAAMBQGA1UdEQQNMAuCCWxvY2FsaG9zdDAKBggqhkjOPQQD +AgNJADBGAiEA/SdthKOoNw3azSHuPid7XJsXYB8DisIC9LBwcb/QTMECIQCAB36Y +OI15ao+J/RUz2sXdPXCAN8hlohi6OnmZmJB32g== +-----END CERTIFICATE----- \ No newline at end of file