Refactor PKI parsing and prepare v0.1.3-rc.8#38
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 19 minutes and 56 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 (2)
📝 WalkthroughWalkthrough将工作区从基于 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as rginx Server
participant PKI as crate::pki
participant CRL_FS as CRL/FileSystem
participant WebPKI as webpki
Client->>Server: 建立 TLS 连接 (带证书链)
Server->>PKI: parse_tls_client_identity(der_chain)
PKI->>PKI: 使用 rasn 解码每个 Certificate，提取字段与 SAN
PKI->>Server: 返回 ParsedClientIdentity (leaf fields, chain_subjects)
Server->>CRL_FS: 读取 CRL 文件 (.crl)
Server->>PKI: validate_der_certificate_revocation_list(path, der)
PKI->>PKI: rasn::der::decode CertificateList，检查无尾随字节
PKI->>Server: 验证结果 OK / Err
Server->>WebPKI: (在 OCSP/签名验证中) verify_signature(...)
WebPKI-->>Server: 签名验证结果
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 Possibly related PRs
诗
🚥 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 refactors rginx-http’s PKI/X.509 handling away from x509-parser toward rasn-based parsing and webpki signature verification (notably in the OCSP validation path), adds migration/baseline documentation for the staged refactor, and bumps the workspace/release version to v0.1.3-rc.8.
Changes:
- Replace
x509-parserusage in OCSP/certificate parsing paths withrasn-pkixdecoding andrustls-webpkisignature verification. - Introduce an internal
pkimodule to centralize certificate inspection, mTLS identity extraction, and CRL DER validation. - Add migration plan + stage-0 baseline docs; bump version references to
v0.1.3-rc.8.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates referenced current version/tag to v0.1.3-rc.8. |
| docs/x509-parser-to-rasn-plan.md | Adds detailed staged migration plan for replacing x509-parser with rasn/webpki. |
| docs/x509-parser-stage0-baseline.md | Adds stage-0 baseline behavior + required test gates for regression-proof refactor. |
| crates/rginx-http/src/tls/ocsp/mod.rs | Refactors OCSP parsing/verification to rasn + webpki; adds AIA responder URL test; removes x509-parser bridges. |
| crates/rginx-http/src/tls/certificates.rs | Moves CRL DER validation behind crate::pki API and adds CRL characterization tests. |
| crates/rginx-http/src/state/tls_runtime/mod.rs | Re-exports certificate inspection from crate::pki for tests. |
| crates/rginx-http/src/state/tls_runtime/certificates.rs | Switches certificate inspection to crate::pki::inspect_certificate and removes local implementation. |
| crates/rginx-http/src/state/mod.rs | Removes no-longer-needed x509-parser and related parsing imports after refactor. |
| crates/rginx-http/src/server/tests.rs | Adds characterization test ensuring mTLS identity extraction preserves chain order/leaf fields. |
| crates/rginx-http/src/server/connection.rs | Switches mTLS client identity parsing to crate::pki implementation. |
| crates/rginx-http/src/pki/mod.rs | Introduces internal PKI module entrypoints. |
| crates/rginx-http/src/pki/crl.rs | Adds rasn-based DER CRL validation with trailing-data rejection. |
| crates/rginx-http/src/pki/certificate.rs | Adds rasn-pkix based certificate inspection + mTLS identity parsing utilities. |
| crates/rginx-http/src/lib.rs | Registers the new internal pki module. |
| crates/rginx-http/Cargo.toml | Removes x509-parser, adds rustls-webpki, and drops rcgen’s x509-parser feature. |
| crates/rginx-app/tests/ocsp.rs | Updates test helper certificate parsing away from x509-parser. |
| crates/rginx-app/Cargo.toml | Removes x509-parser and rcgen feature usage. |
| Cargo.toml | Bumps workspace version and adds workspace webpki dependency. |
| Cargo.lock | Updates lockfile for version bump and dependency graph changes (webpki, untrusted, etc.). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/x509-parser-to-rasn-plan.md (1)
561-567: “当前建议”段落与文档状态存在时态冲突。Line 561–567 在“阶段 0-6 已完成”的前提下仍使用“如果现在开始执行”的表述，容易让读者误解迁移尚未落地。建议改成“复盘建议/可复用方法”。
✏️ 建议改写
-## 当前建议 +## 迁移后复盘建议 -如果现在开始执行，建议先落地两个最小动作： +若后续在其他子系统复用该迁移方法，建议优先落地两个最小动作：🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/x509-parser-to-rasn-plan.md` around lines 561 - 567, Update the "当前建议" section heading and its opening sentence to remove present-tense conditional phrasing; rename the heading to "复盘建议/可复用方法" (or similar) and change "如果现在开始执行，建议先落地两个最小动作：" to a retrospective phrasing such as "建议先落地两个最小动作：" or "复盘建议：先落地两个最小动作：" while keeping the two bullet items unchanged; locate the paragraph by the heading text "当前建议" in docs/x509-parser-to-rasn-plan.md and adjust the wording accordingly.crates/rginx-http/src/pki/crl.rs (1)
9-12: 错误信息与函数职责略有不一致。错误消息提到 "as PEM or DER CRL"，但此函数
validate_der_certificate_revocation_list仅处理 DER 格式。调用方validate_der_crl在 PEM 解析失败后才调用此函数，因此 "PEM or" 部分可能会误导用户。🔧 建议修正错误信息
let (_crl, remaining) = rasn::der::decode_with_remainder::<CertificateList>(der).map_err( |error| { Error::Server(format!( - "failed to parse certificate revocation list `{}` as PEM or DER CRL: {error}", + "failed to parse certificate revocation list `{}` as DER CRL: {error}", path.display() )) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/pki/crl.rs` around lines 9 - 12, The error message in validate_der_certificate_revocation_list is misleading because it mentions "as PEM or DER CRL" while the function only handles DER; update the Error::Server message in validate_der_certificate_revocation_list to state that parsing failed as a DER CRL (remove "PEM or"), keep including the path.display() and the {error} details so callers get the file path and underlying error, and ensure the change is made inside the validate_der_certificate_revocation_list function referenced in crl.rs.crates/rginx-http/src/tls/ocsp/mod.rs (1)
239-241: AIA 扩展提取逻辑在找到首个有效扩展后立即返回。当前实现在找到第一个包含 OCSP URL 的 AIA 扩展后就返回。如果证书包含多个 AIA 扩展（虽然不常见），后续扩展将被忽略。这可能是预期行为，但建议确认是否符合需求。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/tls/ocsp/mod.rs` around lines 239 - 241, 当前代码在遍历 AIA 扩展时遇到第一个包含 OCSP URL 的扩展就通过 if !urls.is_empty() { return urls; } 立即返回，导致证书中后续的 AIA 扩展被忽略。请修改遍历逻辑（查找产生 urls 的循环处，涉及变量 urls 和早期 return 的分支）以继续扫描所有 AIA 扩展并将所有发现的 OCSP URL 收集到 urls 中（可选去重），然后在循环结束后统一返回；如果确实只想要第一个有效 URL，添加明确注释或开关以表明该行为。
🤖 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/pki/certificate.rs`:
- Around line 137-140: Parsing failures for cert entries (decode_certificate
returning None) are causing later adjacency checks to use wrong indices because
chain_entries is compressed; update the logic so that when decode_certificate
fails for a chain element (the block that currently does let Some(cert) =
decode_certificate(...) else { chain_diagnostics.push(...); continue; }) you
also ensure no adjacency/neighbor checks run for that position — either by
preserving the original index mapping (store original_index alongside compressed
entries) or by gating all adjacency validation paths with a check that the
parsed cert (cert variable) exists/was successfully decoded before any neighbor
comparisons; apply the same fix for the other affected region (around the code
handling entries 212-242) and ensure chain_diagnostics uses the correct original
index in messages.
- Around line 145-147: The current days calculation using (not_after - now_secs)
/ 86_400 truncates toward zero and maps negative seconds like -1..-86399 to 0,
so expired certs may appear as 0 days remaining; change the computation in
expires_in_days (where
time_to_unix_secs(cert.tbs_certificate.validity.not_after) is mapped) to perform
floor division for negatives (e.g., compute seconds = not_after - now_secs and
then convert to days with proper rounding: if seconds >= 0 { seconds / 86_400 }
else { -(((-seconds) + 86_400 - 1) / 86_400) } or use a floating-point division
with .floor()) so that values like -1 second become -1 day; apply the same fix
to the analogous calculation in the 153-158 region.
- Around line 229-233: The current check uses chain_entries[index + 2..] to
count CA certs upstream of the current entry, which is reversed; change it to
count downstream CA certificates closer to the leaf by iterating over
chain_entries[..index] (i.e., entries before the current index) when computing
remaining_ca_certs used with path_len_constraint; update the slice used in the
iterator that computes remaining_ca_certs (the code around chain_entries, index,
path_len_constraint, and remaining_ca_certs) so the filter still checks
entry.is_ca == Some(true) but over the preceding entries instead of the
following ones.
---
Nitpick comments:
In `@crates/rginx-http/src/pki/crl.rs`:
- Around line 9-12: The error message in
validate_der_certificate_revocation_list is misleading because it mentions "as
PEM or DER CRL" while the function only handles DER; update the Error::Server
message in validate_der_certificate_revocation_list to state that parsing failed
as a DER CRL (remove "PEM or"), keep including the path.display() and the
{error} details so callers get the file path and underlying error, and ensure
the change is made inside the validate_der_certificate_revocation_list function
referenced in crl.rs.
In `@crates/rginx-http/src/tls/ocsp/mod.rs`:
- Around line 239-241: 当前代码在遍历 AIA 扩展时遇到第一个包含 OCSP URL 的扩展就通过 if
!urls.is_empty() { return urls; } 立即返回，导致证书中后续的 AIA 扩展被忽略。请修改遍历逻辑（查找产生 urls
的循环处，涉及变量 urls 和早期 return 的分支）以继续扫描所有 AIA 扩展并将所有发现的 OCSP URL 收集到 urls
中（可选去重），然后在循环结束后统一返回；如果确实只想要第一个有效 URL，添加明确注释或开关以表明该行为。
In `@docs/x509-parser-to-rasn-plan.md`:
- Around line 561-567: Update the "当前建议" section heading and its opening
sentence to remove present-tense conditional phrasing; rename the heading to
"复盘建议/可复用方法" (or similar) and change "如果现在开始执行，建议先落地两个最小动作：" to a retrospective
phrasing such as "建议先落地两个最小动作：" or "复盘建议：先落地两个最小动作：" while keeping the two
bullet items unchanged; locate the paragraph by the heading text "当前建议" in
docs/x509-parser-to-rasn-plan.md and adjust the wording accordingly.
🪄 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: 3250d722-d411-4b28-b544-43b71653dd55
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Cargo.tomlREADME.mdcrates/rginx-app/Cargo.tomlcrates/rginx-app/tests/ocsp.rscrates/rginx-http/Cargo.tomlcrates/rginx-http/src/lib.rscrates/rginx-http/src/pki/certificate.rscrates/rginx-http/src/pki/crl.rscrates/rginx-http/src/pki/mod.rscrates/rginx-http/src/server/connection.rscrates/rginx-http/src/server/tests.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/tls_runtime/certificates.rscrates/rginx-http/src/state/tls_runtime/mod.rscrates/rginx-http/src/tls/certificates.rscrates/rginx-http/src/tls/ocsp/mod.rsdocs/x509-parser-stage0-baseline.mddocs/x509-parser-to-rasn-plan.md
💤 Files with no reviewable changes (1)
- crates/rginx-http/src/state/mod.rs
There was a problem hiding this comment.
♻️ Duplicate comments (3)
crates/rginx-http/src/pki/certificate.rs (3)
227-233:⚠️ Potential issue | 🟠 Major
pathLenConstraint统计方向反了，应统计“下游(靠近 leaf) CA 数”Line 229 当前统计
index + 2..（上游/更靠近 root）会导致误报或漏报。对cert[index+1]应统计其下游 CA（..index+1）。建议修复
- if let Some(path_len_constraint) = chain_entries[index + 1].path_len_constraint { - let remaining_ca_certs = - chain_entries[index + 2..].iter().filter(|entry| entry.is_ca == Some(true)).count() - as u32; - if remaining_ca_certs > path_len_constraint { + if let Some(path_len_constraint) = chain_entries[index + 1].path_len_constraint { + let descendant_ca_certs = chain_entries[..index + 1] + .iter() + .filter(|entry| entry.is_ca == Some(true)) + .count() as u32; + if descendant_ca_certs > path_len_constraint { chain_diagnostics.push(format!( - "cert[{}] path_len_constraint_exceeded remaining_ca_certs={} path_len_constraint={}", + "cert[{}] path_len_constraint_exceeded descendant_ca_certs={} path_len_constraint={}", index + 1, - remaining_ca_certs, + descendant_ca_certs, path_len_constraint )); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/pki/certificate.rs` around lines 227 - 233, The path_len_constraint check is counting the wrong direction: change the CA count slice for cert at chain_entries[index + 1] from chain_entries[index + 2..] (upstream/root side) to the downstream/leaf side chain_entries[..index + 1], then recompute remaining_ca_certs as the number of entries with is_ca == Some(true) in that slice and compare it to path_len_constraint before pushing to chain_diagnostics; update the reference to chain_entries, path_len_constraint, remaining_ca_certs, index and chain_diagnostics accordingly.
144-145:⚠️ Potential issue | 🟠 Major
expires_in_days的负值取整方向错误，会漏报“已过期不足1天”Line 145 使用
/ 86_400会向 0 截断，-1..-86399秒会变成0，从而不触发 expired。应使用欧几里得除法。建议修复
- let expires_in_days = time_to_unix_secs(cert.tbs_certificate.validity.not_after) - .map(|not_after| (not_after - now_secs) / 86_400); + let expires_in_days = time_to_unix_secs(cert.tbs_certificate.validity.not_after) + .map(|not_after| (not_after - now_secs).div_euclid(86_400));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/pki/certificate.rs` around lines 144 - 145, expires_in_days 目前用 (not_after - now_secs) / 86_400 会对负数向 0 截断，导致 “已过期不足1天” 被误判为 0；在计算 expires_in_days 时把除法改为欧几里得除法（div_euclid）或等价的向下取整处理，例如对差值使用 .div_euclid(86_400)（确保差值是有符号整数类型），也就是修改计算在 time_to_unix_secs(cert.tbs_certificate.validity.not_after).map(...) 中将 `/ 86_400` 替换为 `.div_euclid(86_400)`（或等价实现），以正确处理负秒数边界。
136-140:⚠️ Potential issue | 🟠 Major解析失败后链索引映射已失真，且可能把中间证书当作 leaf 返回
Line 136-140 发生解析失败后
chain_entries被压缩，Line 210+ 的邻接校验与 Line 242+ 的链完整性/leaf 选择会基于错误位置运行；若cert[0]解析失败，当前实现还可能返回非叶子证书。建议在叶子不可解析时直接返回None，并在存在不可解析条目时跳过邻接校验。建议修复（最小改动）
@@ - for (index, der) in certs.iter().enumerate() { + let mut leaf_parsed = false; + for (index, der) in certs.iter().enumerate() { @@ let Some(cert) = decode_certificate(der.as_ref()) else { chain_diagnostics.push(format!("cert[{index}] could_not_be_parsed_as_x509")); continue; }; + if index == 0 { + leaf_parsed = true; + } @@ - for index in 0..chain_entries.len().saturating_sub(1) { - let issuer = chain_entries[index].issuer.as_deref(); - let next_subject = chain_entries[index + 1].subject.as_deref(); - if issuer != next_subject { - chain_diagnostics.push(format!( - "chain_link_mismatch cert[{index}]_issuer_to_cert[{}]_subject", - index + 1 - )); - } - if let (Some(aki), Some(ski)) = ( - chain_entries[index].authority_key_identifier.as_deref(), - chain_entries[index + 1].subject_key_identifier.as_deref(), - ) && aki != ski - { - chain_diagnostics - .push(format!("chain_aki_ski_mismatch cert[{index}]_to_cert[{}]", index + 1)); - } - ... - } + if chain_entries.len() == certs.len() { + for index in 0..chain_entries.len().saturating_sub(1) { + let issuer = chain_entries[index].issuer.as_deref(); + let next_subject = chain_entries[index + 1].subject.as_deref(); + if issuer != next_subject { + chain_diagnostics.push(format!( + "chain_link_mismatch cert[{index}]_issuer_to_cert[{}]_subject", + index + 1 + )); + } + if let (Some(aki), Some(ski)) = ( + chain_entries[index].authority_key_identifier.as_deref(), + chain_entries[index + 1].subject_key_identifier.as_deref(), + ) && aki != ski + { + chain_diagnostics.push(format!( + "chain_aki_ski_mismatch cert[{index}]_to_cert[{}]", + index + 1 + )); + } + // 其余链路邻接检查保持不变 + } + } else { + chain_diagnostics.push( + "chain_link_checks_skipped_due_to_unparseable_certificate".to_string(), + ); + } @@ - let leaf = chain_entries.into_iter().next()?; + if !leaf_parsed { + return None; + } + let leaf = chain_entries.into_iter().next()?;Also applies to: 210-226, 242-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/pki/certificate.rs` around lines 136 - 140, The parsing loop currently compresses chain_entries on decode_certificate failures which corrupts index mapping and can lead to returning a non-leaf as the leaf; change the logic so that if decode_certificate fails for cert[0] (the leaf) the function returns None immediately, and for other entries push the diagnostic but set a flag (e.g., skip_adjacency_checks) instead of removing entries; then guard the adjacency verification and chain integrity/leaf-selection code (the blocks that reference chain_entries later) to skip adjacency checks when that flag is set so index mapping and leaf selection remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/rginx-http/src/pki/certificate.rs`:
- Around line 227-233: The path_len_constraint check is counting the wrong
direction: change the CA count slice for cert at chain_entries[index + 1] from
chain_entries[index + 2..] (upstream/root side) to the downstream/leaf side
chain_entries[..index + 1], then recompute remaining_ca_certs as the number of
entries with is_ca == Some(true) in that slice and compare it to
path_len_constraint before pushing to chain_diagnostics; update the reference to
chain_entries, path_len_constraint, remaining_ca_certs, index and
chain_diagnostics accordingly.
- Around line 144-145: expires_in_days 目前用 (not_after - now_secs) / 86_400 会对负数向
0 截断，导致 “已过期不足1天” 被误判为 0；在计算 expires_in_days
时把除法改为欧几里得除法（div_euclid）或等价的向下取整处理，例如对差值使用
.div_euclid(86_400)（确保差值是有符号整数类型），也就是修改计算在
time_to_unix_secs(cert.tbs_certificate.validity.not_after).map(...) 中将 `/
86_400` 替换为 `.div_euclid(86_400)`（或等价实现），以正确处理负秒数边界。
- Around line 136-140: The parsing loop currently compresses chain_entries on
decode_certificate failures which corrupts index mapping and can lead to
returning a non-leaf as the leaf; change the logic so that if decode_certificate
fails for cert[0] (the leaf) the function returns None immediately, and for
other entries push the diagnostic but set a flag (e.g., skip_adjacency_checks)
instead of removing entries; then guard the adjacency verification and chain
integrity/leaf-selection code (the blocks that reference chain_entries later) to
skip adjacency checks when that flag is set so index mapping and leaf selection
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 29c3df85-b51e-47f7-b3f5-010c0c30dbaa
📒 Files selected for processing (6)
crates/rginx-app/tests/ocsp.rscrates/rginx-http/src/lib.rscrates/rginx-http/src/pki/certificate.rscrates/rginx-http/src/pki/crl.rscrates/rginx-http/src/server/tests.rscrates/rginx-http/src/tls/ocsp/mod.rs
✅ Files skipped from review due to trivial changes (2)
- crates/rginx-http/src/lib.rs
- crates/rginx-app/tests/ocsp.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/rginx-http/src/server/tests.rs
- crates/rginx-http/src/pki/crl.rs
- crates/rginx-http/src/tls/ocsp/mod.rs
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
Validation