Repository navigation
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: c0dbdc1f2d
ℹ️ 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".
| if c.ACME.RenewTimeLeftDuration > c.ACME.CertLifeTimeDuration { | ||
| return fmt.Errorf("RenewTimeLeft (%q) must not be longer than CertLifeTime (%q)", | ||
| c.ACME.RenewTimeLeft, c.ACME.CertLifeTime) |
There was a problem hiding this comment.
This rejects previously valid configurations such as certLifeTime = "720h" and renewTimeLeft = "1440h", causing the server to fail at startup after an upgrade. The certificate is not due before issuance in this case: renew records ValidBefore as now + 720h and requests a Google NotAfter of now + 2160h, while the renewal loop checks every 360h. Thus the certificate rotates after 30 days with 60 days remaining, exactly as configured and within the 90-day provider limit. Remove this ordering restriction and retain the total provider-lifetime check.
Useful? React with 👍 / 👎.
RegisterAccount used to write a fresh key over an existing private/<email>_<provider>.key, losing the only proof of ownership of the old account. It now refuses unless force is set, and when forced it puts the previous key back if the registration fails half way through. certdx_tools google-account gains -f/--force to opt in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Reject configs that could never work, or silently did something else, when they are loaded instead of deep inside an ACME order or request: - certLifeTime / renewTimeLeft must be positive and renewTimeLeft must not be longer than certLifeTime; for the Google providers, which put the requested lifetime on the wire, their sum must fit in 90 days (other providers only warn). - DnsProvider nameservers must be host[:port]. - HttpServer authMethod defaults to "token" and must be token or mtls. An enabled token server with an empty token used to serve everyone; that now needs an explicit allowAnonymous = true. Token auth over plain HTTP logs a warning unless allowInsecureToken = true. - HttpProvider S3 requires bucket, url and credentials. The S3 challenge provider gains an optional acl: unset keeps sending "public-read" as before, acl = "" sends no ACL header (needed for buckets with ACLs disabled). Each S3 request now times out after 30s instead of hanging the challenge. "tencent", the spelling used in the sample config, is accepted as an alias of the "tencentcloud" DNS provider type. A test checks the shipped sample configs keep decoding without unknown keys. The stricter rules are listed under "Upgrading" in docs/server.md. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
lego only asks the recursive nameservers for zone / CNAME discovery and verifies the TXT value on the authoritative servers. With disableCompletePropagationRequirement set, that check was skipped and the record was not verified anywhere before the CA was asked to validate. When the authoritative check is disabled (and conservativeDnsCheck, which overrides it, is off) and nameservers are configured, require the record on those resolvers instead; without nameservers, log a warning. The option building moves into dns01Options, keeping the dnsTimeout propagation-wait override and the conservative checker as they were. Also route the "tencent" alias to the Tencent Cloud provider. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The cert is requested for certLifeTime + renewTimeLeft and rotated once certLifeTime has passed, so a renew window longer than certLifeTime (e.g. rotate after 30d with 60d left) is a valid setup. Drop the ordering check; the positive-duration and provider total-lifetime checks stay. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
e6ec590 to
bdaa0fb
Compare
| - `[HttpServer] authMethod = "token" with an empty token ...` — set `token`, | ||
| or `allowAnonymous = true` to serve without authentication. | ||
|
|
||
| ## Upgrading |
There was a problem hiding this comment.
The next release will be v0.8.0, so please write them into a new breaking-changes-v0.8.0.md
2 participants
Summary
The ACME and server config fixes from #75, ported onto current
main:pkg/acme/user.go,certdx_tools google-account):RegisterAccountno longer writes a new key over an existingprivate/<email>_<provider>.key. It refuses unlessforceis set, and a forced run that fails restores the previous key.google-accountgets-f/--force.pkg/acme/challenge.go): withdisableCompletePropagationRequirement = true, lego skipped the authoritative check and did not verify the TXT record anywhere. Whennameserversare configured, the record is now required on those resolvers (dns01.RecursiveNSsPropagationRequirement). Without them the server logs a warning. This is merged with main'sconservativeDnsCheck(e27bcc6): the conservative check still overrides the disable flag and replaces lego's pre-check. ThednsTimeoutpropagation-wait override also works as before. Option building moved intodns01Options, which has tests.acl. If unset,public-readis still sent.acl = ""sends no ACL header, which buckets with ACLs disabled require. Any other canned ACL is sent as-is.Present/CleanUpnow use a 30s per-request timeout instead ofcontext.Background().pkg/config/server.go):certLifeTime/renewTimeLeftmust be positive.renewTimeLeftmay still be longer thancertLifeTime, e.g. rotate after 30d with 60d left.certLifeTime + renewTimeLeftmust be at most 90 days. For other providers this only logs a warning.nameserversentries must behost[:port].authMethodnow defaults totokenand must betokenormtls.allowAnonymous = true.allowInsecureToken = trueturns it off.bucket,urland credentials, andaclmust be a known value.config/server_config*.tomlanddocs/server.mdare updated.docs/server.mdhas a new "Upgrading" section.docs/tools.mddocuments--force.pkg/config/samples_test.godecodes every shipped sample inconfig/(server and client) and fails on unknown keys.server_config.tomlandclient_config.tomlmust also passValidate.Review feedback from #75 addressed
maxCacheEntries: "Is it really necessary? I'd suggest don't add this." It is not in this PR: no field, sample key or docs.# type = "tencent"inserver_config_full.toml: "Keep using old value "tencent" for simplicity." The line stays as it is. However, thetencentcloudprovider only matched"tencentcloud", so uncommenting that sample line gaveunknown DnsProvider: tencent. To keep the sample usable,"tencent"is now accepted as an alias oftencentcloudin both validation and challenger selection (config.DnsProviderTypeTencent). Tests cover both. The docs nametencentcloudas the main value and mention the alias.docs/server.md. No versioned breaking-changes file was added, since the next version number is not decided yet.Breaking changes
Configs that loaded before and are now rejected at startup:
[HttpServer]using token auth withtoken = ""used to serve certificates to everyone. It is now an error. Set atoken, or setallowAnonymous = trueif anonymous access is intended.authMethod. Values other thantoken/mtlsare now rejected when the config loads. Before,HttpSrvfailed withunsupported HTTP auth methodwhen it started. An unsetauthMethodused to fail the same way; it now defaults totoken(so rule 1 applies).certLifeTimeorrenewTimeLeftthat is zero or negative is rejected. There is no ordering rule between them.provider = "google"/"googletest",certLifeTime + renewTimeLeftover 90 days is rejected. The CA would have refused the order anyway. Let's Encrypt only logs a warning.[DnsProvider].nameserversentry that is nothostorhost:portis rejected: empty entries, URLs/schemes, paths, whitespace, a bad port, or an invalid hostname.[HttpProvider.S3]withoutbucket,url,accessKeyIdoraccessKeySecretis rejected. An unknownaclvalue is rejected too.certdx_tools google-accountrefuses to run when the account key already exists; pass--forceto replace it. For Go callers,acme.RegisterAccounttakes a newforce boolparameter.No behaviour change for S3 users who leave
aclout:public-readis still sent.Test results
gofmt -l pkg exec test: cleango vet ./pkg/...: cleango test -race ./pkg/...: all passexec/tools,exec/server:go build ./... && go vet ./... && go test ./...passexec/client,exec/caddytls:go build ./...passtest/e2e:go vet -tags e2e ./...andgo test -tags e2e ./...pass (346s)Split from #75.
🤖 Generated with Claude Code