Repository navigation
fix(tools): harden mTLS material tooling - #80
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ddce3c3f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| serial.SetInt64(firstEntitySerial) | ||
| } | ||
|
|
||
| notBefore, notAfter := o.validity(now()) |
There was a problem hiding this comment.
When a CA is created with a short --valid-for and a later make-server or make-client invocation uses either the default expiry or a longer lifetime, this computes a leaf NotAfter later than caCert.NotAfter. x509.CreateCertificate permits that, but certificate-chain verification still rejects the expired CA, so the generated bundle stops working before the leaf's advertised expiry. Reject such a lifetime or cap the leaf expiry to the issuing CA's expiry.
Useful? React with 👍 / 👎.
| if pool.AppendCertsFromPEM(pem.EncodeToMemory(block)) { | ||
| peers++ |
There was a problem hiding this comment.
If a bundle contains a parseable second certificate that is not a CA, such as a duplicated leaf or an unrelated entity certificate, AppendCertsFromPEM still returns true and increments peers. The new missing-CA guard therefore accepts the malformed bundle, but its trust pool cannot authenticate normal peers and the failure remains deferred to the TLS handshake. Parse appended certificates and require the expected CA constraints before counting them.
Useful? React with 👍 / 👎.
- Issue positive, unique serials: the CA gets a random 128-bit serial and entity serials start at 1; a legacy counter of 0 is bumped. The counter is persisted before the bundle so a crash burns a serial rather than reusing one. - Mark the CA with pathLenConstraint 0 (MaxPathLen/MaxPathLenZero). - Add an optional --valid-for to make-ca/make-server/make-client via tools.WithLifetime. Default expiry stays 2100-01-01. NotAfter counts from the actual issuance time, so a short lifetime can no longer yield an already-expired certificate; non-positive values are rejected. - mtls: fail at load when a bundle yields an empty CA pool instead of rejecting every peer at handshake time. - paths: reject bundle names and ACME email/provider values that are not plain filename components (separators, ".", "..", NUL). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…undles - makeCert caps a default (until 2100) leaf expiry at the issuing CA's NotAfter and prints a notice; an explicit --valid-for that would end after the CA expires is rejected before anything is written. - mtls: only certificates with BasicConstraintsValid && IsCA satisfy the missing-CA check, so a bundle whose trust section is just another leaf fails at load. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
883a60a to
1551e31
Compare
2 participants
Summary
0, and entity serials start at1. A counter file left at0by an older version gets bumped to 1. The counter is now written before the bundle, so a crash between the two writes skips a serial instead of reusing one. The certificate also takes a copy of the counter rather than aliasing it.MaxPathLen: 0/MaxPathLenZero: true, so it can sign entity certs but not intermediates.--valid-for(optional) onmake-ca,make-serverandmake-client, passed through astools.WithLifetime. Default is unchanged: without the flag, certificates are valid until 2100-01-01.--valid-for, a leaf's expiry is capped at the CA's and a notice is printed. An explicit--valid-forthat would end after the CA expires is rejected before anything is written.BasicConstraintsValid && IsCA) after the entity cert and key. Otherwise it now fails at load with an error that names the file. Before, it produced a pool with no usable anchor, and every handshake failed with an unclear error.MtlsBundlePathandACMEPrivateKeynow reject values that are not plain filename components: path separators,.,.., NUL or an empty string. This stops them from writing outsidemtls/orprivate/.docs/tools.mdanddocs/setup.mdnow cover--valid-for, the cap at the CA's expiry, and the bundle-name rule.Review feedback from #75 addressed
DefaultLeafLifetimeandDefaultCALifetimeand the docs text about them. Without--valid-for, NotAfter is still2100-01-01T00:00:00Z, as inmain. The flag defaults to "unset", and help and docs show the default as "valid until 2100-01-01".issuance time + lifetime. It used to beNotBefore + lifetime, so issuing at 12:45 with--valid-for 30mproduced a cert that expired at 12:30. MakeCA and makeCert both get this fix. Non-positive lifetimes are rejected, not silently ignored:--valid-for must be positive, got 0swhen the flag is given explicitly with a value <= 0. It usesfs.Changedto tell "not given" apart from "given as 0".tools.WithLifetime(d <= 0)makesMakeCA/MakeServerCert/MakeClientCertreturn an error before anything is written.TestShortLifetimeIsNotAlreadyExpiredpins the clock to 12:45 and issues with a 30m lifetime. It fails against the old logic (NotAfter 12:30) and passes now (NotAfter 13:15).Review feedback on this PR addressed
caCert.NotAfterand a notice is printed.--valid-forthat overshoots the CA returns an error. No bundle is written and the counter is not advanced.TestLeafDefaultExpiryCappedAtCAandTestLeafExplicitLifetimeBeyondCARejected.BasicConstraintsValid && IsCAcount toward the check. Every parseable cert is still added to the pool, as before.TestParseBundleNonCATrustCert(the bundle's second cert is a non-CA leaf).make-server/make-clientstill load; the full e2e suite passes.Breaking changes
None for defaults: expiry is still 2100, and certificates that already exist are unaffected.
Behaviour changes worth noting:
make-server/make-clientnow reject-nvalues that contain/or\, or that are./... Before, a name likea/bwas joined into a path, which failed or wrote into a subdirectory. The ACME email and provider values that make up the account key file name get the same check.IsCA) now fails at startup instead of at every handshake.--valid-for: the leaf now expires with the CA (default), or the command errors out (explicit--valid-forthat ends after the CA).Test results
gofmt -l pkg exec test: cleango vet ./pkg/...: cleango test -race -count=1 ./pkg/...(root): all okexec/tools:go vet ./... && go build ./... && go test ./...: ok (newTestValidForOptions)go build ./...in exec/caddytls, exec/client and exec/server: oktest/e2e:go vet -tags e2e ./...and fullgo test -tags e2e -count=1 ./...: ok (268s)notAfter=Jan 1 00:00:00 2100 GMT, with CA serial random and entity serials01,02.--valid-for 30m: NotAfter is 08:42.--valid-for 0is rejected.-n ../xis rejected.Split from #75.
🤖 Generated with Claude Code