Merge pull request #508 from neowulf/siva/0824-cert-perms

tcp_router: fix cert directory permissions

Author: ameowlia

jobs/tcp_router/templates/pre-start.erb

@@ -1,5 +1,4 @@
 #!/bin/bash -e
 
-<% if spec.bootstrap == true %>
+# This pre-start script runs (1) config validation and (2) creates frontend certs on the VM (if any)
 /var/vcap/packages/tcp_router/bin/config-validator -config /var/vcap/jobs/tcp_router/config/tcp_router.yml -enable-cert-creation true
-<% end %>

src/code.cloudfoundry.org/cf-tcp-router/config/config.go

@@ -102,6 +102,12 @@ func resolveGroupID(primary, fallback string) (int, error) {
 	return gid, nil
 }
 
+// initConfigFromFile initializes the config and write the certs to the filesystem when enableCertCreation is true.
+//
+// enableCertCreation is set to true during pre-start on all VMs. During pre-start initConfigFromFile is run
+// to (1) validate the config provided and (2) create the FrontendTLSJob certs on the filesystem if provided.
+//
+// enableCertCreation is set to false when bosh starts the tcp-router job.
 func (c *Config) initConfigFromFile(path string, enableCertCreation bool) error {
 	b, err := os.ReadFile(path)
 	if err != nil {
@@ -121,70 +127,56 @@ func (c *Config) initConfigFromFile(path string, enableCertCreation bool) error
 	}
 
 	if enableCertCreation && len(c.FrontendTLSJob) > 0 {
-		owner, err := user.Lookup("root")
-		if err != nil {
-			return err
+		// remove existing directory which will clean out old certs if necessary
+		if err := os.RemoveAll(c.FrontendTLSJobBasePath()); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("directory could not be removed: %w", err)
 		}
 
-		uid, err := strconv.Atoi(owner.Uid)
-		if err != nil {
-			return err
+		if err := c.createDir(c.FrontendTLSJobBasePath()); err != nil {
+			return fmt.Errorf("could not create directory: %w", err)
 		}
+	}
 
-		gid, err := resolveGroupID("vcap", "root")
-		if err != nil {
-			return err
+	for i, cert := range c.FrontendTLSJob {
+		// validations
+		name := strings.TrimSpace(cert.Name)
+		if name == "" {
+			return fmt.Errorf("frontend_tls[%d]: empty name", i)
 		}
-		var outputs []FrontendTLSConfig
-		basePath := c.FrontendTLSJobBasePath()
-		for i, cert := range c.FrontendTLSJob {
 
-			name := strings.TrimSpace(cert.Name)
-			certChain := strings.TrimSpace(cert.CertChain)
-			privateKey := strings.TrimSpace(cert.PrivateKey)
-
-			if name == "" || certChain == "" || privateKey == "" {
-				return fmt.Errorf("frontend_tls[%d] must include name, cert_chain, and private_key", i)
-			}
-
-			block, _ := pem.Decode([]byte(certChain))
-			if block == nil {
-				return errors.New("failed to parse PEM block")
-			}
-			cert, err := x509.ParseCertificate(block.Bytes)
-			if err != nil {
-				return err
-			}
+		certChain := strings.TrimSpace(cert.CertChain)
+		if certChain == "" {
+			return fmt.Errorf("frontend_tls[%d]: empty cert_chain", i)
+		}
 
-			hasSAN := certHasSAN(cert)
-			if !hasSAN {
-				return fmt.Errorf("frontend_tls[%d].cert_chain must include a subjectAltName extension", i)
-			}
+		privateKey := strings.TrimSpace(cert.PrivateKey)
+		if privateKey == "" {
+			return fmt.Errorf("frontend_tls[%d]: empty private_key", i)
+		}
 
-			dirPath := filepath.Join(basePath, name)
-			if err := os.MkdirAll(dirPath, 0750); err != nil {
-				return err
-			}
+		// check if san is present and that the cert chain is valid
+		if err := certHasSAN(cert.CertChain); err != nil {
+			return fmt.Errorf("frontend_tls[%d]: %w", i, err)
+		}
 
-			certFilePath := filepath.Join(dirPath, fmt.Sprintf("%s.pem", name))
-			keyFilePath := filepath.Join(dirPath, fmt.Sprintf("%s.pem.key", name))
+		dirPath := filepath.Join(c.FrontendTLSJobBasePath(), name)
 
-			if err := writeFile(certFilePath, []byte(certChain), 0750, uid, gid); err != nil {
-				return err
+		// write certs to the disk
+		if enableCertCreation {
+			if err := c.createDir(dirPath); err != nil {
+				return fmt.Errorf("frontend_tls[%d]: %w", i, err)
 			}
 
-			if err := writeFile(keyFilePath, []byte(privateKey), 0750, uid, gid); err != nil {
-				return err
+			if err := c.writeCertsToDisk(dirPath, name, cert, privateKey); err != nil {
+				return fmt.Errorf("frontend_tls[%d]: %w", i, err)
 			}
-
-			outputs = append(outputs, FrontendTLSConfig{
-				Enabled:        true,
-				CertificateDir: dirPath,
-			})
 		}
 
-		c.FrontendTLS = outputs
-
+		// update the config
+		c.FrontendTLS = append(c.FrontendTLS, FrontendTLSConfig{
+			Enabled:        true,
+			CertificateDir: dirPath,
+		})
 	}
 
 	if c.BackendTLS.Enabled {
@@ -262,28 +254,86 @@ func (c *Config) initConfigFromFile(path string, enableCertCreation bool) error
 	return nil
 }
 
-func certHasSAN(cert *x509.Certificate) bool {
-	hasSANExtension := false
-	for _, ext := range cert.Extensions {
-		if ext.Id.String() == "2.5.29.17" {
-			hasSANExtension = true
-			break
-		}
+func (c *Config) createDir(dirPath string) error {
+	// ensure directory exists
+	if err := os.MkdirAll(dirPath, 0750); err != nil {
+		return err
+	}
+
+	targetUID, targetGID, err := c.getUIDGID()
+	if err != nil {
+		return err
 	}
 
-	hasDNSEntries := len(cert.DNSNames) > 0
+	// Change ownership
+	err = os.Chown(dirPath, targetUID, targetGID)
+	if err != nil {
+		return fmt.Errorf("Error changing ownership: %s", err)
+	}
 
-	return hasSANExtension || hasDNSEntries
+	return nil
 }
 
-func writeFile(path string, data []byte, mode os.FileMode, uid, gid int) error {
-	if err := os.WriteFile(path, data, mode); err != nil {
+func (c *Config) writeCertsToDisk(dirPath string, name string, cert FrontendTLSJob, privateKey string) error {
+	uid, gid, err := c.getUIDGID()
+	if err != nil {
 		return err
 	}
 
-	if err := os.Chown(path, uid, gid); err != nil {
+	certFilePath := filepath.Join(dirPath, fmt.Sprintf("%s.pem", name))
+	keyFilePath := filepath.Join(dirPath, fmt.Sprintf("%s.pem.key", name))
+
+	if err := writeFile(certFilePath, []byte(cert.CertChain), 0750, uid, gid); err != nil {
 		return err
 	}
 
+	if err := writeFile(keyFilePath, []byte(privateKey), 0750, uid, gid); err != nil {
+		return err
+	}
 	return nil
 }
+
+func (c *Config) getUIDGID() (int, int, error) {
+	owner, err := user.Lookup("root")
+	if err != nil {
+		return 0, 0, err
+	}
+
+	uid, err := strconv.Atoi(owner.Uid)
+	if err != nil {
+		return 0, 0, err
+	}
+
+	gid, err := resolveGroupID("vcap", "root")
+	if err != nil {
+		return 0, 0, err
+	}
+
+	return uid, gid, nil
+}
+
+func certHasSAN(certChain string) error {
+	block, _ := pem.Decode([]byte(certChain))
+	if block == nil {
+		return errors.New("failed to parse PEM block")
+	}
+
+	cert, err := x509.ParseCertificate(block.Bytes)
+	if err != nil {
+		return fmt.Errorf("could not parse certificate: %w", err)
+	}
+
+	if len(cert.DNSNames) > 0 {
+		return nil
+	}
+
+	return fmt.Errorf("cert_chain must include either a subjectAltName extension or DNSNames")
+}
+
+func writeFile(path string, data []byte, mode os.FileMode, uid, gid int) error {
+	if err := os.WriteFile(path, data, mode); err != nil {
+		return err
+	}
+
+	return os.Chown(path, uid, gid)
+}

src/code.cloudfoundry.org/cf-tcp-router/config/config_test.go

@@ -2,7 +2,9 @@ package config_test
 
 import (
 	"fmt"
+	"math/rand/v2"
 	"os"
+	"path"
 	"path/filepath"
 	"time"
 
@@ -221,15 +223,15 @@ var _ = Describe("Config", Serial, func() {
 		)
 
 		BeforeEach(func() {
-			tmpDir = GinkgoT().TempDir()
+			tmpDir = path.Join(GinkgoT().TempDir(), fmt.Sprintf("frontend-%d", rand.IntN(100)))
 			os.Setenv("FRONTEND_TLS_BASE_PATH", tmpDir)
 		})
 
 		AfterEach(func() {
 			os.Unsetenv("FRONTEND_TLS_BASE_PATH")
 		})
 
-		Context("with valid cert and key", func() {
+		Context("with valid cert and key and enableCertCreation set to true", func() {
 			BeforeEach(func() {
 				cfg, err = config.New("fixtures/valid_frontend_cert.yml", true)
 			})
@@ -238,7 +240,7 @@ var _ = Describe("Config", Serial, func() {
 				Expect(err).NotTo(HaveOccurred())
 			})
 
-			It("adds the certs and keys to the expected directories", func() {
+			It("updates the config with the expected directories", func() {
 				Expect(err).NotTo(HaveOccurred())
 				Expect(cfg.FrontendTLS).To(HaveLen(2))
 
@@ -253,14 +255,26 @@ var _ = Describe("Config", Serial, func() {
 				}))
 			})
 
-			It("writes the correct cert and key files", func() {
+			It("writes the cert and key files", func() {
+				Expect(tmpDir).To(BeADirectory())
+
 				for i, name := range []string{"prod", "dev"} {
 					certPath := filepath.Join(tmpDir, name, name+".pem")
 					keyPath := filepath.Join(tmpDir, name, name+".pem.key")
 
+					Expect(path.Join(tmpDir, name)).To(BeADirectory())
+
 					Expect(certPath).To(BeAnExistingFile())
 					Expect(keyPath).To(BeAnExistingFile())
 
+					certInfo, err := os.Stat(certPath)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(os.FileMode(0750)).To(Equal(certInfo.Mode().Perm()))
+
+					keyInfo, err := os.Stat(keyPath)
+					Expect(err).NotTo(HaveOccurred())
+					Expect(os.FileMode(0750)).To(Equal(keyInfo.Mode().Perm()))
+
 					certData, certErr := os.ReadFile(certPath)
 					Expect(certErr).NotTo(HaveOccurred())
 					Expect(string(certData)).To(Equal(cfg.FrontendTLSJob[i].CertChain))
@@ -272,16 +286,16 @@ var _ = Describe("Config", Serial, func() {
 			})
 		})
 
-		Context("with invalid cert and key", func() {
+		Context("with valid cert (having san and dnsnames) and key and enableCertCreation set to false", func() {
 			BeforeEach(func() {
-				cfg, err = config.New("fixtures/valid_frontend_cert.yml", true)
+				cfg, err = config.New("fixtures/valid_frontend_cert.yml", false)
 			})
 
 			It("loads config without error", func() {
 				Expect(err).NotTo(HaveOccurred())
 			})
 
-			It("adds the certs and keys to the expected directories", func() {
+			It("updates the config with the expected directories", func() {
 				Expect(err).NotTo(HaveOccurred())
 				Expect(cfg.FrontendTLS).To(HaveLen(2))
 
@@ -296,51 +310,53 @@ var _ = Describe("Config", Serial, func() {
 				}))
 			})
 
-			It("writes the correct cert and key files with correct permissions", func() {
-				for i, name := range []string{"prod", "dev"} {
+			It("does not write the cert and key files as enableCertCreation is false", func() {
+				for _, name := range []string{"prod", "dev"} {
 					certPath := filepath.Join(tmpDir, name, name+".pem")
 					keyPath := filepath.Join(tmpDir, name, name+".pem.key")
 
-					Expect(certPath).To(BeAnExistingFile())
-					Expect(keyPath).To(BeAnExistingFile())
-
-					certData, certErr := os.ReadFile(certPath)
-					Expect(certErr).NotTo(HaveOccurred())
-					Expect(string(certData)).To(Equal(cfg.FrontendTLSJob[i].CertChain))
-
-					keyData, keyErr := os.ReadFile(keyPath)
-					Expect(keyErr).NotTo(HaveOccurred())
-					Expect(string(keyData)).To(Equal(cfg.FrontendTLSJob[i].PrivateKey))
-
-					certInfo, err := os.Stat(certPath)
-					Expect(err).NotTo(HaveOccurred())
-					Expect(os.FileMode(0750)).To(Equal(certInfo.Mode().Perm()))
-
-					keyInfo, err := os.Stat(keyPath)
-					Expect(err).NotTo(HaveOccurred())
-					Expect(os.FileMode(0750)).To(Equal(keyInfo.Mode().Perm()))
+					Expect(certPath).ToNot(BeAnExistingFile())
+					Expect(keyPath).ToNot(BeAnExistingFile())
 				}
 			})
 		})
 
 		Context("with invalid frontend_tls config", func() {
-			It("should fail if cert_chain is missing SAN information", func() {
-				_, err := config.New("fixtures/frontend_cert_without_san.yml", true)
+			It("should fail if cert is empty", func() {
+				_, err := config.New("fixtures/no_frontend_certs.yml", false)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal("frontend_tls[0]: empty cert_chain"))
+			})
+
+			It("should fail if key is missing", func() {
+				_, err := config.New("fixtures/invalid_frontend_key.yml", false)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("frontend_tls[0].cert_chain must include a subjectAltName extension"))
+				Expect(err.Error()).To(Equal("frontend_tls[0]: empty private_key"))
 			})
-			It("should fail if certs or keys are empty", func() {
-				_, err := config.New("fixtures/no_frontend_certs.yml", true)
+
+			It("should fail if name is missing", func() {
+				_, err := config.New("fixtures/invalid_frontend_name.yml", false)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("frontend_tls[0] must include name, cert_chain, and private_key"))
+				Expect(err.Error()).To(Equal("frontend_tls[0]: empty name"))
 			})
+
 			It("should fail if cert is invalid", func() {
-				_, err := config.New("fixtures/invalid_frontend_certs.yml", true)
+				_, err := config.New("fixtures/invalid_frontend_certs.yml", false)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal("frontend_tls[0]: failed to parse PEM block"))
+			})
+
+			It("should fail if cert_chain is missing SAN information and DNSnames", func() {
+				_, err := config.New("fixtures/frontend_cert_without_san.yml", false)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("failed to parse PEM block"))
+				Expect(err.Error()).To(Equal("frontend_tls[0]: cert_chain must include either a subjectAltName extension or DNSNames"))
 			})
 
+			It("should fail if cert_chain is invalid", func() {
+				_, err := config.New("fixtures/invalid_frontend_cert_certchain.yml", false)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal("frontend_tls[0]: could not parse certificate: x509: malformed certificate"))
+			})
 		})
 	})
-
 })