Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough此PR为rginx项目添加ACME(自动证书管理环境)支持,包括CLI命令处理、配置编译调整、HTTP-01挑战响应、账户管理、证书颁发流程和后台调度器集成。 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI / main
participant Config as Config<br/>Compiler
participant State as SharedState
participant Challenge as Temporary<br/>Challenge Server
participant Account as ACME<br/>Account
participant Order as ACME<br/>Order
participant Storage as Storage
User->>CLI: acme issue --once
CLI->>Config: load_and_compile_for_acme_issue
Config->>Config: compile_with_base_and_options<br/>(allow_missing=true)
Config-->>CLI: ConfigSnapshot
CLI->>State: Create from config
CLI->>Challenge: bind_for_config
Challenge->>Challenge: Listen on :80
Challenge-->>CLI: TemporaryChallengeServer
CLI->>Account: load_or_create_account
Account->>Storage: load_account_credentials
alt No stored credentials
Account->>Account: Create new account
Account->>Storage: store_account_credentials
end
Account-->>CLI: Account ready
loop For each managed certificate
CLI->>Order: issue_and_store_managed_certificate
Order->>Account: Create order
Order->>Account: Get authorizations
Order->>Order: Select HTTP-01 challenge
Order->>Challenge: register challenge token
Order->>Order: Set challenge ready
Order->>Order: Poll until ready
loop Token registered
User-->>Challenge: GET /.well-known/acme-challenge/{token}
Challenge->>State: acme_http01_response(token)
State-->>Challenge: Key authorization
Challenge-->>User: 200 text/plain
end
Order->>Order: Finalize with CSR
Order->>Order: Poll for certificate
Order->>Storage: write_certificate_pair
Storage-->>Order: Cert written
Order->>Challenge: unregister challenge token
Order-->>CLI: Success
end
CLI->>Challenge: shutdown
Challenge->>Challenge: Stop listening
Challenge-->>CLI: Done
CLI-->>User: IssueSummary
sequenceDiagram
participant Client as ACME<br/>Client
participant Handler as HTTP<br/>Handler
participant Dispatcher as Request<br/>Dispatcher
participant AcmeModule as ACME<br/>Module
participant SharedState as SharedState
Client->>Handler: GET /.well-known/acme-challenge/{token}
Handler->>Dispatcher: dispatch(request_path)
Dispatcher->>AcmeModule: http01_response(request_path)
AcmeModule->>AcmeModule: Parse token from path
AcmeModule->>SharedState: acme_http01_response(token)
SharedState->>SharedState: Lookup in acme_http01_challenges
SharedState-->>AcmeModule: Option<key_authorization>
alt Token registered
AcmeModule-->>Dispatcher: Some(key_authorization)
Dispatcher->>Dispatcher: Create 200 text/plain response
Dispatcher-->>Handler: Response
else Token not found
AcmeModule-->>Dispatcher: None
Dispatcher->>Dispatcher: Continue normal routing
end
Handler-->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 27 minutes and 46 seconds.Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds comprehensive ACME certificate issuance and renewal functionality with HTTP-01 challenge support. The implementation includes account persistence, atomic certificate writes, and runtime reconciliation with exponential backoff retry logic.
Critical Issue
Security Vulnerability (Must Fix): Race condition in atomic_write on non-Unix systems where private keys may be created with insecure default permissions before content is written, potentially exposing credentials during the write window (CWE-378).
Architecture Review
The implementation demonstrates solid engineering practices:
- Proper separation of concerns with dedicated modules for account management, challenge handling, and order processing
- Atomic file writes with fsync for durability guarantees
- Exponential backoff retry logic (60s → 6h max) for failed certificate issuances
- Challenge token cleanup in defer-style pattern ensuring unregistration even on errors
- Integration with existing HTTP dispatch for ACME challenge responses
- Comprehensive test coverage for reconcile planning and certificate persistence
The issue_once command provides a safe cold-start flow, allowing managed TLS identities to compile before the first certificate issuance. The runtime reconciler properly handles configuration reloads and refreshes TLS acceptors after certificate updates.
Recommendation
Approval contingent on fixing the security vulnerability. After addressing the file permission race condition on non-Unix platforms, this PR is ready to merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
|
Addressed the Amazon Q review concern in crates/rginx-runtime/src/acme/storage.rs: the non-Unix fallback no longer creates ACME temp files with platform-default permissions and now fails explicitly instead. Re-ran cargo test -p rginx-runtime acme::tests and ./scripts/run-clippy-gate.sh after the change. |
![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: 0acb48713a
ℹ️ 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".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements automated ACME certificate management, including a new acme CLI subcommand for manual issuance and a background service for automatic renewals. The implementation introduces a temporary HTTP-01 challenge server, ACME account and order orchestration via the instant-acme crate, and logic for atomic certificate storage. Feedback suggests improving the robustness of the temporary challenge listener by handling transient accept errors and refining the directory creation logic in the storage module to handle edge cases in path resolution.
There was a problem hiding this comment.
Pull request overview
Adds an ACME runtime subsystem to rginx to support managed certificate issuance/renewal (HTTP-01) and a one-shot issuance CLI path, while wiring HTTP-01 handling into the request dispatcher and relaxing compile-time TLS identity checks for the cold-start issuance workflow.
Changes:
- Introduces a new
rginx-runtime::acmemodule (instant-acme based) with account persistence, issuance, reconciliation scheduling, and HTTP-01 challenge handling. - Adds
rginx acme issue --onceplus a config compile mode that allows managed TLS identities to compile before the first issuance. - Wires HTTP-01 challenge short-circuiting into the HTTP dispatcher and adds lifecycle state for challenge token storage.
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rginx-runtime/src/lib.rs | Exposes the new acme runtime module. |
| crates/rginx-runtime/src/bootstrap/mod.rs | Spawns the ACME background task during runtime startup. |
| crates/rginx-runtime/src/bootstrap/shutdown.rs | Adds ACME task to graceful shutdown/join/abort flow. |
| crates/rginx-runtime/src/bootstrap/shutdown/tests.rs | Extends shutdown tests to cover the ACME task. |
| crates/rginx-runtime/src/acme/mod.rs | ACME module entrypoints: background run + one-shot issuance. |
| crates/rginx-runtime/src/acme/account.rs | Loads/creates ACME account with persisted credentials. |
| crates/rginx-runtime/src/acme/challenge.rs | Temporary HTTP-01 challenge server + runtime challenge backend. |
| crates/rginx-runtime/src/acme/order.rs | ACME order flow: authz, HTTP-01 readiness, finalize, fetch cert, write to disk. |
| crates/rginx-runtime/src/acme/scheduler.rs | Periodic reconcile/renew loop with retry backoff and TLS acceptor refresh. |
| crates/rginx-runtime/src/acme/storage.rs | Persists account creds; writes certificate/key material to disk. |
| crates/rginx-runtime/src/acme/types.rs | Reconcile planning helpers + listener addr discovery. |
| crates/rginx-runtime/src/acme/tests.rs | Unit tests for listener detection, reconcile planning, and storage writes. |
| crates/rginx-runtime/Cargo.toml | Adds instant-acme (+ http) and tempfile dev-dependency. |
| crates/rginx-http/src/state/mod.rs | Adds in-memory ACME HTTP-01 challenge map to SharedState. |
| crates/rginx-http/src/state/lifecycle.rs | Registers new lifecycle submodule for ACME helpers. |
| crates/rginx-http/src/state/lifecycle/acme.rs | SharedState APIs to register/unregister/fetch HTTP-01 challenge responses. |
| crates/rginx-http/src/handler/dispatch/mod.rs | Short-circuits requests to serve ACME HTTP-01 tokens before routing. |
| crates/rginx-http/src/handler/dispatch/acme.rs | Parses ACME HTTP-01 request path and fetches response from SharedState. |
| crates/rginx-http/src/handler/tests/routing/handle.rs | Adds test verifying ACME short-circuit overrides route handling. |
| crates/rginx-config/src/lib.rs | Adds load_and_compile_for_acme_issue helper for one-shot issuance mode. |
| crates/rginx-config/src/compile/mod.rs | Adds CompileOptions and plumbs option through compile pipeline. |
| crates/rginx-config/src/compile/vhost.rs | Allows missing managed TLS identity for ACME vhosts when compile option enabled. |
| crates/rginx-config/src/compile/server.rs | Plumbs allow-missing-managed-identity options into server compilation. |
| crates/rginx-config/src/compile/server/fields.rs | Adds allow_missing_tls_identity into server field compilation. |
| crates/rginx-config/src/compile/server/listener.rs | Allows missing identities only for listeners using managed (ACME) cert/key pairs. |
| crates/rginx-config/src/compile/server/listener_managed_identity.rs | Helper to detect whether a listener/server TLS identity is ACME-managed. |
| crates/rginx-config/src/compile/server/tls.rs | Plumbs allow-missing flag into TLS compilation. |
| crates/rginx-config/src/compile/server/tls/identity.rs | Skips cert/key/OCSP existence checks when allow-missing is enabled. |
| crates/rginx-config/src/compile/tests/acme.rs | Adds test for compile mode that allows missing managed identity files. |
| crates/rginx-app/src/cli.rs | Adds rginx acme issue --once clap command shape. |
| crates/rginx-app/src/cli/tests.rs | Adds CLI parsing test for one-shot ACME issuance. |
| crates/rginx-app/src/main.rs | Early-dispatches ACME one-shot command and runs issuance without full runtime startup. |
| crates/rginx-app/src/admin_cli/mod.rs | Treats acme as non-admin command for the admin CLI pathway. |
| Cargo.toml | Adds workspace dependency on instant-acme. |
| Cargo.lock | Locks new dependencies pulled in by instant-acme / runtime changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the remaining review threads in
Validation rerun on this commit:
Note on the cross-file atomicity comments: with separate configured cert/key paths, we can make ordinary failure handling consistent via rollback, but a truly crash-atomic two-file swap would require changing the on-disk identity layout. I kept this PR to the rollback-based fix. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 648b215475
ℹ️ 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".
| tls_runtime_snapshot_for_config(config) | ||
| .certificates | ||
| .into_iter() | ||
| .map(|status| (status.scope.clone(), status)) |
There was a problem hiding this comment.
certificate_status_index stores snapshots under raw status.scope values (e.g. vhost:servers[0] from TLS runtime), but managed ACME specs are compiled with unprefixed scopes (e.g. servers[0]), so plan_reconcile(..., certificate_statuses.get(&spec.scope), ...) never finds an existing status. In practice this makes every managed certificate look missing on each reconcile/acme issue --once run, which repeatedly re-issues already-valid certificates and can quickly hit ACME rate limits.
Useful? React with 👍 / 👎.
Summary
instant-acmebased runtime module for account persistence, order processing, HTTP-01 challenge handling, and atomic certificate/key writesrginx acme issue --oncefor cold-start certificate issuance and allow managed TLS identities to compile before the first issuanceVerification