mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-12 21:58:52 +00:00
fix(security): close BUNDLE 1 — server+agent connector config validation chain
Bundle 1 closure (2026-05-12 acquisition diligence audit). Closes the
acquisition-blocker chain: target.edit (default r-operator grant per
migrations/000029_rbac.up.sql:196) → arbitrary reload_command stored
without validation → agent createTargetConnector json.Unmarshal-only
→ sh -c on agent host. README's 'shell injection prevention on all
connector scripts' claim is now true at the chain level.
Server-side: new internal/connector/target/configcheck package + a
configcheck.Validate call in target.go::Create + ::Update +
::CreateTarget + ::UpdateTarget (all 4 entry points). Rejects shell
metacharacters in reload_command / validate_command / restart_command
for nginx, apache, haproxy, postfix/dovecot, javakeystore, ssh. Sentinel
errors.Is(err, service.ErrInvalidConnectorConfig) available for handler
400 mapping. Non-shell connector types (F5, IIS, Caddy, Traefik, Envoy,
cloud targets, K8s) are no-ops by design.
Agent-side: defense-in-depth connector.ValidateConfig(ctx, configJSON)
call in cmd/agent/main.go inserted between createTargetConnector and
DeployCertificate. This catches (a) configs pre-dating the server gate,
(b) encrypted-blob tampering, (c) per-connector filesystem invariants
that the server can't check.
F5 (S2 finding): proven docs-vs-code drift, not a security bug. The
applyDefaults function never set Insecure=true; runtime default has
always been Go zero-value (false → TLS verified). Three lying 'default
true' comments in f5/f5.go (lines 30, 45-47, 126) rewritten to match
actual code behavior.
Docs (C4 + C9): README L12 + L68 narrowed — 'any CA / any server' →
'Twelve native CA connectors plus an OpenSSL adapter; fifteen native
deployment-target connectors plus a proxy-agent pattern.' 'Every deploy
goes through atomic-write + ...' narrowed to file-based connectors with
inline link to per-target guarantee matrix. New deployment-model.md §1.6
ships a 15-target × 8-property guarantee table covering atomic write /
owner-perms / SHA-256 idempotency / pre-deploy snapshot / on-failure
rollback / post-deploy TLS verify / Prometheus counters / shell-injection
validation — including the K8s preview honesty marker (CLAIM-H4).
Tests: internal/connector/target/configcheck/configcheck_test.go covers
14 shell-injection payloads (semicolon, pipe, backtick, dollar-paren,
redirect, and-chain, newline, double-quote, escape, dollar-var) × 7
shell-using connectors + benign-command acceptance + non-shell no-op
behavior + empty config + malformed JSON. All pass.
Verification (run from /sessions/gifted-blissful-pasteur/mnt/cowork/certctl):
go fmt ./... # clean (no diffs)
go vet ./... # clean (no findings)
go test -short -count=1 ./internal/... ./cmd/...
# 60+ packages all ok, zero FAIL
Audit-Closes: BUNDLE-1 RT-C1 SEC-M4 CLAIM-M2 CLAIM-L3
Audit-Verifies-False: S2 (F5 'default insecure' was a comment lie, code was always secure)
This commit is contained in:
@@ -8,11 +8,18 @@ import (
|
||||
"log/slog"
|
||||
"time"
|
||||
|
||||
"github.com/certctl-io/certctl/internal/connector/target/configcheck"
|
||||
"github.com/certctl-io/certctl/internal/crypto"
|
||||
"github.com/certctl-io/certctl/internal/domain"
|
||||
"github.com/certctl-io/certctl/internal/repository"
|
||||
)
|
||||
|
||||
// ErrInvalidConnectorConfig is returned by Create / Update / CreateTarget /
|
||||
// UpdateTarget when configcheck.Validate rejects the target's config JSON
|
||||
// (e.g., shell metacharacters in reload_command). The HTTP handler should
|
||||
// map this to 400 via errors.Is. Bundle 1 / RT-C1 closure 2026-05-12.
|
||||
var ErrInvalidConnectorConfig = errors.New("invalid connector config")
|
||||
|
||||
// ErrAgentNotFound is returned by [TargetService.CreateTarget] when the caller
|
||||
// references an agent_id that is empty or does not correspond to a registered
|
||||
// agent. The handler layer maps this to HTTP 400 via [errors.Is]. See C-002 in
|
||||
@@ -121,6 +128,14 @@ func (s *TargetService) Create(ctx context.Context, target *domain.DeploymentTar
|
||||
return fmt.Errorf("unsupported target type: %s", target.Type)
|
||||
}
|
||||
|
||||
// Bundle 1 / RT-C1 closure: reject shell-metacharacter injection in
|
||||
// command-bearing config fields BEFORE encryption + storage. Without
|
||||
// this, target.edit (default r-operator grant) → agent sh -c becomes
|
||||
// an RCE chain. See internal/connector/target/configcheck/configcheck.go.
|
||||
if err := configcheck.Validate(string(target.Type), target.Config); err != nil {
|
||||
return fmt.Errorf("%w: %v", ErrInvalidConnectorConfig, err)
|
||||
}
|
||||
|
||||
if target.ID == "" {
|
||||
target.ID = generateID("target")
|
||||
}
|
||||
@@ -177,6 +192,13 @@ func (s *TargetService) Update(ctx context.Context, id string, target *domain.De
|
||||
return fmt.Errorf("failed to merge config: %w", err)
|
||||
}
|
||||
|
||||
// Bundle 1 / RT-C1 closure: validate the POST-MERGE config to catch
|
||||
// injection attempts even when the redacted-merge path is used.
|
||||
// Validation runs on the same bytes that will be encrypted + stored.
|
||||
if err := configcheck.Validate(string(target.Type), mergedConfig); err != nil {
|
||||
return fmt.Errorf("%w: %v", ErrInvalidConnectorConfig, err)
|
||||
}
|
||||
|
||||
// Encrypt the merged config
|
||||
encrypted, _, encErr := crypto.EncryptIfKeySet(mergedConfig, s.encryptionKey)
|
||||
if encErr != nil {
|
||||
@@ -299,6 +321,13 @@ func (s *TargetService) CreateTarget(ctx context.Context, target domain.Deployme
|
||||
return nil, fmt.Errorf("unsupported target type: %s", target.Type)
|
||||
}
|
||||
|
||||
// Bundle 1 / RT-C1 closure: reject shell-metacharacter injection in
|
||||
// command-bearing config fields BEFORE encryption + storage. Mirrors
|
||||
// Create() above so the HTTP handler entry point is also gated.
|
||||
if err := configcheck.Validate(string(target.Type), target.Config); err != nil {
|
||||
return nil, fmt.Errorf("%w: %v", ErrInvalidConnectorConfig, err)
|
||||
}
|
||||
|
||||
// C-002: enforce agent_id FK at service layer so we return a clean 400
|
||||
// instead of bubbling a Postgres 23503 foreign-key violation out as 500.
|
||||
// The schema (migrations/000001 line 104) declares agent_id TEXT NOT NULL
|
||||
@@ -372,6 +401,12 @@ func (s *TargetService) UpdateTarget(ctx context.Context, id string, target doma
|
||||
return nil, fmt.Errorf("failed to merge config: %w", err)
|
||||
}
|
||||
|
||||
// Bundle 1 / RT-C1 closure: validate the POST-MERGE config (same
|
||||
// reasoning as Update() above).
|
||||
if err := configcheck.Validate(string(target.Type), mergedConfig); err != nil {
|
||||
return nil, fmt.Errorf("%w: %v", ErrInvalidConnectorConfig, err)
|
||||
}
|
||||
|
||||
encrypted, _, encErr := crypto.EncryptIfKeySet(mergedConfig, s.encryptionKey)
|
||||
if encErr != nil {
|
||||
return nil, fmt.Errorf("failed to encrypt config: %w", encErr)
|
||||
|
||||
Reference in New Issue
Block a user