fix: reject client CN containing path separator (#31) #37

Manually merged

heiko merged 7 commits from fix/31-reject-cn-path-separator into master

2026-05-20 23:48:36 +02:00

heiko commented

2026-05-20 19:44:43 +02:00

Owner

Fixes #31.

Summary

Validate the X.509 CN before using it as a filename in the clients config dir.
Reuse list.ValidateDomain (already used in the client). It rejects /, \, NUL,
empty CN, leading/trailing dot, and other unsafe characters via the same
regex used for domain names elsewhere.
Error messages do not echo the CN, so the response from auth.go/serve.go
stays free of attacker-controlled input (relates to #29).

Test plan

go test ./cmd/cert-proxy-server/ — TestCnList_RejectsPathSeparator covers
sub/inner, sub\inner, NUL, empty, leading dot.
golangci-lint run ./... is clean.
Existing TestCnList_Valid / TestCnList_Comments / TestCnList_Missing
still pass (no regression on the happy path).

(co)authored by ai (claude-opus-4-7)

Fixes #31. ## Summary - Validate the X.509 CN before using it as a filename in the clients config dir. - Reuse list.ValidateDomain (already used in the client). It rejects /, \\, NUL, empty CN, leading/trailing dot, and other unsafe characters via the same regex used for domain names elsewhere. - Error messages do not echo the CN, so the response from auth.go/serve.go stays free of attacker-controlled input (relates to #29). ## Test plan - [x] go test ./cmd/cert-proxy-server/ — TestCnList_RejectsPathSeparator covers sub/inner, sub\\inner, NUL, empty, leading dot. - [x] golangci-lint run ./... is clean. - [x] Existing TestCnList_Valid / TestCnList_Comments / TestCnList_Missing still pass (no regression on the happy path). (co)authored by ai (claude-opus-4-7)

heiko added 3 commits

2026-05-20 19:44:43 +02:00

test: add regression for CN path separator (#31 ) 8294cb7f98

Asserts cnList rejects CNs containing /, \, NUL, empty,
or starting with '.'. Also guards against echoing the CN in
the error message, which would be forwarded to the client
by auth.go (see #29).

fix: reject client CN containing path separator 6c9fd11567

Treat CN as opaque identifier and validate before using it as a
filename in the clients config dir. http.Dir already blocks ..
traversal, but accepts forward slashes as path components, so a
CN like sub/inner would read clients/sub/inner.

Reject: empty CN, leading '.', and any '/', '\' or NUL. Returns
a sentinel error that does not echo the CN, since auth.go and
serve.go forward the error message to the HTTP client.

Fixes #31.

refactor: reuse list.ValidateDomain for CN check 85718326f4

Replace the inline path-separator check with list.ValidateDomain,
already used in the client. It rejects the same characters via
^[a-zA-Z0-9*._-]+$ plus empty labels, and its error messages do
not echo the input.

Drops the local errInvalidCN sentinel and the errors/strings
imports.

heiko added 1 commit

2026-05-20 21:44:06 +02:00

feat: reject Windows reserved names in client CN 135d674292

Add list.ValidateClientName for filenames in the per-client config
dir. It extends ValidateDomain with rejection of:
- DOS/Windows reserved device names CON, PRN, AUX, NUL,
  COM0..9, LPT0..9 — case-insensitive, applied to the base name
  before any extension (CON.txt resolves to the console device
  on Windows regardless of extension).
- the wildcard character *, which is reserved on Windows and has
  no meaning for a CN.

cnList now uses ValidateClientName. ValidateDomain is unchanged
because legitimate DNS names may contain labels like con.

The new behavioural test plants clients/CON and clients/sub/inner
so that vacuous validation would observably leak content.

heiko referenced this pull request

2026-05-20 21:48:42 +02:00

serve.go: validate URL-derived domain (defense in depth, Windows considerations) #38

heiko referenced this pull request

2026-05-20 21:51:02 +02:00

client: ValidateDomain accepts Windows reserved names used as path components #39

heiko referenced this pull request from a commit

2026-05-20 22:00:58 +02:00

fix: map invalid client CN to 401 instead of 500

heiko added 1 commit

2026-05-20 22:00:58 +02:00

fix: map invalid client CN to 401 instead of 500 17b40065ce

Before this change cnList returned the bare ValidateClientName
error, and both call sites (auth.go for authz, serve.go for the
list endpoint) only special-cased os.IsNotExist for the 401
mapping. A CA-issued client cert carrying a malformed CN therefore
hit the default http.StatusInternalServerError branch, which is a
behaviour regression introduced by #31 / #37.

Export ErrInvalidName from internal/list and wrap every
ValidateDomain / ValidateClientName failure with %w of that
sentinel. Both call sites now treat ErrInvalidName the same way as
a missing config file — 401 Unauthorized, since the client is
authenticated by mutual TLS but the CN is not acceptable.

TestAuthz_InvalidCN proves the new behaviour: a CN containing /
must yield 401, not 500.

Also skip the clients/CON file plant on Windows so the test setup
does not address the console device.

heiko added 1 commit

2026-05-20 22:11:12 +02:00

test: fuzz target and HTTP-boundary checks for client CN fd629d8643

internal/list/validate_fuzz_test.go: FuzzValidateClientName. The
asserted invariants are orthogonal to the validator's
implementation, so a regression cannot pass by accident:

  - every failure must wrap ErrInvalidName (errors.Is)
  - any input ValidateClientName accepts must also be accepted
    by ValidateDomain (ValidateClientName is strictly stricter)

A 20s run with 8 workers reaches ~1.1M execs without finding a
violation; the seed corpus carries every interesting case from
TestCnList_RejectsUnsafeName.

cmd/cert-proxy-server/server_test.go: two new behavioural tests
at the HTTP boundary, distinct from the function-level checks:

  - TestServe_ListInvalidCN — /v1/list with a malformed CN
    returns 401 (not 500), and the response body does not echo
    the attacker-controlled CN
  - TestAuthz_InvalidCN body assertion — same no-echo guarantee
    extended from the err.Error() level to the actual w.Body

These guard the path that auth.go and serve.go take when cnList
returns ErrInvalidName.

heiko added 1 commit

2026-05-20 22:31:35 +02:00

fix: sanitize 401 body, strengthen fuzz invariants ff83a40da1

Three findings from codex's second-pass review:

1) auth.go and serve.go forwarded err.Error() as the 401 response
   body for the os.IsNotExist branch too, not just for the new
   ErrInvalidName branch. The underlying error from http.Dir.Open
   looks like "open /etc/cert-proxy/clients/<cn>: no such file or
   directory", which reflects both the configured path and the
   attacker-controlled CN. Replace the body with a generic
   "unauthorized" string for both 401 paths; the detailed err is
   still propagated up the handler for logging.

   TestAuthz_NoConfigFile now asserts the body does not contain
   the attacker CN or the clients directory path.

2) FuzzValidateClientName's accepted-side invariant was too weak.
   If ValidateClientName regressed to just calling ValidateDomain
   the fuzz would still pass, because the only positive assertion
   was "ValidateDomain also accepts." Add explicit invariants for
   the rules ValidateClientName adds on top: no '*' and the base
   name (before any dot) is not in windowsReserved.

3) Document COM0/LPT0 inclusion. Go's filepathlite only checks
   COM1..9 / LPT1..9; Microsoft's Naming Files docs explicitly
   list COM0 and LPT0 as reserved. We follow Microsoft.

heiko referenced this pull request from a commit

2026-05-20 23:48:34 +02:00

Merge #37: reject client CN containing path separator ai:claude-opus-4-7

heiko manually merged commit cc0ef6114e into master

2026-05-20 23:48:36 +02:00