diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 37d476b4..1baeff1b 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -98,7 +98,18 @@ func main() { glog.Infof("starting net-attach-def-admission-controller webhook server") - keyPair, err := webhook.NewTLSKeypairReloader(*cert, *key) + // Validate certificate and key file paths + validatedCertPath, err := validateFilePath(*cert) + if err != nil { + glog.Fatal("certificate file path validation failed") + } + + validatedKeyPath, err := validateFilePath(*key) + if err != nil { + glog.Fatal("private key file path validation failed") + } + + keyPair, err := webhook.NewTLSKeypairReloader(validatedCertPath, validatedKeyPath) if err != nil { glog.Fatalf("error load certificate: %s", err.Error()) } @@ -134,17 +145,13 @@ func main() { oldHashVal := "" for { hasher := sha512.New() - certPath, err := filepath.Abs(*cert) + // Use the already-validated certificate path + s, err := ioutil.ReadFile(validatedCertPath) if err != nil { - glog.Fatalf("illegal path %s in certPath: %s: %v", *cert, certPath, err) + glog.Fatalf("failed to read file %s: %v", validatedCertPath, err) os.Exit(1) } - s, err := ioutil.ReadFile(certPath) hasher.Write(s) - if err != nil { - glog.Fatalf("failed to read file %s: %v", *cert, err) - os.Exit(1) - } newHashVal := hex.EncodeToString(hasher.Sum(nil)) if oldHashVal != "" && newHashVal != oldHashVal { if err := proc.Signal(syscall.SIGHUP); err != nil { @@ -278,3 +285,44 @@ func startHTTPMetricServer(metricsAddress string, tlsConfig *tls.Config) *http.S return srv } + +// validateFilePath validates and cleans a file path to prevent path traversal attacks +func validateFilePath(path string) (string, error) { + if path == "" { + return "", fmt.Errorf("file path cannot be empty") + } + + // Reject parent-directory elements before normalization + // Split the path and check for ".." as a path element + pathElements := strings.Split(filepath.ToSlash(path), "/") + for _, element := range pathElements { + if element == ".." { + return "", fmt.Errorf("path contains parent directory reference") + } + } + + // Clean the path to remove any . or .. elements + cleanPath := filepath.Clean(path) + + // Convert to absolute path + absPath, err := filepath.Abs(cleanPath) + if err != nil { + return "", fmt.Errorf("failed to resolve absolute path: %w", err) + } + + // Evaluate symlinks to validate the target exists and is accessible + // This is only for validation - we return absPath to preserve symlinks + // for certificate rotation (Kubernetes AtomicWriter uses ..data symlinks) + _, err = filepath.EvalSymlinks(absPath) + if err != nil { + // If the file doesn't exist yet, that's acceptable + // The actual file read will fail later if it's truly missing + if !os.IsNotExist(err) { + return "", fmt.Errorf("failed to evaluate symlinks: %w", err) + } + } + + // Return the cleaned absolute path (not the resolved symlink target) + // This ensures certificate rotation works with Kubernetes projected volumes + return absPath, nil +} diff --git a/cmd/webhook/main_test.go b/cmd/webhook/main_test.go index 2043ef1d..56eb9a0c 100644 --- a/cmd/webhook/main_test.go +++ b/cmd/webhook/main_test.go @@ -27,6 +27,7 @@ import ( "net" "net/http" "os" + "path/filepath" "strconv" "testing" "time" @@ -39,6 +40,7 @@ import ( var ( _ = Describe("StringSliceFlag", testStringSliceFlag) _ = Describe("HTTP Servers", testHTTPServers) + _ = Describe("validateFilePath", testValidateFilePath) ) func TestMain(t *testing.T) { @@ -389,3 +391,71 @@ func generateTestCertificate() (string, string, error) { return certFile.Name(), keyFile.Name(), nil } + +func testValidateFilePath() { + Context("with valid file paths", func() { + It("should accept absolute paths", func() { + // Use a path that doesn't exist so it won't be resolved via symlinks + validated, err := validateFilePath("/opt/certs/cert.pem") + Expect(err).NotTo(HaveOccurred()) + Expect(validated).To(Equal("/opt/certs/cert.pem")) + }) + + It("should accept relative paths and convert to absolute", func() { + validated, err := validateFilePath("./cert.pem") + Expect(err).NotTo(HaveOccurred()) + Expect(validated).To(HavePrefix("/")) + Expect(validated).To(HaveSuffix("/cert.pem")) + }) + + It("should preserve symlinks for certificate rotation", func() { + // Create a temporary directory structure mimicking Kubernetes projected volumes + tmpDir, err := os.MkdirTemp("", "cert-test-*") + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(tmpDir) + + // Create a ..data directory (Kubernetes AtomicWriter pattern) + dataDir := filepath.Join(tmpDir, "..data") + err = os.Mkdir(dataDir, 0755) + Expect(err).NotTo(HaveOccurred()) + + // Create the actual certificate file in the ..data directory + certPath := filepath.Join(dataDir, "tls.crt") + err = os.WriteFile(certPath, []byte("fake cert"), 0644) + Expect(err).NotTo(HaveOccurred()) + + // Create a symlink pointing to the file in ..data + symlinkPath := filepath.Join(tmpDir, "tls.crt") + err = os.Symlink(filepath.Join("..data", "tls.crt"), symlinkPath) + Expect(err).NotTo(HaveOccurred()) + + // Validate the symlink path + validated, err := validateFilePath(symlinkPath) + Expect(err).NotTo(HaveOccurred()) + + // The validated path should be the symlink itself, not the resolved target + // This ensures certificate rotation continues to work + Expect(validated).To(Equal(symlinkPath)) + Expect(validated).NotTo(ContainSubstring("..data")) + + // Verify the symlink can still be read + content, err := os.ReadFile(validated) + Expect(err).NotTo(HaveOccurred()) + Expect(string(content)).To(Equal("fake cert")) + }) + + It("should reject paths with parent directory references", func() { + _, err := validateFilePath("/tmp/../tmp/cert.pem") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("parent directory reference")) + }) + }) + + Context("with invalid inputs", func() { + It("should reject empty paths", func() { + _, err := validateFilePath("") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("cannot be empty")) + }) + }) +}