Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 32 seconds. ⌛ 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: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough移除 PR‑Agent 工作流与配置，升级工作区版本并更新 README，新增 OCSP 解析/验证依赖与公开验证 API，强化 OCSP 响应验证、缓存检查与刷新失败处理，扩展 OCSP 测试；删除两份 TLS 规划文档。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Cert as 证书链
participant Req as OCSP 请求构建器
participant Fetch as OCSP 获取器
participant Valid as OCSP 验证器
participant Cache as 缓存管理器
participant State as 状态记录器
Client->>Cert: 建立 TLS（需 Staple）
Cert->>Req: 提供证书链
Req->>Req: 提取 CertId，构建 OCSP 请求 DER
Req->>Fetch: 发送 OCSP 请求
Fetch->>Valid: 返回 OCSP 响应 DER
Valid->>Valid: 解析 DER，检查 response_status 与 response_type
Valid->>Valid: 匹配 CertId，验证 thisUpdate / nextUpdate（相对于 SystemTime::now()）
alt 验证成功
Valid->>Cache: 写入或更新缓存（受 MAX_OCSP_RESPONSE_BYTES 限制）
Cache->>State: 记录成功
State->>Client: 提供有效 stapled OCSP
else 验证失败或格式错误
Valid->>Cache: 按需原子清空缓存（写入空文件并重命名）
Cache->>State: 记录失败与清理标志
State->>Client: 不提供或移除 stapled OCSP
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens OCSP stapling handling for the v0.1.3-rc.4 release by validating fetched/stored OCSP responses against the current certificate and validity window, clearing stale cache entries on refresh failures, and updating release metadata while removing the PR-Agent workflow/config.
Changes:
- Add OCSP response validation (certificate match +
thisUpdate/nextUpdatewindow) before stapling/caching, and clear invalid/expired cache entries on refresh failures. - Extend status/snapshot OCSP cache inspection to surface cache validation errors, and add regression tests for valid/invalid/expired OCSP paths.
- Bump version/docs references to
v0.1.3-rc.4and remove PR-Agent automation files from the branch.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates displayed current version/tag examples for v0.1.3-rc.4. |
| docs/ssl-plan.md | Removed SSL/TLS plan document from the branch. |
| docs/ssl-compat-matrix.md | Removed SSL/TLS compatibility matrix document from the branch. |
| crates/rginx-runtime/src/ocsp.rs | Validates OCSP responses before caching; clears stale cache on refresh failure; rebuilds TLS acceptors when cache changes. |
| crates/rginx-http/src/tls/certificates.rs | Adds OCSP response parsing + certificate/time-window validation; ignores invalid cached staples at load; adds unit tests. |
| crates/rginx-http/src/tls.rs | Exposes OCSP validation helper via rginx_http API. |
| crates/rginx-http/src/state.rs | Enhances OCSP cache inspection for status snapshots by validating cached response and reporting errors. |
| crates/rginx-http/src/lib.rs | Re-exports OCSP validation API. |
| crates/rginx-http/Cargo.toml | Adds OCSP/DER parsing dependencies needed for validation. |
| crates/rginx-app/tests/ocsp.rs | Adds integration coverage for invalid response rejection and expired cache clearing. |
| crates/rginx-app/Cargo.toml | Adds test-time dependencies for constructing OCSP responses. |
| Cargo.toml | Bumps workspace version to 0.1.3-rc.4. |
| Cargo.lock | Locks new dependencies and version bump. |
| .pr_agent.toml | Removes PR-Agent configuration. |
| .github/workflows/pr-agent.yaml | Removes PR-Agent workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/rginx-http/src/tls/certificates.rs (1)
369-393: 时间验证逻辑正确，可考虑添加时钟偏移容差当前实现精确比较
thisUpdate和nextUpdate。在生产环境中，服务器时钟可能存在轻微偏移，可以考虑添加少量容差（如 5 分钟）以提高健壮性。这不是阻塞问题，当前实现在大多数场景下可正常工作。♻️ 可选：添加时钟偏移容差
+const CLOCK_SKEW_TOLERANCE: std::time::Duration = std::time::Duration::from_secs(300); // 5 minutes + fn validate_ocsp_response_time( path: &Path, response: &x509_ocsp::SingleResponse, now: SystemTime, ) -> Result<()> { let this_update = response.this_update.0.to_system_time(); - if this_update > now { + let future_tolerance = now + CLOCK_SKEW_TOLERANCE; + if this_update > future_tolerance { return Err(Error::Server(format!( "OCSP response for certificate `{}` is not yet valid (thisUpdate is in the future)", path.display() ))); } if let Some(next_update) = response.next_update { let next_update = next_update.0.to_system_time(); - if next_update < now { + let past_tolerance = now.checked_sub(CLOCK_SKEW_TOLERANCE).unwrap_or(now); + if next_update < past_tolerance { return Err(Error::Server(format!( "OCSP response for certificate `{}` is expired (nextUpdate is in the past)", path.display() ))); } } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/tls/certificates.rs` around lines 369 - 393, The time checks in validate_ocsp_response_time should allow a small clock skew tolerance (e.g., 5 minutes) to avoid false positives; add use std::time::Duration, define let skew = Duration::from_secs(300), then change the comparisons to consider skew: treat the response as "not yet valid" only if this_update > now + skew, and treat it as "expired" only if next_update + skew < now (when next_update is Some). Update the error messages to mention the applied tolerance and keep function name validate_ocsp_response_time and variables this_update/next_update/now for locating the change.crates/rginx-runtime/src/ocsp.rs (2)
284-287: 冗余文件读取
clear_ocsp_cache_file在 Line 285 再次读取文件检查是否为空，但调用方clear_invalid_ocsp_cache_file已经在 Line 273 验证过文件非空。如果clear_ocsp_cache_file仅被clear_invalid_ocsp_cache_file调用，这个检查是冗余的。如果此函数需要独立使用，可以考虑添加参数来跳过重复检查：
♻️ 可选的重构建议
-async fn clear_ocsp_cache_file(path: &Path) -> Result<bool, String> { - if tokio::fs::read(path).await.ok().is_some_and(|body| body.is_empty()) { - return Ok(false); - } +async fn clear_ocsp_cache_file(path: &Path, skip_empty_check: bool) -> Result<bool, String> { + if !skip_empty_check { + if tokio::fs::read(path).await.ok().is_some_and(|body| body.is_empty()) { + return Ok(false); + } + }然后在
clear_invalid_ocsp_cache_file调用时传入true：- clear_ocsp_cache_file(cache_path).await + clear_ocsp_cache_file(cache_path, true).await🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-runtime/src/ocsp.rs` around lines 284 - 287, The function clear_ocsp_cache_file performs a redundant file read to check for emptiness even though its caller clear_invalid_ocsp_cache_file already verified the file is non-empty; either remove the duplicate read in clear_ocsp_cache_file (trusting the caller) or add a Boolean parameter (e.g., skip_empty_check: bool) to skip the internal read when the caller has already validated the file, and update clear_invalid_ocsp_cache_file to pass skip_empty_check = true so the redundant check is avoided.
259-282: TOCTOU 竞态窗口（低风险）
clear_invalid_ocsp_cache_file在 Line 263 读取文件、Line 277 验证、Line 281 清除之间存在时间窗口。理论上文件可能在验证后、清除前被其他进程更新为有效响应。考虑到：
- OCSP 刷新周期为 6 小时，并发更新概率极低
- 最坏情况仅导致有效缓存被清除后重新获取
- 不影响安全性
这是一个可接受的权衡。如果未来需要更严格的保证，可考虑使用文件锁或比较写入前后的内容哈希。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-runtime/src/ocsp.rs` around lines 259 - 282, TOCTOU: clear_invalid_ocsp_cache_file reads the cache, validates it, then clears it allowing a race; fix by re-checking the file contents immediately before deletion: in clear_invalid_ocsp_cache_file (and when using rginx_http::validate_ocsp_response_for_certificate and clear_ocsp_cache_file) after successful validation re-read cache_path and compare the new bytes (or their hash) to the originally read body and only call clear_ocsp_cache_file when they match; if they differ, return Ok(false). This avoids deleting a concurrently-updated valid cache without adding complex locking; alternatively add an advisory file lock around the read/validate/clear sequence if you prefer stronger guarantees.crates/rginx-app/tests/ocsp.rs (1)
369-373: 建议添加注释说明硬编码的 DER 字节这些硬编码字节表示
sha256WithRSAEncryption算法标识符（OID 1.2.840.113549.1.1.11），添加注释有助于维护。📝 建议添加注释
+ // sha256WithRSAEncryption AlgorithmIdentifier (OID 1.2.840.113549.1.1.11 with NULL params) signature_algorithm: AlgorithmIdentifierOwned::from_der(&[ 0x30, 0x0d, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x0b, 0x05, 0x00, ]) .expect("signature algorithm should decode"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/ocsp.rs` around lines 369 - 373, Add an inline comment next to the hardcoded DER byte array used in AlgorithmIdentifierOwned::from_der for the signature_algorithm field explaining that these bytes encode the sha256WithRSAEncryption algorithm identifier (OID 1.2.840.113549.1.1.11) so future readers understand the meaning of the literal DER bytes; reference the signature_algorithm assignment and AlgorithmIdentifierOwned::from_der when adding the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/rginx-app/tests/ocsp.rs`:
- Around line 369-373: Add an inline comment next to the hardcoded DER byte
array used in AlgorithmIdentifierOwned::from_der for the signature_algorithm
field explaining that these bytes encode the sha256WithRSAEncryption algorithm
identifier (OID 1.2.840.113549.1.1.11) so future readers understand the meaning
of the literal DER bytes; reference the signature_algorithm assignment and
AlgorithmIdentifierOwned::from_der when adding the comment.
In `@crates/rginx-http/src/tls/certificates.rs`:
- Around line 369-393: The time checks in validate_ocsp_response_time should
allow a small clock skew tolerance (e.g., 5 minutes) to avoid false positives;
add use std::time::Duration, define let skew = Duration::from_secs(300), then
change the comparisons to consider skew: treat the response as "not yet valid"
only if this_update > now + skew, and treat it as "expired" only if next_update
+ skew < now (when next_update is Some). Update the error messages to mention
the applied tolerance and keep function name validate_ocsp_response_time and
variables this_update/next_update/now for locating the change.
In `@crates/rginx-runtime/src/ocsp.rs`:
- Around line 284-287: The function clear_ocsp_cache_file performs a redundant
file read to check for emptiness even though its caller
clear_invalid_ocsp_cache_file already verified the file is non-empty; either
remove the duplicate read in clear_ocsp_cache_file (trusting the caller) or add
a Boolean parameter (e.g., skip_empty_check: bool) to skip the internal read
when the caller has already validated the file, and update
clear_invalid_ocsp_cache_file to pass skip_empty_check = true so the redundant
check is avoided.
- Around line 259-282: TOCTOU: clear_invalid_ocsp_cache_file reads the cache,
validates it, then clears it allowing a race; fix by re-checking the file
contents immediately before deletion: in clear_invalid_ocsp_cache_file (and when
using rginx_http::validate_ocsp_response_for_certificate and
clear_ocsp_cache_file) after successful validation re-read cache_path and
compare the new bytes (or their hash) to the originally read body and only call
clear_ocsp_cache_file when they match; if they differ, return Ok(false). This
avoids deleting a concurrently-updated valid cache without adding complex
locking; alternatively add an advisory file lock around the read/validate/clear
sequence if you prefer stronger guarantees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d932f6a8-b7c2-4cc2-b5da-ea7fc1b246c3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.github/workflows/pr-agent.yaml.pr_agent.tomlCargo.tomlREADME.mdcrates/rginx-app/Cargo.tomlcrates/rginx-app/tests/ocsp.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/lib.rscrates/rginx-http/src/state.rscrates/rginx-http/src/tls.rscrates/rginx-http/src/tls/certificates.rscrates/rginx-runtime/src/ocsp.rsdocs/ssl-compat-matrix.mddocs/ssl-plan.md
💤 Files with no reviewable changes (4)
- .pr_agent.toml
- docs/ssl-plan.md
- docs/ssl-compat-matrix.md
- .github/workflows/pr-agent.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-http/src/state.rs`:
- Around line 2053-2055: The current match on std::fs::metadata(path) swallows
all errors and returns (false, None, None, None); change it to only treat
NotFound as a missing cache and surface all other errors into the cache_error
slot so callers can see permission/corruption issues. Concretely, in the
metadata = match std::fs::metadata(path) block, on Err(e) check e.kind() ==
std::io::ErrorKind::NotFound and return (false, None, None, None) only in that
case; for any other error, return (false, None, None, Some(e.to_string() or
appropriate error type)) so cache_error is populated (preserve existing tuple
shape and types used elsewhere). Ensure references to the metadata variable and
the tuple return shape remain consistent with the surrounding function
(state.rs).
In `@crates/rginx-runtime/src/ocsp.rs`:
- Around line 251-257: 当前在 match clear_invalid_ocsp_cache_file(...) 的 Err 分支只返回
false 导致上层不会调用 refresh_tls_acceptors_from_current_config()，这会让已过期/失效的 stapled
OCSP 继续被已加载的 acceptor 服务；修改该分支逻辑使其在清理失败时也强制触发 acceptor 失效/重建（即将 Err
分支的“需要重建”标志设为 true 或另行返回表示“强制重建”），并在日志信息里保留 clear_error 细节；调整与
clear_invalid_ocsp_cache_file、refresh_tls_acceptors_from_current_config
相关的调用/返回值契约以确保上层能正确处理强制重建。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e351935e-1688-4888-a0e0-2432f72c407a
📒 Files selected for processing (6)
README.mdcrates/rginx-app/tests/ocsp.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/state.rscrates/rginx-http/src/tls/certificates.rscrates/rginx-runtime/src/ocsp.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/rginx-http/src/lib.rs
- crates/rginx-http/src/tls/certificates.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rginx-http/src/state.rs (1)
2025-2031:⚠️ Potential issue | 🟡 Minor不要让缓存错误覆盖 responder 根因。
Line 2025 这里把
cache_error放在responder_error前面后，只要“缓存无效”和“responder 发现/解析失败”同时发生，TlsOcspStatusSnapshot.last_error就只会暴露缓存问题，真正导致无法自动刷新的根因会被遮住。建议至少合并两类错误，或继续优先展示 responder/request 失败原因。建议修改
- let static_error = cache_error.or(responder_error).or_else(|| { + let static_error = match (cache_error, responder_error) { + (Some(cache_error), Some(responder_error)) => Some(format!( + "{responder_error}; cached OCSP staple also invalid: {cache_error}" + )), + (None, Some(responder_error)) => Some(responder_error), + (Some(cache_error), None) => Some(cache_error), + (None, None) => None, + } + .or_else(|| { if bundle.ocsp_staple_path.is_some() && responder_urls.is_empty() { Some("certificate does not expose an OCSP responder URL".to_string()) } else { request_error } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/state.rs` around lines 2025 - 2031, The current static_error computation puts cache_error before responder_error which lets cache misses mask the real responder/request failures (affecting TlsOcspStatusSnapshot.last_error); change the logic in the static_error assignment so responder_error and request_error are prioritized (e.g., check responder_error.or(request_error) first) or combine messages when both exist (merge cache_error with responder_error/request_error) and keep the existing bundle.ocsp_staple_path and responder_urls checks intact; update the expression around static_error, cache_error, responder_error, and request_error to ensure responder/request failures are surfaced rather than being overridden by cache errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-http/src/state.rs`:
- Around line 2067-2095: The snapshot code in status_snapshot currently calls
std::fs::read(path) after checking metadata.len(), which is racy and does
blocking full-file I/O; change this to a bounded, non-blocking-safe approach:
open the file and stream/read at most crate::MAX_OCSP_RESPONSE_BYTES + 1 bytes
(e.g., File::open then read_exact/read_to_end with a size cap) to detect
oversize without reading arbitrarily large data, then call
validate_ocsp_response_for_certificate on the bounded buffer; alternatively move
the whole read-and-validate work into a blocking thread via
tokio::task::spawn_blocking (or perform validation during refresh and cache the
result) so status_snapshot does not perform blocking disk I/O—update the logic
around metadata.len(), the std::fs::read call, and the cache_error construction
to use the capped stream or spawn_blocking path and preserve the existing
size/modified return shape and MAX_OCSP_RESPONSE_BYTES checks.
---
Outside diff comments:
In `@crates/rginx-http/src/state.rs`:
- Around line 2025-2031: The current static_error computation puts cache_error
before responder_error which lets cache misses mask the real responder/request
failures (affecting TlsOcspStatusSnapshot.last_error); change the logic in the
static_error assignment so responder_error and request_error are prioritized
(e.g., check responder_error.or(request_error) first) or combine messages when
both exist (merge cache_error with responder_error/request_error) and keep the
existing bundle.ocsp_staple_path and responder_urls checks intact; update the
expression around static_error, cache_error, responder_error, and request_error
to ensure responder/request failures are surfaced rather than being overridden
by cache errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d3ed6753-5e8c-466d-8ed2-b1f757aa7a66
📒 Files selected for processing (2)
crates/rginx-http/src/state.rscrates/rginx-runtime/src/ocsp.rs
✅ Files skipped from review due to trivial changes (1)
- crates/rginx-runtime/src/ocsp.rs
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v0.1.3-rc.4release updatesValidation
./scripts/prepare-release.sh --tag v0.1.3-rc.4