feat(xmldsig): parse X509Certificate with x509-parser#53
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds strict DER parsing of ChangesX.509 Certificate Parsing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
There was a problem hiding this comment.
Pull request overview
This PR enhances XMLDSig <KeyInfo>/<X509Data> parsing by decoding <X509Certificate> DER bytes with x509-parser, extracting certificate metadata (DNs, SKI) and public key details while preserving metadata for unsupported key algorithms instead of failing the whole parse.
Changes:
- Add
ParsedX509CertificateandX509PublicKeyInfotypes and store parsed certificate metadata inX509DataInfo. - Parse
<X509Certificate>DER usingx509-parserand extract subject/issuer DN, SKI, and SPKI key info (RSA/EC/Unsupported). - Update tests to use a real PEM fixture certificate and add an invalid-DER regression test.
- parse <X509Certificate> DER with x509-parser and extract subject/issuer DN, SKI, and SPKI key metadata - keep unsupported SPKI algorithms as parsed metadata instead of hard parse failure - extend KeyInfo/X509Data tests to use real fixture cert DER and cover invalid DER rejection
- move add_x509_data_usage call before parse_x509_certificate in X509Certificate branch - reject oversized X509Data aggregate before expensive DER/X.509 parsing
polaz
force-pushed
the
feat/p2-003-x509-parser
branch
from
a591d4c to
89b6536
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 186-188: Add a clear doc comment documenting the parallel-array
invariant between the certificates and parsed_certificates fields: state that
both Vecs have equal length and that entries at the same index correspond to the
same certificate and its parsed representation. Put this comment near the struct
definition (above the struct or above both fields) referencing the field names
certificates and parsed_certificates and the ParsedX509Certificate type so
future maintainers know they must be updated together.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a3ab81db-0d88-42cc-bf29-948df54746ae
📒 Files selected for processing (1)
src/xmldsig/parse.rs
- document 1:1 index correspondence between certificates and parsed_certificates
- treat EC SPKI params that are present but non-OID as unsupported key algorithm - keep missing EC params as InvalidStructure - add regression test for unsupported SPKI algorithm marker using merlin x509 fixture
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 1383-1409: Update the test
parse_key_info_marks_unsupported_spki_algorithm_as_unsupported to assert the
preserved metadata on the unsupported public key instead of only checking the
Unsupported variant: after locating parsed_certificates[0] (from parse_key_info
/ KeyInfoSource::X509Data), assert that its X509PublicKeyInfo::Unsupported
contains the preserved fields (subject_dn, issuer_dn, and ski) and that they are
Some/contain the expected values (or non-empty) so the parser must retain those
values when returning Unsupported; adjust the match on
parsed_certificates[0].public_key to bind the Unsupported struct and add
assertions for subject_dn, issuer_dn, and ski.
- Around line 1370-1381: Add a regression test alongside
parse_key_info_rejects_x509_certificate_with_invalid_der that constructs a valid
DER X.509 fixture, base64-encodes it, appends a few extra bytes to the DER
payload (so X509Certificate::from_der returns Ok((_, rest)) with non-empty
rest), embeds that string into the same KeyInfo XML, calls
parse_key_info(doc.root_element()), and asserts it returns
Err(ParseError::InvalidStructure(_)); this ensures the parser branch in
parse_key_info that rejects non-empty rest after X509Certificate::from_der
cannot regress silently.
- Around line 219-224: The doc for X509PublicKeyInfo::Ec promises "uncompressed
point bytes" but code (parse.rs, X509PublicKeyInfo::Ec) currently accepts any
SEC1 encoding; either enforce the uncompressed form by validating the
public_key's first byte == 0x04 when constructing/parsing the Ec variant and
return an error if not, or change the field docstring to "encoded point bytes"
to reflect that compressed forms (0x02/0x03) are allowed; update the
constructor/parser that produces X509PublicKeyInfo::Ec and the doc comment on
the Ec variant accordingly so docs and behavior match.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cf663fe-b6b4-4240-9bc1-0fca6a672c52
📒 Files selected for processing (1)
src/xmldsig/parse.rs
| /// EC public key (`curve_oid`, uncompressed point bytes). | ||
| Ec { | ||
| /// Named-curve OID from SubjectPublicKeyInfo parameters. | ||
| curve_oid: String, | ||
| /// Raw EC point bytes from SubjectPublicKeyInfo. | ||
| public_key: Vec<u8>, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In RFC 5480 and x509-parser 0.18.0, is the EC SubjectPublicKeyInfo point guaranteed to be uncompressed, or can it be encoded in compressed SEC1 form as well?
💡 Result:
In RFC 5480, the EC SubjectPublicKeyInfo point is not guaranteed to be uncompressed. Implementations MUST support the uncompressed form (starting with 0x04) and MAY support the compressed form (starting with 0x02 or 0x03) as defined in SEC1 [1][2][3]. The public key MUST be rejected if the first octet is any other value [1]. In x509-parser 0.18.0, the EC point is parsed as an ECPoint struct containing the raw octet string data from the BIT STRING in SubjectPublicKeyInfo [4][5][6]. The ECPoint constructor takes the raw bytes (ECPoint::from(b) where b is the bit string data) without validation of the format, so it accepts both uncompressed and compressed forms, as well as invalid first octets (though RFC requires rejection of invalid ones, the parser does not enforce this) [4][6]. The library is designed for standards-compliant X.509 parsing per RFC 5280 and references RFC 5480 for EC points [5].
Citations:
- 1: https://rfc-editor.org/rfc/rfc5480
- 2: https://www.rfc-editor.org/rfc/rfc5480.html
- 3: https://datatracker.ietf.org/doc/html/rfc5480
- 4: https://docs.rs/x509-parser/0.13.2/x509_parser/public_key/struct.ECPoint.html
- 5: https://github.com/rusticata/x509-parser
- 6: https://docs.rs/x509-parser/0.13.2/src/x509_parser/x509.rs.html
Don't promise uncompressed EC point bytes without enforcing it.
X509PublicKeyInfo::Ec documents "uncompressed point bytes," but RFC 5480 permits both uncompressed (0x04) and compressed (0x02/0x03) SEC1 encodings. The x509-parser library accepts both forms without validation. Since the code passes through whatever encoding is present, either validate the 0x04 uncompressed prefix before constructing the struct, or relax the docs to "encoded point bytes" to match the actual behavior.
Suggested doc-only fix
- /// EC public key (`curve_oid`, uncompressed point bytes).
+ /// EC public key (`curve_oid`, encoded point bytes).
Ec {
/// Named-curve OID from SubjectPublicKeyInfo parameters.
curve_oid: String,
- /// Raw EC point bytes from SubjectPublicKeyInfo.
+ /// Encoded EC point bytes from SubjectPublicKeyInfo.
public_key: Vec<u8>,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// EC public key (`curve_oid`, uncompressed point bytes). | |
| Ec { | |
| /// Named-curve OID from SubjectPublicKeyInfo parameters. | |
| curve_oid: String, | |
| /// Raw EC point bytes from SubjectPublicKeyInfo. | |
| public_key: Vec<u8>, | |
| /// EC public key (`curve_oid`, encoded point bytes). | |
| Ec { | |
| /// Named-curve OID from SubjectPublicKeyInfo parameters. | |
| curve_oid: String, | |
| /// Encoded EC point bytes from SubjectPublicKeyInfo. | |
| public_key: Vec<u8>, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/xmldsig/parse.rs` around lines 219 - 224, The doc for
X509PublicKeyInfo::Ec promises "uncompressed point bytes" but code (parse.rs,
X509PublicKeyInfo::Ec) currently accepts any SEC1 encoding; either enforce the
uncompressed form by validating the public_key's first byte == 0x04 when
constructing/parsing the Ec variant and return an error if not, or change the
field docstring to "encoded point bytes" to reflect that compressed forms
(0x02/0x03) are allowed; update the constructor/parser that produces
X509PublicKeyInfo::Ec and the doc comment on the Ec variant accordingly so docs
and behavior match.
| #[test] | ||
| fn parse_key_info_rejects_x509_certificate_with_invalid_der() { | ||
| let xml = r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#"> | ||
| <X509Data> | ||
| <X509Certificate>AQID</X509Certificate> | ||
| </X509Data> | ||
| </KeyInfo>"#; | ||
| let doc = Document::parse(xml).unwrap(); | ||
|
|
||
| let err = parse_key_info(doc.root_element()).unwrap_err(); | ||
| assert!(matches!(err, ParseError::InvalidStructure(_))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a regression case for valid DER plus trailing bytes.
The new parser explicitly rejects non-empty rest after X509Certificate::from_der, but this test only covers completely invalid DER (AQID). Please add a case that appends a few bytes to a real fixture certificate and asserts ParseError::InvalidStructure, so the branch at Lines 689-692 cannot regress silently.
Example test shape
+ #[test]
+ fn parse_key_info_rejects_x509_certificate_with_trailing_der_bytes() {
+ let mut cert = base64::engine::general_purpose::STANDARD
+ .decode(fixture_rsa_cert_base64())
+ .expect("fixture PEM must contain valid base64");
+ cert.extend_from_slice(&[0x00, 0x01]);
+ let cert_base64 = base64::engine::general_purpose::STANDARD.encode(cert);
+ let xml = format!(
+ r#"<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
+ <X509Data>
+ <X509Certificate>{cert_base64}</X509Certificate>
+ </X509Data>
+ </KeyInfo>"#
+ );
+ let doc = Document::parse(&xml).unwrap();
+
+ let err = parse_key_info(doc.root_element()).unwrap_err();
+ assert!(matches!(err, ParseError::InvalidStructure(_)));
+ }As per coding guidelines, "Corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/xmldsig/parse.rs` around lines 1370 - 1381, Add a regression test
alongside parse_key_info_rejects_x509_certificate_with_invalid_der that
constructs a valid DER X.509 fixture, base64-encodes it, appends a few extra
bytes to the DER payload (so X509Certificate::from_der returns Ok((_, rest))
with non-empty rest), embeds that string into the same KeyInfo XML, calls
parse_key_info(doc.root_element()), and asserts it returns
Err(ParseError::InvalidStructure(_)); this ensures the parser branch in
parse_key_info that rejects non-empty rest after X509Certificate::from_der
cannot regress silently.
| #[test] | ||
| fn parse_key_info_marks_unsupported_spki_algorithm_as_unsupported() { | ||
| let xml = include_str!( | ||
| "../../tests/fixtures/xmldsig/merlin-xmldsig-twenty-three/signature-x509-crt.xml" | ||
| ); | ||
| let doc = Document::parse(xml).unwrap(); | ||
| let key_info_node = doc | ||
| .descendants() | ||
| .find(|node| { | ||
| node.is_element() | ||
| && node.tag_name().namespace() == Some(XMLDSIG_NS) | ||
| && node.tag_name().name() == "KeyInfo" | ||
| }) | ||
| .expect("fixture must contain ds:KeyInfo"); | ||
|
|
||
| let key_info = parse_key_info(key_info_node).expect("KeyInfo parse should succeed"); | ||
| let x509_info = match &key_info.sources[0] { | ||
| KeyInfoSource::X509Data(x509) => x509, | ||
| other => panic!("expected X509Data source, got {other:?}"), | ||
| }; | ||
| assert_eq!(x509_info.certificates.len(), 1); | ||
| assert_eq!(x509_info.parsed_certificates.len(), 1); | ||
| assert!(matches!( | ||
| x509_info.parsed_certificates[0].public_key, | ||
| X509PublicKeyInfo::Unsupported { .. } | ||
| )); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Assert the preserved metadata, not just the unsupported marker.
This regression currently passes even if the parser starts dropping subject_dn, issuer_dn, or SKI while still returning X509PublicKeyInfo::Unsupported. Since the feature here is “preserve parsed metadata for unsupported key algorithms,” add assertions that those fields remain populated as well.
Minimal assertion extension
assert_eq!(x509_info.certificates.len(), 1);
assert_eq!(x509_info.parsed_certificates.len(), 1);
+ assert!(!x509_info.parsed_certificates[0].subject_dn.is_empty());
+ assert!(!x509_info.parsed_certificates[0].issuer_dn.is_empty());
assert!(matches!(
x509_info.parsed_certificates[0].public_key,
X509PublicKeyInfo::Unsupported { .. }
));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/xmldsig/parse.rs` around lines 1383 - 1409, Update the test
parse_key_info_marks_unsupported_spki_algorithm_as_unsupported to assert the
preserved metadata on the unsupported public key instead of only checking the
Unsupported variant: after locating parsed_certificates[0] (from parse_key_info
/ KeyInfoSource::X509Data), assert that its X509PublicKeyInfo::Unsupported
contains the preserved fields (subject_dn, issuer_dn, and ski) and that they are
Some/contain the expected values (or non-empty) so the parser must retain those
values when returning Unsupported; adjust the match on
parsed_certificates[0].public_key to bind the Unsupported struct and add
assertions for subject_dn, issuer_dn, and ski.
Summary
Verification