NO-JIRA: Fix path traversal vulnerability in certificate file handling

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
+}

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"))
+		})
+	})
+}