feat(xmldsig): add ecdsa signature verification#27
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdd ECDSA (P-256/P-384) XMLDSig verification entry points (PEM/SPKI), EC SPKI parsing and SEC1 point validation, signature-format classification (fixed r||s vs ASN.1 DER with ambiguous handling), new typed errors, RSA PEM refactor, and integration tests. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant VerifyFunc as verify_ecdsa_signature_pem/spki
participant KeyParse as PublicKeyParser
participant SigFormat as SignatureClassifier
participant RingLib as ring::signature
Caller->>VerifyFunc: (algorithm, public_key, signed_data, signature_value)
VerifyFunc->>KeyParse: parse PEM or SPKI → extract EC public key (uncompressed SEC1)
KeyParse-->>VerifyFunc: EC public key or Err
alt key parse failed
VerifyFunc-->>Caller: Err(InvalidKeyDer / InvalidKeyPem / KeyAlgorithmMismatch)
else key parsed
VerifyFunc->>SigFormat: inspect signature bytes (fixed r||s / ASN.1 DER / Ambiguous / Invalid)
SigFormat-->>VerifyFunc: classification
alt Ambiguous
VerifyFunc->>RingLib: verify as DER
RingLib-->>VerifyFunc: Ok(true/false)
alt failed
VerifyFunc->>RingLib: verify as raw r||s
RingLib-->>VerifyFunc: Ok(true/false)
end
else valid encoding
VerifyFunc->>RingLib: verify with curve-specific algorithm (P-256/P-384)
RingLib-->>VerifyFunc: Ok(true/false) or Err(...)
end
VerifyFunc-->>Caller: Ok(bool) or Err(InvalidSignatureFormat / KeyAlgorithmMismatch / ...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
polaz
force-pushed
the
feat/#26-ecdsa-signature-verification
branch
from
add7768 to
13fa548
Compare
There was a problem hiding this comment.
Pull request overview
Adds XMLDSig ECDSA signature verification support alongside the existing RSA verification helpers, expanding xmldsig to validate ecdsa-sha256 and ecdsa-sha384 signatures (including an ASN.1/DER fallback path for interop).
Changes:
- Implement ECDSA verification helpers (
verify_ecdsa_signature_pem/verify_ecdsa_signature_spki) for P-256/SHA-256 and P-384/SHA-384, with fixed-widthr||sand DER signature handling. - Re-export the new ECDSA verification API from
xml_sec::xmldsig. - Add ECDSA integration tests covering donor P-256 verification, local P-384 signing+verification, and multiple typed failure paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/xmldsig/signature.rs |
Adds ECDSA verification implementation and signature-format handling (fixed-width vs DER). |
src/xmldsig/mod.rs |
Re-exports new ECDSA verification helpers from the xmldsig module. |
tests/ecdsa_signature_integration.rs |
New integration tests for ECDSA verification (positive vectors + failure-path assertions). |
![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
🤖 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/signature.rs`:
- Around line 74-97: The error text and docs for key parse failures are
RSA-specific but are used by the new ECDSA entry points (e.g.,
verify_ecdsa_signature_pem and verify_ecdsa_signature_spki); update the error
types and messages to avoid misreporting EC keys by either (a)
renaming/generifying SignatureVerificationError::InvalidKeyDer and its displayed
message/docs to a neutral phrase like "invalid SubjectPublicKeyInfo DER" or (b)
splitting it into two variants (e.g., InvalidRsaSpkiDer and InvalidEcSpkiDer)
and return the appropriate one from the ECDSA paths; make the same change for
the shared error docs referenced by the RSA code so ECDSA failures produce
correct key-family text.
- Around line 264-267: The current selection logic in
uses_xmlsig_fixed_signature (and its callers around the block returning
fixed_algorithm vs asn1_algorithm) treats inputs that both look like DER
(looks_like_der_sequence) and have the same total length as raw r||s as
unambiguous raw signatures, causing valid DER ECDSA signatures to be misrouted;
update uses_xmlsig_fixed_signature to fully parse the ASN.1 DER ECDSA structure
(validate SEQUENCE, INTEGER tags and lengths and exact r/s lengths) and treat
any well-formed DER as ASN.1, or if you cannot fully parse, treat the case where
looks_like_der_sequence == true and total length == raw_width as ambiguous and
attempt verification with the ASN.1 verifier before returning false; apply the
same change to the related selection logic in the 283-327 region so malformed
DER blobs no longer bypass InvalidSignatureFormat and DER-encoded signatures are
tried first.
In `@tests/ecdsa_signature_integration.rs`:
- Around line 83-121: Add two new tests in tests/ecdsa_signature_integration.rs:
one that verifies the DER-encoded ECDSA signature path succeeds and one that
asserts a malformed-DER signature returns InvalidSignatureFormat. Reuse the
existing helpers (read_fixture, canonicalized_signed_info_and_signature) to
obtain canonical_signed_info, load a DER-encoded signature fixture (instead of
the fixed-width r||s) and call verify_ecdsa_signature_pem with
SignatureAlgorithm::EcdsaP384Sha384 and the matching public_key_pem to assert
Ok(true). For the failure case, tamper a byte in the DER signature fixture (or
use a deliberately truncated DER blob) and assert the verifier returns the
InvalidSignatureFormat error variant (matching the error type produced by
verify_ecdsa_signature_pem). Ensure tests reference the same key/algorithm pair
as local_p384_signature_matches so they exercise the ASN.1/DER fallback path.
🪄 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: 3d52dba2-2aa8-4699-9aa7-75cb25b899b1
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/signature.rstests/ecdsa_signature_integration.rs
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/xmldsig/signature.rs (1)
325-329:⚠️ Potential issue | 🔴 CriticalKeep
0x30-prefixed signatures ambiguous until raw is ruled out.A valid XMLDSig raw
r || ssignature can legitimately begin with0x30. Here that prefix commits the input to the DER parser, so any fixed-width signature that starts with0x30but is not also well-formed ASN.1 is rejected asInvalidSignatureFormatbefore the raw verifier runs. Please only surface a format error after both encodings are excluded, or retry the fixed-width path for same-width inputs.Also applies to: 343-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/signature.rs` around lines 325 - 329, Do not let ASN.1 parse errors abort detection when a same-length raw XMLDSig signature is possible: change the call to inspect_der_encoded_ecdsa_signature(signature_value, component_len) so you do not use the ? operator to propagate parse errors; instead match on the Result and handle Err(_) as "not DER" and fall through to the fixed-width check. Concretely: if inspect_der returns Ok(Some(())) return EcdsaSignatureEncoding::Asn1Der; if it returns Ok(None) or Err(_) and signature_value.len() == expected_len return EcdsaSignatureEncoding::XmlDsigFixed; otherwise return SignatureVerificationError::InvalidSignatureFormat. Apply the same change to the other check that uses inspect_der_encoded_ecdsa_signature (the block around lines 343-348).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ecdsa_signature_integration.rs`:
- Around line 16-18: Update the test import to use the public re-export in
xml_sec::xmldsig instead of the internal xmldsig::signature module: replace the
current use of SignatureVerificationError, verify_ecdsa_signature_pem, and
verify_ecdsa_signature_spki from xml_sec::xmldsig::signature with a direct
import from xml_sec::xmldsig so the new verification API re-export is exercised
(i.e., import SignatureVerificationError, verify_ecdsa_signature_pem,
verify_ecdsa_signature_spki from xml_sec::xmldsig).
---
Duplicate comments:
In `@src/xmldsig/signature.rs`:
- Around line 325-329: Do not let ASN.1 parse errors abort detection when a
same-length raw XMLDSig signature is possible: change the call to
inspect_der_encoded_ecdsa_signature(signature_value, component_len) so you do
not use the ? operator to propagate parse errors; instead match on the Result
and handle Err(_) as "not DER" and fall through to the fixed-width check.
Concretely: if inspect_der returns Ok(Some(())) return
EcdsaSignatureEncoding::Asn1Der; if it returns Ok(None) or Err(_) and
signature_value.len() == expected_len return
EcdsaSignatureEncoding::XmlDsigFixed; otherwise return
SignatureVerificationError::InvalidSignatureFormat. Apply the same change to the
other check that uses inspect_der_encoded_ecdsa_signature (the block around
lines 343-348).
🪄 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: 88acc96b-edea-41ef-bf90-c8eacd1eca59
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/signature.rstests/ecdsa_signature_integration.rs
|
@coderabbitai The duplicate review-body item about ambiguity is fixed in commit : same-width signatures now fall back to raw when DER parse fails, and a regression test () was added to lock this behavior. |
|
@coderabbitai The duplicate review-body item about 0x30 ambiguity is fixed in commit 9158892: same-width signatures now fall back to raw r||s when DER parse fails, and a regression test (local_p384_raw_signature_with_der_like_prefix_matches) was added to lock this behavior. |
|
Tip For best results, initiate chat on the files or code changes.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5258
What changed in The old approach hard-stopped on any DER-parse error when the signature started with Ok(None) | Err(_) if signature_value.len() == expected_len => Ok(EcdsaSignatureEncoding::XmlDsigFixed)
Ok(None) | Err(_) => Err(InvalidSignatureFormat)…correctly treats a DER-parse failure as a fallback signal: if the length matches the expected fixed-width, it's raw Regression test is sound: Negative test is properly tightened: renaming to The fix accurately resolves the 0x30 ambiguity concern. ✅ |
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/xmldsig/signature.rs (1)
167-175:⚠️ Potential issue | 🟠 MajorRetry the fixed-width verifier for same-width inputs that also parse as DER.
Line 325 currently commits any same-width
r || sblob to the ASN.1 verifier as soon as it is structurally valid DER. That still leaves a false-negative class: XMLDSig raw signatures are just fixed-width bytes, so a valid raw signature can also happen to parse asSEQUENCE(INTEGER, INTEGER). In that case this code returnsOk(false)without ever trying the fixed-width algorithm. The ambiguity needs to be resolved by verification, not by classification alone: forsignature_value.len() == component_len * 2, try ASN.1 first and then fall back to raw before returningfalse.As per coding guidelines,
**/*.rs: Tier 1 - Flag logic bugs: data corruption (wrong algorithm, incorrect ordering, dropped or duplicated data).Also applies to: 305-308, 317-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/signature.rs` around lines 167 - 175, The EC verification path (PublicKey::EC) currently classifies same-width blobs that parse as DER and returns false without trying the fixed-width verifier; change the logic in the block that constructs verification_algorithm via ecdsa_verification_algorithm and the UnparsedPublicKey::new + key.verify(signed_data, signature_value) call so that if signature_value.len() == component_len * 2 you attempt ASN.1 verification first and if it fails fall back to the raw fixed-width verifier (i.e., convert the blob into r||s components and call the fixed-width ECDSA verifier) before returning Ok(false); ensure this retry covers the same-width branches mentioned (around the code paths for component_len checks and the verify() invocation).
🤖 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/signature.rs`:
- Around line 259-277: The code currently returns
SignatureVerificationError::InvalidKeyDer for EC keys that were successfully
parsed but don't match the supported curve/point length; change the branches
that now return InvalidKeyDer to return
SignatureVerificationError::KeyAlgorithmMismatch (or a new unsupported-curve
error) when SubjectPublicKeyInfo::parsed() produced PublicKey::EC(_) but
curve_oid/point_len do not match the expected pairs; keep InvalidKeyDer reserved
only for actual parse/structure failures, and update the match arms handling
SignatureAlgorithm::EcdsaP256Sha256 (and the similar EcdsaP384Sha384 arm) to
return KeyAlgorithmMismatch using algorithm.uri().to_string() in those fallback
cases.
- Around line 408-437: parse_der_length currently accepts long-form length
encodings for values < 128 (BER overlong), so update parse_der_length to enforce
DER-minimal encoding: when the high-bit of the first length byte indicates
long-form, after decoding declared_len (and after the existing leading-zero
check) return Err if declared_len < 128 (i.e., require long-form only for
lengths >= 128); keep the existing checks for len_len==0, len_len >
size_of::<usize>(), rest.len() < len_len, and leading zero in len_bytes. This
change in parse_der_length will cause the sequence and integer parsers that call
it to reject BER overlong encodings as invalid DER.
---
Duplicate comments:
In `@src/xmldsig/signature.rs`:
- Around line 167-175: The EC verification path (PublicKey::EC) currently
classifies same-width blobs that parse as DER and returns false without trying
the fixed-width verifier; change the logic in the block that constructs
verification_algorithm via ecdsa_verification_algorithm and the
UnparsedPublicKey::new + key.verify(signed_data, signature_value) call so that
if signature_value.len() == component_len * 2 you attempt ASN.1 verification
first and if it fails fall back to the raw fixed-width verifier (i.e., convert
the blob into r||s components and call the fixed-width ECDSA verifier) before
returning Ok(false); ensure this retry covers the same-width branches mentioned
(around the code paths for component_len checks and the verify() invocation).
🪄 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: 57ae5d7e-674d-44a2-9041-b5fda59f4cb9
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/signature.rstests/ecdsa_signature_integration.rs
- validate EC SPKI point encoding before ring verification - reject DER overlong lengths and keep 0x30 raw signatures deterministic - add regression coverage for invalid EC point prefix and DER classifier edge cases
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/signature.rs (1)
27-32:⚠️ Potential issue | 🟡 MinorGeneralize the
UnsupportedAlgorithmdocs.This variant now comes out of
verify_ecdsa_signature_pem()andverify_ecdsa_signature_spki()too, so the RSA-only description is stale for the shared public error type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/signature.rs` around lines 27 - 32, The enum variant UnsupportedAlgorithm is documented as RSA-specific but is used by verify_ecdsa_signature_pem() and verify_ecdsa_signature_spki(); update its doc comment and error text to be algorithm-agnostic. Replace the docstring "The signature method is not an RSA PKCS#1 v1.5 algorithm." with a generic description such as "The signature method is not a supported algorithm." (keep the diagnostic field uri and the #[error("unsupported signature algorithm: {uri}")] message or change it to similarly generic text) so the documentation and error semantics match its usage across verify_ecdsa_signature_pem, verify_ecdsa_signature_spki, and other callers.
🤖 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/signature.rs`:
- Around line 91-107: Extract the duplicated PEM parsing/validation in
verify_ecdsa_signature_pem and verify_rsa_signature_pem into a small private
helper (e.g., fn parse_public_key_pem(pem_str: &str) -> Result<Vec<u8>,
SignatureVerificationError>) that: calls x509_parser::pem::parse_x509_pem,
checks trailing bytes are all ASCII whitespace, verifies pem.label == "PUBLIC
KEY" and returns pem.contents on success or the appropriate
SignatureVerificationError on failure; then replace the parsing block in
verify_ecdsa_signature_pem and verify_rsa_signature_pem to call this helper and
pass the returned contents to verify_ecdsa_signature_spki /
verify_rsa_signature_spki respectively.
- Around line 398-418: parse_der_integer() is enforcing ECDSA scalar rules (the
checks on magnitude.is_empty() and magnitude.len() > component_len) which causes
valid DER-encoded integers (e.g., 0) to be rejected; remove those two
scalar-validity checks and only perform DER-encoding checks: reject empty
integer_bytes, reject integer_bytes.len() > component_len + 1, reject high-bit
set on first octet, and reject unnecessary leading zero (integer_bytes.len() > 1
&& integer_bytes[0] == 0 && integer_bytes[1] & 0x80 == 0); keep the logic that
strips a single leading zero into magnitude (the integer_bytes[0] == 0 branch)
but do not validate magnitude length or emptiness against component_len in
parse_der_integer so encoding classification stays separate from ECDSA scalar
validation.
---
Outside diff comments:
In `@src/xmldsig/signature.rs`:
- Around line 27-32: The enum variant UnsupportedAlgorithm is documented as
RSA-specific but is used by verify_ecdsa_signature_pem() and
verify_ecdsa_signature_spki(); update its doc comment and error text to be
algorithm-agnostic. Replace the docstring "The signature method is not an RSA
PKCS#1 v1.5 algorithm." with a generic description such as "The signature method
is not a supported algorithm." (keep the diagnostic field uri and the
#[error("unsupported signature algorithm: {uri}")] message or change it to
similarly generic text) so the documentation and error semantics match its usage
across verify_ecdsa_signature_pem, verify_ecdsa_signature_spki, and other
callers.
🪄 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: 7cca9af0-24d4-41a8-a091-3ee7b44d7407
📒 Files selected for processing (1)
src/xmldsig/signature.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/xmldsig/signature.rs (1)
398-418:⚠️ Potential issue | 🟡 MinorKeep DER classification separate from ECDSA scalar validity.
parse_der_integer()still rejects well-formed DER like30 06 02 01 00 02 01 01becausemagnitude.is_empty()/magnitude.len() > component_lenare value checks, not encoding checks. That makes syntactically valid ASN.1 inputs returnInvalidSignatureFormat(or fall back to raw on same-width inputs) instead of reaching the ASN.1 verifier and returningOk(false).♻️ Minimal fix
- let magnitude = if integer_bytes[0] == 0 { - &integer_bytes[1..] - } else { - integer_bytes - }; - if magnitude.is_empty() || magnitude.len() > component_len { - return Err(SignatureVerificationError::InvalidSignatureFormat); - } - Ok(remainder)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/signature.rs` around lines 398 - 418, parse_der_integer currently mixes DER encoding validation with ECDSA scalar value checks: remove the value-based rejections so syntactically valid DER (e.g. zero-length magnitude encoded as 0x00) is not rejected by parse_der_integer; instead only enforce DER/ASN.1 encoding rules (empty integer_bytes, excessive total length, leading-sign-bit rules, unnecessary leading zero) inside parse_der_integer and stop short of rejecting based on magnitude.is_empty() or magnitude.len() > component_len — move those magnitude size/value checks into the ECDSA-specific verifier that calls parse_der_integer (so the verifier can return Ok(false for invalid scalars), while parse_der_integer returns the raw magnitude/encoding result for further validation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ecdsa_signature_integration.rs`:
- Around line 266-280: Add a positive test case that calls
verify_ecdsa_signature_spki with a real SPKI DER fixture and asserts Ok(())
instead of expect_err: locate the test function
malformed_spki_der_returns_typed_error and add a new assertion mirroring an
existing PEM happy-path test by loading the same fixture key's DER bytes, the
matching SignatureAlgorithm (e.g., SignatureAlgorithm::EcdsaP256Sha256), the
same payload and signature bytes, and assert the call returns Ok(()) (repeat
this pattern for the other failing-test blocks around the ranges mentioned:
283-318 and 363-380) to ensure the SPKI verifier accepts valid DER keys.
---
Duplicate comments:
In `@src/xmldsig/signature.rs`:
- Around line 398-418: parse_der_integer currently mixes DER encoding validation
with ECDSA scalar value checks: remove the value-based rejections so
syntactically valid DER (e.g. zero-length magnitude encoded as 0x00) is not
rejected by parse_der_integer; instead only enforce DER/ASN.1 encoding rules
(empty integer_bytes, excessive total length, leading-sign-bit rules,
unnecessary leading zero) inside parse_der_integer and stop short of rejecting
based on magnitude.is_empty() or magnitude.len() > component_len — move those
magnitude size/value checks into the ECDSA-specific verifier that calls
parse_der_integer (so the verifier can return Ok(false for invalid scalars),
while parse_der_integer returns the raw magnitude/encoding result for further
validation).
🪄 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: dbba54de-5231-468a-a3a5-14e4dad506e7
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/signature.rstests/ecdsa_signature_integration.rs
- deduplicate PUBLIC KEY PEM parsing in a shared helper - keep DER classifier focused on encoding, not scalar validity - add positive verify_ecdsa_signature_spki fixture assertion
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
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/signature.rs`:
- Around line 45-50: The code currently maps any non-EC public key that
successfully parses to InvalidKeyDer; instead, in verify_ecdsa_signature_pem and
verify_ecdsa_signature_spki, after SubjectPublicKeyInfo::from_der() and
spki.parsed() succeed, detect the parsed algorithm OID/URI and return
KeyAlgorithmMismatch { uri } when the key is not an EC key (rather than treating
it as a DER error). Locate the error mapping around spki.parsed() and replace
the wildcard/InvalidKeyDer branch with logic that extracts the algorithm URI
(from spki.algorithm or parsed metadata) and constructs KeyAlgorithmMismatch
with that URI; apply the same change to the other analogous block in the 174-200
range so that parsed non-EC keys are reported as KeyAlgorithmMismatch.
🪄 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: 5e4d4201-5b84-4a5f-b425-d4087de344cd
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/signature.rstests/ecdsa_signature_integration.rs
- Reject DER INTEGER values longer than component width unless they use a required leading 0x00 sign byte - Report parsed non-EC SPKI keys in ECDSA verification as KeyAlgorithmMismatch instead of InvalidKeyDer - Add regression coverage for both cases
- Return UnsupportedAlgorithm from verify_ecdsa_signature_spki before SPKI parsing for non-ECDSA signature methods - Add regression coverage to ensure non-ECDSA algorithms are rejected consistently regardless of key bytes
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
Summary
ecdsa-sha256andecdsa-sha384xmldsigWhy
P1-020was the next real unfinished implementation item after confirmingP1-019ais already present onmainenveloped-sha384-ecdsa-sha384.xmlfixture is not a usable P-384 donor vector as-is, so the PR documents that drift and covers P-384 with vendored local keys insteadNotes
r||ssignatures for P-256/P-384aleksey-xmldsig-01/enveloped-sha384-ecdsa-sha384.xmlfixture carriesKeyName=TestKeyName-ec-prime521v1and a 132-byte raw signature, so it is not treated as a valid P-384 donor verification vector in this PRTesting
Closes #26