fix(agent,service): SEC-002 — validate certificate_id shape + contain key path

Sprint 1 unified-master-audit closure. Pre-fix the agent built its
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 compromised control
plane (or a poisoned database row) 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 or read on the agent host.

Fix (two ends; both load-bearing):

Server side:
  - New internal/validation/certificate_id.go: ValidateCertificateID
    pins the canonical TEXT-PK shape (^[A-Za-z0-9._-]{1,128}$, plus
    explicit '.'/'..' rejection).
  - CertificateService.Create now invokes ValidateCertificateID after
    the existing required-fields check; malformed IDs are refused
    before persistence or downstream job creation.

Agent side:
  - cmd/agent/keymem.go: validateAgentCertID mirrors the server-side
    shape regex. safeAgentKeyPath additionally asserts the joined
    path is contained within KeyDir via filepath.Rel — even if a
    future refactor bypasses the shape check, a path that escapes
    KeyDir fails closed.
  - poll.go + deploy.go: both filepath.Join call sites routed
    through safeAgentKeyPath; rejection surfaces via reportJobStatus
    so the control plane sees the failure.

Regression coverage:
  - internal/validation/certificate_id_test.go: production shapes
    accepted; explicit rejection table for empty, overlong, posix
    traversal, absolute, Windows traversal, Windows separator, NUL
    byte, newline/tab injection, drive prefix, space, unicode dots.
  - cmd/agent/keymem_test.go: validateAgentCertID acceptance +
    rejection tables; safeAgentKeyPath happy path + the 8 audit
    vectors plus empty-keyDir refusal.

Closes SEC-002.
This commit is contained in:
shankar0123
2026-05-16 03:31:59 +00:00
parent e6cfd756ac
commit 037dab7b6f
7 changed files with 381 additions and 4 deletions
+10
View File
@@ -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)
+67
View File
@@ -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
}
@@ -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", "abcdef"}, // 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)
}
})
}
}