feat(xmldsig): compute signing reference digests#79
Conversation
- Add signing-template digest computation and fill APIs - Preserve strict verification parsing for populated DigestValue - Cover same-document, multi-reference, enveloped, and SHA-1 rejection cases - Update p384 to stable 0.14 Closes #78
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds XMLDSig signing-side reference digest computation, template signing with RSA/ECDSA keys, public API exposure, and tests. The README and Cargo feature set are updated to reflect the signing pipeline and its optional dependency. ChangesSigning template pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Filename | Overview |
|---|---|
| Cargo.toml | Adds crypto-common and updates XMLDSig crypto feature wiring. |
| src/xmldsig/sign.rs | Adds signing digest computation, full template signing, and RSA/ECDSA PKCS#8 signing key adapters. |
| tests/signing_digest.rs | Adds coverage for digest filling, signing policy rejection, and RSA/ECDSA signing round trips. |
| src/xmldsig/mod.rs | Exports the new XMLDSig signing APIs. |
| README.md | Documents the new template signing and signing key support. |
Reviews (3): Last reviewed commit: "fix(xmldsig): harden signing key operati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add SignContext for template and builder-based signing - Add RSA PKCS1v15 and ECDSA P-256/P-384 signing keys - Fill SignatureValue after SignedInfo canonicalization - Verify RSA-SHA256 and ECDSA-P256 signing round trips
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/sign.rs`:
- Around line 178-192: The RSA signing paths in SignatureAlgorithm::RsaSha256,
SignatureAlgorithm::RsaSha384, and SignatureAlgorithm::RsaSha512 currently use
the non-randomized pkcs1v15::SigningKey::sign flow, which may skip blinding on
the private-key operation. Update the signing logic in src/xmldsig/sign.rs to
use the randomized signing API (RandomizedSigner::try_sign_with_rng with a
secure RNG) for the RSA variants, and verify that this still preserves the
expected PKCS#1 v1.5 signature output interoperability.
- Around line 177-197: The signing flow in `Signer::sign` currently uses the
infallible `sign` method for RSA, which can panic on real failures; update the
`SignatureAlgorithm::RsaSha256`, `RsaSha384`, and `RsaSha512` branches to use
`try_sign` and propagate errors. Add a `SigningFailed` variant to
`SigningKeyError` so failures are returned instead of unwrapping, and apply the
same `try_sign`-based error handling in the ECDSA signing paths for consistency.
In `@tests/signing_digest.rs`:
- Around line 175-203: Add a round-trip test for the publicly exported
EcdsaP384SigningKey to close the coverage gap: mirror the existing
signs_ecdsa_p256_template_and_verifies_round_trip test, but use the P-384 key
fixtures, SignatureAlgorithm::EcdsaP384Sha384, and the same
SignContext::new/sign_with_builder plus verify_signature_with_pem_key flow. Keep
the assertions consistent so the new test validates that the P-384 signing and
verification pipeline works end to end.
🪄 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 Plus
Run ID: 7064a3a1-ff6f-403c-a110-3b9d1b0eeadd
📒 Files selected for processing (4)
Cargo.tomlsrc/xmldsig/mod.rssrc/xmldsig/sign.rstests/signing_digest.rs
- Use randomized RSA PKCS1v15 signing with SysRng - Propagate signing failures instead of using infallible sign - Add P-384 signing round-trip coverage - Update README signing status
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Cargo.toml`:
- Line 34: The xmldsig dependency on crypto-common is missing the getrandom
feature, which prevents crypto_common::getrandom::SysRng from compiling in
src/xmldsig/sign.rs. Update the crypto-common entry in Cargo.toml to enable the
getrandom feature alongside dep:crypto-common so the SysRng path is available
when xmldsig is built.
🪄 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 Plus
Run ID: 61e329c0-b360-4907-a0d1-c078f131c146
📒 Files selected for processing (4)
Cargo.tomlREADME.mdsrc/xmldsig/sign.rstests/signing_digest.rs
| x509-parser = { version = "0.18", features = ["verify"], optional = true } | ||
| der = { version = "0.8", optional = true } | ||
| crypto-bigint = { version = "0.7", optional = true } | ||
| crypto-common = { version = "0.2", optional = true } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the manifest enables crypto-common's getrandom feature when SysRng is imported.
rg -n 'crypto_common::getrandom::SysRng|crypto-common|getrandom' src Cargo.tomlRepository: structured-world/xml-sec
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Cargo.toml ==\n'
cat -n Cargo.toml | sed -n '1,120p'
printf '\n== src/xmldsig/sign.rs ==\n'
cat -n src/xmldsig/sign.rs | sed -n '1,220p'Repository: structured-world/xml-sec
Length of output: 11911
🌐 Web query:
crypto-common 0.2 SysRng getrandom feature Cargo.toml.orig
💡 Result:
In the crypto-common crate (specifically version 0.2 and later), the getrandom feature enables the usage of the getrandom crate, specifically leveraging its SysRng type [1][2]. This integration replaced older, vendored random number generator implementations within crypto-common [2]. The SysRng type, introduced in getrandom v0.4, acts as the system's preferred random number source, providing a cross-platform interface for retrieving cryptographically secure random data [3]. Within the Cargo.toml of crypto-common 0.2.x, this is configured as an optional dependency: getrandom = { version = "0.4", optional = true, features = ["sys_rng"] } And defined in the [features] section: getrandom = ["rand_core", "dep:getrandom"] When the getrandom feature is enabled in your project, it allows cryptographic traits within crypto-common to utilize SysRng for operations requiring entropy, such as key generation [2][4].
Citations:
- 1: https://github.com/rustcrypto/traits/blob/93dee26c6bde3741a197f1c5f6b7baac277705f3/crypto-common/Cargo.toml
- 2: RustCrypto/traits@870fec5
- 3: https://docs.rs/getrandom/latest/getrandom/index.html
- 4: RustCrypto/traits@crypto-common-v0.2.0-rc.5...crypto-common-v0.2.0-rc.8
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any Cargo.lock entry and feature wiring for crypto-common.
rg -n '^name = "crypto-common"|^version = |getrandom|SysRng' Cargo.lock Cargo.toml src -nRepository: structured-world/xml-sec
Length of output: 3367
🌐 Web query:
crypto-common 0.2 SysRng feature getrandom site:docs.rs
💡 Result:
In the crypto-common crate version 0.2, the getrandom feature enables integration with the getrandom crate, providing access to the system's ambient cryptographically secure random number generator [1][2]. Key details regarding this feature include: Integration: When the getrandom feature is enabled, the crypto-common crate re-exports the getrandom crate [3]. This allows developers to utilize system-provided randomness directly within cryptographic workflows [2]. Functionality: Enabling this feature grants access to the Generate trait's ambient generation methods, specifically try_generate and generate [2]. These methods allow types that implement Generate to be randomly initialized using the system's preferred entropy source without requiring the user to manually pass a random number generator (RNG) instance [2]. Usage Note: The generate method will panic if the system's ambient RNG experiences an internal failure, whereas try_generate returns a Result to handle such errors gracefully [2]. The SysRng mentioned in your query refers to the interface provided by the getrandom crate (which crypto-common facilitates) to interact with the system's primary random source [4]. As of version 0.2, usage of these features is the recommended approach for secure random generation, replacing older, now-deprecated methods that relied solely on passing an explicit CryptoRng [5][6].
Citations:
- 1: https://docs.rs/crypto-common/latest/src/crypto_common/generate.rs.html
- 2: https://docs.rs/crypto-common/latest/crypto_common/trait.Generate.html
- 3: https://docs.rs/crypto-common/latest/src/crypto_common/lib.rs.html
- 4: https://docs.rs/getrandom/latest/i686-pc-windows-msvc/getrandom/
- 5: https://docs.rs/crypto-common/latest/crypto_common/trait.KeyInit.html
- 6: https://docs.rs/crate/crypto-common/0.2.1/target-redirect/i686-pc-windows-msvc/crypto_common/trait.KeyIvInit.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== crypto-common lock entries ==\n'
awk '
$0 ~ /^name = "crypto-common"$/ {show=1; start=NR-2}
show {print NR ":" $0}
show && $0 == "" {count++; if (count >= 2) exit}
' Cargo.lock
printf '\n== surrounding packages that depend on crypto-common ==\n'
python3 - <<'PY'
from pathlib import Path
text = Path("Cargo.lock").read_text().splitlines()
for i, line in enumerate(text):
if line.strip() == 'name = "crypto-common"':
for j in range(max(0, i-5), min(len(text), i+30)):
print(f"{j+1}:{text[j]}")
print("----")
PYRepository: structured-world/xml-sec
Length of output: 1717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== manifest features around xmldsig ==\n'
sed -n '46,62p' Cargo.toml
printf '\n== references to crypto-common feature names in repository ==\n'
rg -n 'crypto-common.*getrandom|getrandom.*crypto-common|feature.*getrandom|SysRng' .Repository: structured-world/xml-sec
Length of output: 805
Enable crypto-common/getrandom for SysRng.
xmldsig enables dep:crypto-common, but src/xmldsig/sign.rs uses crypto_common::getrandom::SysRng. crypto-common gates that module behind its getrandom feature, so xmldsig can fail to compile until the dependency enables it.
🤖 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 `@Cargo.toml` at line 34, The xmldsig dependency on crypto-common is missing
the getrandom feature, which prevents crypto_common::getrandom::SysRng from
compiling in src/xmldsig/sign.rs. Update the crypto-common entry in Cargo.toml
to enable the getrandom feature alongside dep:crypto-common so the SysRng path
is available when xmldsig is built.
1 participant
Summary
SignContextwithsign_template()andsign_with_builder()for full template signing.<DigestValue>, reparse/canonicalize<SignedInfo>, sign canonical bytes, and fill<SignatureValue>.<DigestValue>is accepted only by the signing-template path.SysRngfor blinding.p384from0.14.0-rc.15to stable0.14.Testing
cargo check --all-featurescargo fmt -- --checkcargo clippy --all-targets --all-features -- -D warningscargo nextest run --all-features -E 'binary(signing_digest)'— 8 passedcargo nextest run --all-features— 528 passedcargo test --doc --all-features— 3 passedCloses #78