From 4f3478c586fa1ff2ee76b6a10d7567e2d73f745c Mon Sep 17 00:00:00 2001 From: nfebe Date: Sat, 7 Feb 2026 11:53:04 +0100 Subject: [PATCH] fix(ssl): Resolve missing SSL blocks in vhosts Certificate requests were skipped because the orchestrator pre-disabled SSL.Enabled for domains without existing certs, and RequestCertificatesForDomains required both Enabled and AutoCert. Now only AutoCert drives certificate requests. Also default new domain container_port from legacy networking config when omitted, preventing wrong port in nginx upstream. Closes #75 Signed-off-by: nfebe --- internal/api/domains_test.go | 90 ++++++++++++++ internal/api/server.go | 4 + internal/nginx/manager_test.go | 221 +++++++++++++++++++++++++++++++++ internal/ssl/manager.go | 7 +- internal/ssl/manager_test.go | 63 ++++++++++ 5 files changed, 383 insertions(+), 2 deletions(-) diff --git a/internal/api/domains_test.go b/internal/api/domains_test.go index 06e9d18..e4654b0 100644 --- a/internal/api/domains_test.go +++ b/internal/api/domains_test.go @@ -451,4 +451,94 @@ func TestAddDomain(t *testing.T) { t.Errorf("expected status 201 Created, got %d: %s", w.Code, w.Body.String()) } }) + + t.Run("defaults container port from legacy networking", func(t *testing.T) { + createTestDeployment(t, tmpDir, "port-default", &models.ServiceMetadata{ + Name: "port-default", + Type: "web", + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "legacy.example.com", + ContainerPort: 9090, + }, + }) + + newDomain := models.DomainConfig{ + Domain: "new.example.com", + } + body, _ := json.Marshal(newDomain) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "name", Value: "port-default"}} + c.Request = httptest.NewRequest("POST", "/", bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + + server.addDomain(c) + + metadataPath := filepath.Join(tmpDir, "port-default", "service.yml") + data, err := os.ReadFile(metadataPath) + if err != nil { + t.Fatalf("Failed to read metadata: %v", err) + } + var metadata models.ServiceMetadata + if err := yaml.Unmarshal(data, &metadata); err != nil { + t.Fatalf("Failed to unmarshal metadata: %v", err) + } + + // Legacy domain migrated + new domain = 2 + if len(metadata.Domains) != 2 { + t.Fatalf("expected 2 domains, got %d", len(metadata.Domains)) + } + + newDomainEntry := metadata.Domains[1] + if newDomainEntry.ContainerPort != 9090 { + t.Errorf("expected new domain to inherit container port 9090, got %d", newDomainEntry.ContainerPort) + } + }) + + t.Run("preserves explicit container port", func(t *testing.T) { + createTestDeployment(t, tmpDir, "port-explicit", &models.ServiceMetadata{ + Name: "port-explicit", + Type: "web", + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "legacy.example.com", + ContainerPort: 9090, + }, + }) + + newDomain := models.DomainConfig{ + Domain: "explicit.example.com", + ContainerPort: 3000, + } + body, _ := json.Marshal(newDomain) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "name", Value: "port-explicit"}} + c.Request = httptest.NewRequest("POST", "/", bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + + server.addDomain(c) + + metadataPath := filepath.Join(tmpDir, "port-explicit", "service.yml") + data, err := os.ReadFile(metadataPath) + if err != nil { + t.Fatalf("Failed to read metadata: %v", err) + } + var metadata models.ServiceMetadata + if err := yaml.Unmarshal(data, &metadata); err != nil { + t.Fatalf("Failed to unmarshal metadata: %v", err) + } + + if len(metadata.Domains) != 2 { + t.Fatalf("expected 2 domains, got %d", len(metadata.Domains)) + } + + newDomainEntry := metadata.Domains[1] + if newDomainEntry.ContainerPort != 3000 { + t.Errorf("expected explicit port 3000 to be preserved, got %d", newDomainEntry.ContainerPort) + } + }) } diff --git a/internal/api/server.go b/internal/api/server.go index 1ad2501..a6b0862 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -3479,6 +3479,10 @@ func (s *Server) addDomain(c *gin.Context) { } } + if domain.ContainerPort == 0 && deployment.Metadata.Networking.ContainerPort != 0 { + domain.ContainerPort = deployment.Metadata.Networking.ContainerPort + } + deployment.Metadata.Domains = append(deployment.Metadata.Domains, domain) if err := s.manager.SaveMetadata(name, deployment.Metadata); err != nil { diff --git a/internal/nginx/manager_test.go b/internal/nginx/manager_test.go index f9228da..478d262 100644 --- a/internal/nginx/manager_test.go +++ b/internal/nginx/manager_test.go @@ -1799,6 +1799,227 @@ func TestIsNginxConfigValid(t *testing.T) { } } +func TestGenerateConfig_SSLToggle(t *testing.T) { + cfg := &config.NginxConfig{ + ContainerWebrootPath: "/var/www/html", + } + m := NewManager(cfg, "/deployments", "") + + deployment := &models.Deployment{ + Name: "ssl-toggle-test", + Metadata: &models.ServiceMetadata{ + Networking: models.NetworkingConfig{ + Expose: true, + Domain: "toggle.example.com", + ContainerPort: 8080, + }, + SSL: models.SSLConfig{Enabled: true}, + }, + } + + sslConfig, err := m.generateConfig(deployment) + if err != nil { + t.Fatalf("generateConfig with SSL failed: %v", err) + } + + if !strings.Contains(sslConfig, "listen 443 ssl;") { + t.Error("SSL-enabled config must contain 'listen 443 ssl;'") + } + if !strings.Contains(sslConfig, "ssl_certificate") { + t.Error("SSL-enabled config must contain ssl_certificate directive") + } + + deployment.Metadata.SSL.Enabled = false + httpConfig, err := m.generateConfig(deployment) + if err != nil { + t.Fatalf("generateConfig without SSL failed: %v", err) + } + + if strings.Contains(httpConfig, "listen 443") { + t.Error("HTTP-only config must not contain 'listen 443'") + } + if strings.Contains(httpConfig, "ssl_certificate") { + t.Error("HTTP-only config must not contain ssl_certificate directive") + } + if !strings.Contains(httpConfig, "listen 80;") { + t.Error("HTTP-only config must contain 'listen 80;'") + } +} + +func TestGenerateMultiDomainConfig_SSLToggle(t *testing.T) { + cfg := &config.NginxConfig{ + ContainerWebrootPath: "/var/www/html", + } + m := NewManager(cfg, "/deployments", "") + + t.Run("all domains SSL enabled", func(t *testing.T) { + deployment := &models.Deployment{ + Name: "multi-ssl", + Metadata: &models.ServiceMetadata{ + Domains: []models.DomainConfig{ + { + ID: "d1", + Service: "web", + ContainerPort: 8080, + Domain: "app.example.com", + SSL: models.SSLConfig{Enabled: true}, + }, + { + ID: "d2", + Service: "api", + ContainerPort: 3000, + Domain: "api.example.com", + SSL: models.SSLConfig{Enabled: true}, + }, + }, + }, + } + + config, err := m.generateMultiDomainConfig(deployment) + if err != nil { + t.Fatalf("generateMultiDomainConfig failed: %v", err) + } + + if !strings.Contains(config, "listen 443 ssl;") { + t.Error("all-SSL multi-domain config must contain 'listen 443 ssl;'") + } + if !strings.Contains(config, "ssl_certificate") { + t.Error("all-SSL multi-domain config must contain ssl_certificate directive") + } + }) + + t.Run("all domains SSL disabled", func(t *testing.T) { + deployment := &models.Deployment{ + Name: "multi-http", + Metadata: &models.ServiceMetadata{ + Domains: []models.DomainConfig{ + { + ID: "d1", + Service: "web", + ContainerPort: 8080, + Domain: "app.example.com", + SSL: models.SSLConfig{Enabled: false}, + }, + { + ID: "d2", + Service: "api", + ContainerPort: 3000, + Domain: "api.example.com", + SSL: models.SSLConfig{Enabled: false}, + }, + }, + }, + } + + config, err := m.generateMultiDomainConfig(deployment) + if err != nil { + t.Fatalf("generateMultiDomainConfig failed: %v", err) + } + + if strings.Contains(config, "listen 443") { + t.Error("HTTP-only multi-domain config must not contain 'listen 443'") + } + if strings.Contains(config, "ssl_certificate") { + t.Error("HTTP-only multi-domain config must not contain ssl_certificate") + } + }) + + t.Run("mixed SSL domains", func(t *testing.T) { + deployment := &models.Deployment{ + Name: "multi-mixed", + Metadata: &models.ServiceMetadata{ + Domains: []models.DomainConfig{ + { + ID: "d1", + Service: "web", + ContainerPort: 8080, + Domain: "secure.example.com", + SSL: models.SSLConfig{Enabled: true}, + }, + { + ID: "d2", + Service: "api", + ContainerPort: 3000, + Domain: "plain.example.com", + SSL: models.SSLConfig{Enabled: false}, + }, + }, + }, + } + + config, err := m.generateMultiDomainConfig(deployment) + if err != nil { + t.Fatalf("generateMultiDomainConfig failed: %v", err) + } + + if !strings.Contains(config, "listen 443 ssl;") { + t.Error("mixed multi-domain config must contain 'listen 443 ssl;' for the SSL domain") + } + if !strings.Contains(config, "secure.example.com") { + t.Error("mixed config must include secure.example.com") + } + if !strings.Contains(config, "plain.example.com") { + t.Error("mixed config must include plain.example.com") + } + }) +} + +func TestGenerateMultiDomainConfig_ContainerPort(t *testing.T) { + cfg := &config.NginxConfig{ + ContainerWebrootPath: "/var/www/html", + } + m := NewManager(cfg, "/deployments", "") + + t.Run("uses specified container port", func(t *testing.T) { + deployment := &models.Deployment{ + Name: "port-test", + Metadata: &models.ServiceMetadata{ + Domains: []models.DomainConfig{ + { + ID: "d1", + Service: "web", + ContainerPort: 9090, + Domain: "app.example.com", + }, + }, + }, + } + + config, err := m.generateMultiDomainConfig(deployment) + if err != nil { + t.Fatalf("generateMultiDomainConfig failed: %v", err) + } + + if !strings.Contains(config, "web:9090") { + t.Errorf("config should proxy to port 9090, got:\n%s", config) + } + }) + + t.Run("defaults to port 80 when container port is zero", func(t *testing.T) { + deployment := &models.Deployment{ + Name: "port-default-test", + Metadata: &models.ServiceMetadata{ + Domains: []models.DomainConfig{ + { + ID: "d1", + Service: "web", + Domain: "app.example.com", + }, + }, + }, + } + + config, err := m.generateMultiDomainConfig(deployment) + if err != nil { + t.Fatalf("generateMultiDomainConfig failed: %v", err) + } + + if !strings.Contains(config, "web:80") { + t.Errorf("config should default to port 80 when ContainerPort is 0, got:\n%s", config) + } + }) +} + func TestGroupDomainsByHost_DeduplicatesLocations(t *testing.T) { m := NewManager(&config.NginxConfig{}, "/deployments", "") diff --git a/internal/ssl/manager.go b/internal/ssl/manager.go index fa54f60..f4a8983 100644 --- a/internal/ssl/manager.go +++ b/internal/ssl/manager.go @@ -327,9 +327,12 @@ type MultiCertificateResult struct { } func (m *Manager) RequestCertificatesForDomains(domains []models.DomainConfig) (*MultiCertificateResult, error) { + // AutoCert is intentionally checked independently of SSL.Enabled: + // the orchestrator temporarily disables Enabled before certs exist, + // then re-enables it once obtained. domainSet := make(map[string]bool) for _, d := range domains { - if d.SSL.Enabled && d.SSL.AutoCert && d.Domain != "" { + if d.SSL.AutoCert && d.Domain != "" { domainSet[d.Domain] = true for _, alias := range d.Aliases { domainSet[alias] = true @@ -371,7 +374,7 @@ func (m *Manager) RequestCertificatesForDomains(domains []models.DomainConfig) ( func (m *Manager) GetDomainsNeedingCertificates(domains []models.DomainConfig) []string { var result []string for _, d := range domains { - if d.SSL.Enabled && d.SSL.AutoCert && d.Domain != "" { + if d.SSL.AutoCert && d.Domain != "" { if !m.CertificateExists(d.Domain) { result = append(result, d.Domain) } diff --git a/internal/ssl/manager_test.go b/internal/ssl/manager_test.go index 21f3b3a..35622b7 100644 --- a/internal/ssl/manager_test.go +++ b/internal/ssl/manager_test.go @@ -1,11 +1,13 @@ package ssl import ( + "os" "path/filepath" "strings" "testing" "github.com/flatrun/agent/pkg/config" + "github.com/flatrun/agent/pkg/models" ) func TestNewManager_WebrootPath(t *testing.T) { @@ -113,6 +115,67 @@ func TestContainerWebrootPath(t *testing.T) { } } +func TestGetDomainsNeedingCertificates_AutoCertWithDisabledSSL(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "ssl-cert-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.CertbotConfig{ + CertsPath: tmpDir, + } + m := NewManager(cfg, tmpDir) + + domains := []models.DomainConfig{ + { + Domain: "new.example.com", + SSL: models.SSLConfig{ + Enabled: false, + AutoCert: true, + }, + }, + } + + result := m.GetDomainsNeedingCertificates(domains) + + if len(result) != 1 { + t.Fatalf("expected 1 domain needing certificate, got %d", len(result)) + } + if result[0] != "new.example.com" { + t.Errorf("expected 'new.example.com', got '%s'", result[0]) + } +} + +func TestGetDomainsNeedingCertificates_SkipsWithoutAutoCert(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "ssl-cert-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + cfg := &config.CertbotConfig{ + CertsPath: tmpDir, + } + m := NewManager(cfg, tmpDir) + + domains := []models.DomainConfig{ + { + Domain: "manual.example.com", + SSL: models.SSLConfig{ + Enabled: true, + AutoCert: false, + }, + }, + } + + result := m.GetDomainsNeedingCertificates(domains) + + if len(result) != 0 { + t.Errorf("expected no domains needing certificates for AutoCert=false, got %d", len(result)) + } +} + func TestGetServiceExecConfig_WebrootVolume(t *testing.T) { tests := []struct { name string