diff --git a/cmd/agent/deploy.go b/cmd/agent/deploy.go index 9bc679c..6a062b3 100644 --- a/cmd/agent/deploy.go +++ b/cmd/agent/deploy.go @@ -11,7 +11,6 @@ import ( "io" "net/http" "os" - "path/filepath" "strings" "github.com/certctl-io/certctl/internal/connector/target" @@ -105,8 +104,24 @@ func (a *Agent) executeDeploymentJob(ctx context.Context, job JobItem) { // Split PEM into cert and chain (separated by double newline between PEM blocks) certOnly, chainPEM := splitPEMChain(certPEM) - // Check for locally-stored private key (agent keygen mode) - keyPath := filepath.Join(a.config.KeyDir, job.CertificateID+".key") + // Check for locally-stored private key (agent keygen mode). + // + // SEC-002 closure (Sprint 1, 2026-05-16): safeAgentKeyPath validates + // the certificate_id shape AND asserts the joined path is contained + // within a.config.KeyDir. A crafted certificate_id (path traversal, + // absolute path, NUL byte, Windows separators) fails closed before + // any disk I/O. See cmd/agent/keymem.go for the helper. + keyPath, kerr := safeAgentKeyPath(a.config.KeyDir, job.CertificateID) + if kerr != nil { + a.logger.Error("agent key path validation failed for deployment", + "job_id", job.ID, + "certificate_id", job.CertificateID, + "error", kerr) + if reportErr := a.reportJobStatus(ctx, job.ID, "Failed", fmt.Sprintf("key path validation failed: %v", kerr)); reportErr != nil { + a.logger.Error("failed to report job status to server", "job_id", job.ID, "error", reportErr) + } + return + } var keyPEM string keyData, err := os.ReadFile(keyPath) if err != nil { diff --git a/cmd/agent/keymem.go b/cmd/agent/keymem.go index ec99e99..2d2906b 100644 --- a/cmd/agent/keymem.go +++ b/cmd/agent/keymem.go @@ -9,6 +9,8 @@ import ( "fmt" "os" "path/filepath" + "regexp" + "strings" ) // Bundle-9 / Audit L-002 + L-003 (agent edition). @@ -41,6 +43,87 @@ func marshalAgentKeyAndZeroize(priv *ecdsa.PrivateKey, onDER func([]byte) error) return onDER(der) } +// SEC-002 closure (Sprint 1, 2026-05-16). The agent derives an on-disk +// key path from job.CertificateID via filepath.Join. Pre-fix, a +// crafted certificate_id ("../../etc/passwd", "/absolute/path", +// "abc\x00d", "..\\Windows\\path") would drive arbitrary file +// write/read on the agent host. The shape regex below mirrors the +// server-side internal/validation.ValidateCertificateID gate — both +// ends MUST hold for the load-bearing defense (the server can't be +// trusted in isolation; a compromised control plane could deliver a +// crafted job). +// +// agentCertIDPattern accepts ASCII letters, digits, ".", "_", "-", +// bounded to 128 chars. Existing prefixed IDs (mc-..., cert-..., etc.) +// satisfy this trivially. Deliberately rejects path separators (POSIX +// and Windows), NUL byte, whitespace, control characters, and the +// bare relative-path tokens "." and "..". +var agentCertIDPattern = regexp.MustCompile(`^[A-Za-z0-9._-]{1,128}$`) + +// validateAgentCertID returns an error if id is not a well-formed +// certificate identifier. Mirrors internal/validation.ValidateCertificateID +// — the duplication is deliberate per the package-level comment +// ("cmd/agent is a separate binary; copy-paste cheaper than lifting +// a shared internal/keystore for a single shape check"). +func validateAgentCertID(id string) error { + if id == "" { + return fmt.Errorf("certificate_id is required") + } + if len(id) > 128 { + return fmt.Errorf("certificate_id length %d exceeds 128", len(id)) + } + if !agentCertIDPattern.MatchString(id) { + return fmt.Errorf("certificate_id %q contains disallowed characters", id) + } + if id == "." || id == ".." { + return fmt.Errorf("certificate_id %q is a relative-path token", id) + } + return nil +} + +// safeAgentKeyPath returns the on-disk key path for the given +// certificateID, after validating the ID shape AND asserting the +// joined path is contained within keyDir. Containment is the +// authoritative guard — even if validateAgentCertID is bypassed (e.g. +// a future refactor removes it), the post-Clean rel-path check below +// rejects any path that escapes keyDir. +// +// The two-leg defense: +// +// leg 1: shape check (validateAgentCertID) → cheap up-front fail +// leg 2: containment check (filepath.Rel) → load-bearing guard +// +// Returns the joined path on success, or a non-nil error describing +// the rejected vector. +func safeAgentKeyPath(keyDir, certificateID string) (string, error) { + if err := validateAgentCertID(certificateID); err != nil { + return "", err + } + if keyDir == "" { + return "", fmt.Errorf("safeAgentKeyPath: empty keyDir") + } + cleanDir, err := filepath.Abs(filepath.Clean(keyDir)) + if err != nil { + return "", fmt.Errorf("safeAgentKeyPath: resolve keyDir: %w", err) + } + joined := filepath.Join(cleanDir, certificateID+".key") + cleanJoined := filepath.Clean(joined) + rel, err := filepath.Rel(cleanDir, cleanJoined) + if err != nil { + return "", fmt.Errorf("safeAgentKeyPath: rel(%q,%q): %w", cleanDir, cleanJoined, err) + } + // Reject any path that escapes the directory: a leading ".." in the + // relative form means the joined path resolved outside keyDir. + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("safeAgentKeyPath: %q escapes keyDir %q (rel=%q)", certificateID, cleanDir, rel) + } + // Belt-and-suspenders: the rel form must also not contain a NUL. + if strings.ContainsRune(rel, 0) { + return "", fmt.Errorf("safeAgentKeyPath: NUL byte in computed path") + } + return cleanJoined, nil +} + // ensureAgentKeyDirSecure creates dir (and ancestors) with mode 0700 or // asserts an existing dir is owner-only. If a pre-existing dir is more // permissive than 0700 we tighten it to 0700 (logging-free; this is a diff --git a/cmd/agent/keymem_test.go b/cmd/agent/keymem_test.go index f1e1424..a073052 100644 --- a/cmd/agent/keymem_test.go +++ b/cmd/agent/keymem_test.go @@ -716,3 +716,113 @@ func TestKeymem_AgentMainFlowSmoke(t *testing.T) { } } } + +// ============================================================================= +// SEC-002 closure (Sprint 1, 2026-05-16) — safeAgentKeyPath path-traversal +// regression coverage. +// +// Pre-fix the agent built the on-disk key path via: +// +// keyPath := filepath.Join(a.config.KeyDir, job.CertificateID+".key") +// +// migrations/000001_initial_schema.up.sql declares +// managed_certificates.id as TEXT PRIMARY KEY with no shape constraint, so +// a crafted certificate_id from a compromised control plane (or a poisoned +// DB row) could land outside KeyDir. The fix: +// +// - validateAgentCertID rejects shape violations up-front +// - safeAgentKeyPath additionally asserts the joined path is contained +// within KeyDir via filepath.Rel; even a future refactor that drops +// the shape regex would still fail closed on escape. +// +// These tests pin both legs against the four vectors called out in the +// audit (../../etc/passwd, /absolute/path, NUL byte, Windows separators). +// ============================================================================= + +func TestValidateAgentCertID_AcceptsCanonicalShapes(t *testing.T) { + for _, id := range []string{ + "mc-cdn-edge", + "mc-cdn-edge-2026.q1", + "cert-1", + "abc123", + "MC-UPPER", + } { + t.Run(id, func(t *testing.T) { + if err := validateAgentCertID(id); err != nil { + t.Errorf("validateAgentCertID(%q): unexpected error %v", id, err) + } + }) + } +} + +func TestValidateAgentCertID_RejectsTraversalVectors(t *testing.T) { + cases := []struct { + name string + id string + }{ + {"empty", ""}, + {"parent_token", ".."}, + {"current_token", "."}, + {"posix_traversal", "../../etc/passwd"}, + {"absolute_posix", "/absolute/path"}, + {"windows_traversal", `..\..\evil`}, + {"windows_separator", `bad\path`}, + {"nul_byte", "abc\x00def"}, + {"newline", "abc\ndef"}, + {"space", "id with spaces"}, + {"overlong", strings.Repeat("a", 129)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := validateAgentCertID(tc.id); err == nil { + t.Errorf("id=%q: expected rejection, got nil", tc.id) + } + }) + } +} + +func TestSafeAgentKeyPath_HappyPath_ProducesContainedPath(t *testing.T) { + keyDir := t.TempDir() + got, err := safeAgentKeyPath(keyDir, "mc-good") + if err != nil { + t.Fatalf("safeAgentKeyPath: %v", err) + } + want := filepath.Join(keyDir, "mc-good.key") + // filepath.Clean normalisation may strip a trailing separator, etc.; + // compare canonical forms. + if filepath.Clean(got) != filepath.Clean(want) { + t.Errorf("safeAgentKeyPath = %q; want %q", got, want) + } +} + +func TestSafeAgentKeyPath_RejectsTraversalVectors(t *testing.T) { + keyDir := t.TempDir() + cases := []struct { + name string + id string + }{ + {"posix_traversal", "../../etc/passwd"}, + {"absolute_posix", "/etc/passwd"}, + {"parent_token", ".."}, + {"current_token", "."}, + {"windows_traversal", `..\..\evil`}, + {"windows_separator", `bad\path`}, + {"nul_byte", "abc\x00def"}, + {"empty", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := safeAgentKeyPath(keyDir, tc.id) + if err == nil { + t.Errorf("id=%q: expected rejection, got nil", tc.id) + } + }) + } +} + +func TestSafeAgentKeyPath_RejectsEmptyKeyDir(t *testing.T) { + _, err := safeAgentKeyPath("", "mc-good") + if err == nil { + t.Errorf("empty keyDir: expected rejection, got nil") + } +} diff --git a/cmd/agent/poll.go b/cmd/agent/poll.go index dcfdfab..52852a6 100644 --- a/cmd/agent/poll.go +++ b/cmd/agent/poll.go @@ -151,7 +151,20 @@ func (a *Agent) executeCSRJob(ctx context.Context, job JobItem) { // before any write touches disk. Also defer-clear the PEM buffer for // the same reason — the encoded key isn't sensitive in transit (it's // going to disk) but lingers on the heap if we don't. - keyPath := filepath.Join(a.config.KeyDir, job.CertificateID+".key") + // + // SEC-002 closure (Sprint 1, 2026-05-16): safeAgentKeyPath validates + // the certificate_id shape AND asserts the joined path is contained + // within a.config.KeyDir. A crafted certificate_id like + // "../../etc/passwd" or "/abs/path" now fails closed before any + // disk I/O. See cmd/agent/keymem.go for the helper. + keyPath, kerr := safeAgentKeyPath(a.config.KeyDir, job.CertificateID) + if kerr != nil { + a.logger.Error("agent key path validation failed", "job_id", job.ID, "certificate_id", job.CertificateID, "error", kerr) + if reportErr := a.reportJobStatus(ctx, job.ID, "Failed", fmt.Sprintf("key path validation failed: %v", kerr)); reportErr != nil { + a.logger.Error("failed to report job status to server", "job_id", job.ID, "status", "Failed", "error", reportErr) + } + return + } if err := ensureAgentKeyDirSecure(filepath.Dir(keyPath)); err != nil { a.logger.Error("agent key dir hardening failed", "job_id", job.ID, "error", err) if reportErr := a.reportJobStatus(ctx, job.ID, "Failed", fmt.Sprintf("key dir hardening failed: %v", err)); reportErr != nil { diff --git a/internal/service/certificate.go b/internal/service/certificate.go index bc8c39b..6ec14e4 100644 --- a/internal/service/certificate.go +++ b/internal/service/certificate.go @@ -11,6 +11,7 @@ import ( "github.com/certctl-io/certctl/internal/domain" "github.com/certctl-io/certctl/internal/repository" + "github.com/certctl-io/certctl/internal/validation" ) // CertificateService provides business logic for certificate management. @@ -163,6 +164,15 @@ func (s *CertificateService) Create(ctx context.Context, cert *domain.ManagedCer if cert.ID == "" || cert.CommonName == "" || cert.IssuerID == "" { return fmt.Errorf("invalid certificate: missing required fields") } + // SEC-002 closure (Sprint 1, 2026-05-16): pin the certificate_id + // shape at the server boundary. The agent derives an on-disk key + // path from this ID via filepath.Join; without this gate a + // crafted ID like "../../etc/passwd" or "/absolute/path" would + // drive arbitrary file write/read on the agent host. Companion + // containment check lives in cmd/agent/keymem.go (safeAgentKeyPath). + if err := validation.ValidateCertificateID(cert.ID); err != nil { + return fmt.Errorf("invalid certificate id: %w", err) + } // Run policy validation violations, err := s.policyService.ValidateCertificate(ctx, cert) diff --git a/internal/validation/certificate_id.go b/internal/validation/certificate_id.go new file mode 100644 index 0000000..e3e6b54 --- /dev/null +++ b/internal/validation/certificate_id.go @@ -0,0 +1,67 @@ +// Copyright 2026 certctl LLC. All rights reserved. +// SPDX-License-Identifier: BUSL-1.1 + +package validation + +// SEC-002 closure (Sprint 1, 2026-05-16). The agent derives an on-disk +// key path from `job.CertificateID` via filepath.Join: +// +// keyPath := filepath.Join(a.config.KeyDir, job.CertificateID+".key") +// +// migrations/000001_initial_schema.up.sql declares managed_certificates.id +// as TEXT PRIMARY KEY with no shape constraint, so a compromised control +// plane (or a crafted row in the database) could deliver a job whose +// certificate_id is "../../etc/passwd", "/absolute/path", a NUL-byte +// payload, or a Windows-separator-laden string — driving arbitrary +// file write/read on the agent host. +// +// ValidateCertificateID is the server-side shape gate. It pins the +// canonical TEXT-PK prefix convention used across certctl (lowercase +// alphanumeric + `_-`, bounded length) and rejects everything else +// before the row reaches the database or a downstream agent. The +// agent host owns a symmetric containment check via safeAgentKeyPath +// in cmd/agent/keymem.go — both ends MUST hold for the load-bearing +// defense. + +import ( + "fmt" + "regexp" +) + +// certificateIDPattern is the canonical shape for managed_certificates.id. +// Permits ASCII letters, digits, underscore, hyphen, and dot (so existing +// rows like "mc-cdn-edge-2026.q1" continue to validate). Length capped at +// 128 — well beyond any human-readable identifier and short enough that +// a path built from it stays within typical filesystem path limits. +// +// Deliberately rejects: +// - "/" and "\\" (path separators on POSIX + Windows) +// - ".." (relative-path escape token) +// - "\x00" (NUL byte truncates the path on many syscalls) +// - whitespace / control characters +// - the empty string +// +// Existing prefixed IDs in production (`mc-…`, `t-…`, `o-…`, etc.) all +// satisfy this pattern. +var certificateIDPattern = regexp.MustCompile(`^[A-Za-z0-9._-]{1,128}$`) + +// ValidateCertificateID returns an error if id is not a well-formed +// certificate identifier. Callers MUST run this before passing the id +// to any filesystem-touching code path. +func ValidateCertificateID(id string) error { + if id == "" { + return fmt.Errorf("certificate_id is required") + } + if len(id) > 128 { + return fmt.Errorf("certificate_id length %d exceeds 128", len(id)) + } + if !certificateIDPattern.MatchString(id) { + return fmt.Errorf("certificate_id %q contains disallowed characters; allowed: A-Z a-z 0-9 . _ -", id) + } + // Defense-in-depth: even within the allowed set, ".." would slip + // through the regex. Reject it explicitly. + if id == ".." || id == "." { + return fmt.Errorf("certificate_id %q is a relative-path token", id) + } + return nil +} diff --git a/internal/validation/certificate_id_test.go b/internal/validation/certificate_id_test.go new file mode 100644 index 0000000..a236f0a --- /dev/null +++ b/internal/validation/certificate_id_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 certctl LLC. All rights reserved. +// SPDX-License-Identifier: BUSL-1.1 + +package validation + +import ( + "strings" + "testing" +) + +// SEC-002 closure (Sprint 1, 2026-05-16). Pin the server-side +// certificate_id shape gate. Companion to the agent-side +// safeAgentKeyPath containment check in cmd/agent/keymem.go. + +func TestValidateCertificateID_AcceptsProductionShapes(t *testing.T) { + cases := []string{ + "mc-cdn-edge", + "mc-cdn-edge-2026.q1", + "mc_internal_api", + "abc123", + "MC-UPPER-CASE", + strings.Repeat("a", 128), // exact-length boundary + } + for _, id := range cases { + t.Run(id, func(t *testing.T) { + if err := ValidateCertificateID(id); err != nil { + t.Errorf("ValidateCertificateID(%q): unexpected error %v", id, err) + } + }) + } +} + +func TestValidateCertificateID_RejectsEmpty(t *testing.T) { + if err := ValidateCertificateID(""); err == nil { + t.Errorf("empty id: expected rejection, got nil") + } +} + +func TestValidateCertificateID_RejectsOverlong(t *testing.T) { + id := strings.Repeat("a", 129) + err := ValidateCertificateID(id) + if err == nil { + t.Fatalf("overlong id: expected rejection, got nil") + } + if !strings.Contains(err.Error(), "exceeds 128") { + t.Errorf("expected length error, got %v", err) + } +} + +// TestValidateCertificateID_RejectsPathTraversalVectors pins the four +// vectors called out in SEC-002 (../../etc/passwd, /absolute/path, +// NUL-byte, Windows separators) plus the bare ".." token. +func TestValidateCertificateID_RejectsPathTraversalVectors(t *testing.T) { + cases := []struct { + name string + id string + }{ + {"posix_traversal", "../../etc/passwd"}, + {"absolute_posix", "/absolute/path"}, + {"absolute_root", "/"}, + {"parent_token", ".."}, + {"current_token", "."}, + {"windows_traversal", `..\..\evil`}, + {"windows_separator", `bad\path`}, + {"nul_byte", "abc\x00def"}, + {"newline_injection", "abc\ndef"}, + {"tab_injection", "abc\tdef"}, + {"colon_drive", "C:\\Windows"}, + {"space_inside", "id with spaces"}, + {"unicode_dots", "abc․def"}, // U+2024 ONE DOT LEADER — looks like . + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if err := ValidateCertificateID(tc.id); err == nil { + t.Errorf("id=%q: expected rejection, got nil", tc.id) + } + }) + } +}