test(xmldsig): add real SAML IdP integration coverage

[polaz]

Summary

• add integration test coverage for a real signed SAML 2.0 IdP response fixture
• validate positive path (DsigStatus::Valid) and tampered-reference path (ReferenceDigestMismatch)
• cover end-to-end verification via verify_signature_with_pem_key

Validation

• cargo fmt -- --check
• cargo clippy --all-targets --all-features -- -D warnings
• cargo nextest run --workspace
• cargo test --doc --workspace

Notes

• arch/ROADMAP.md is currently git-ignored in this repository, so roadmap status updates were not included in this PR diff.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cead7fa0-88cf-4a3f-b566-36fd1c29e792

📥 Commits

Reviewing files that changed from the base of the PR and between 6f9156e and c97fcb9.

⛔ Files ignored due to path filters (1)

• tests/fixtures/keys/ec/saml-idp-ecdsa-pubkey.pem is excluded by !**/*.pem

📒 Files selected for processing (3)

• tests/fixtures/saml/response_signed_by_idp_ecdsa.xml
• tests/fixtures_smoke.rs
• tests/saml_idp_integration.rs

---

📝 Walkthrough

Summary by CodeRabbit

Tests

• Added comprehensive integration tests for SAML signature verification with ECDSA cryptography
• Tests validate correct signature detection and tampering identification

Walkthrough

Adds end-to-end SAML XMLDSig verification tests and supporting fixtures: a real IdP-signed SAML response fixture plus two integration tests (valid signature check and tampering detection). Also updates smoke fixture assertions and a new ECDSA public-key PEM fixture reference.

Changes

Cohort / File(s)	Summary
SAML integration tests tests/saml_idp_integration.rs	New integration test file with two tests: one verifies a real SAML 2.0 IdP response signature (expects DsigStatus::Valid and stored pre-digest data) and one verifies tampering is detected (expects DsigStatus::Invalid with ReferenceDigestMismatch).
SAML fixture tests/fixtures/saml/response_signed_by_idp_ecdsa.xml	New SAML 2.0 Response fixture containing an ECDSA (ecdsa-sha256) XML Signature, assertion, attributes, timestamps, and IDs used by the integration tests.
Fixture smoke tests tests/fixtures_smoke.rs	Updated assertions: added check for tests/fixtures/keys/ec/saml-idp-ecdsa-pubkey.pem PEM content and incremented expected fixture file count (77 → 79) and key count (22 → 23).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(xmldsig): add full signature verify pipeline #30: Adds end-to-end SAML XML fixtures and integration test scaffolding that these tests call into.
• feat(xmldsig): add VerifyResult status model #34: Introduces/defines verify_signature_with_pem_key result model (DsigStatus/FailureReason) relied upon by the new tests.
• feat(xmldsig): add ecdsa signature verification #27: Adds ECDSA XMLDSig verification support exercised by the new SAML ECDSA fixture and tests.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: adding real SAML IdP integration test coverage for XMLDSig verification.
Description check	✅ Passed	The description is well-related to the changeset, outlining the test coverage added, validation paths covered, and local validation steps performed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch test/p1-026-saml-idp-integration

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Adds an integration test intended to exercise the XMLDSig verification pipeline against a real-world SAML 2.0 IdP response fixture (including a positive verification case and a tampered-reference failure case).

Changes:

• Introduces a new integration test file that verifies an ECDSA-signed SAML response end-to-end using verify_signature_with_pem_key.
• Adds a tampering test that mutates the XML and asserts a ReferenceDigestMismatch failure.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[polaz]

@coderabbitai re-review

[polaz]

@coderabbitai re-review

[coderabbitai]

@polaz Sure, I'll re-review the changes now!

---

🧠 Learnings used

Learnt from: polaz
Repo: structured-world/xml-sec PR: 41
File: tests/donor_full_verification_suite.rs:165-178
Timestamp: 2026-04-04T06:14:34.735Z
Learning: In `tests/donor_full_verification_suite.rs` (xml-sec Rust project), the `Expectation::Skip` arm must exercise skipped vectors through `VerifyContext::new().verify(xml)` (no injected PEM key) so that the real key-resolution path runs, and must assert a concrete blocker outcome per case: `Invalid(KeyNotFound)` for vectors with unresolved KeyValue/X.509 key material, and `ParseSignedInfo(UnsupportedAlgorithm)` for DSA-signature vectors. Accepting any `Err` or non-Valid result as a valid skip is insufficient and can mask stale skip reasons or unrelated failures.

✅ Actions performed

Full review triggered.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the new integration test file
cat tests/saml_idp_integration.rs

echo "---FIXTURE FILES---"
# Find fixture files related to SAML
fd -e xml -e saml . --max-depth 5

echo "---FIND FIXTURE DIR---"
find . -name "*.xml" | head -30

echo "---LIB/SRC EXPORTS---"
# Look at what verify_signature_with_pem_key looks like
rg -n "verify_signature_with_pem_key" --type rust -C 5

echo "---DsigStatus and FailureReason---"
rg -n "FailureReason|DsigStatus" --type rust -C 2 | head -80

echo "---Cargo.toml---"
cat Cargo.toml

Length of output: 42779

---

🏁 Script executed:

#!/bin/bash
echo "---SAML FIXTURE---"
cat tests/fixtures/saml/response_signed_by_idp_ecdsa.xml

echo "---GITIGNORE---"
cat .gitignore 2>/dev/null || echo "no .gitignore found"

echo "---CHECK FIXTURE IN GITIGNORE---"
git check-ignore -v tests/fixtures/saml/response_signed_by_idp_ecdsa.xml 2>&1

echo "---GIT STATUS---"
git status --short

echo "---VERIFY CONTEXT---"
rg -n "pub struct VerifyResult|pub signed_info_references|pub status" --type rust -C 1 | head 50

echo "---replacen usage check---"
# Check if test@example.com only appears in the fixture
rg -n "test@example.com" --type rust
rg -n "test@example.com" tests/fixtures/saml/response_signed_by_idp_ecdsa.xml | wc -l

echo "---VerifyResult fields---"
rg -n "pub struct VerifyResult" --type rust -A 10

Length of output: 5773

---

@polaz Here is the review of PR #44.

---

tests/saml_idp_integration.rs

✅ Strengths

• The positive path is well-guarded: len() check before indexing, .expect() with a meaningful message, and distinct assertions on both overall status and the first reference status.
• The tamper test uses a sound double-guard pattern (contains pre-check + assert_ne! post-check), and replacen(..., 1) is safe since test@example.com appears exactly once in the fixture (confirmed).
• FailureReason::ReferenceDigestMismatch { ref_index: 0 } is asserted precisely rather than accepting any Invalid variant — consistent with the strictness pattern established in reference_integration.rs.

---

🔴 Issues

1. store_pre_digest: true without any pre-digest assertion (Line 19)

let result = verify_signature_with_pem_key(IDP_RESPONSE_SIGNED_XML, IDP_PUBLIC_KEY_PEM, true)

store_pre_digest: true allocates and stores pre-digest data, but the test never asserts anything about it. This exercises a different internal code path than the tamper test (false), which is potentially useful, but the divergence is silent. Either:

• Add an assertion like assert!(result.signed_info_references[0].pre_digest.is_some()), or
• Switch to false to match the tamper test and document the path difference elsewhere.

Using true with no assertion is misleading: it looks like a test of pre-digest behaviour that was left half-written.

---

2. Hard-coded PEM key instead of a fixture file (Lines 11–15)

const IDP_PUBLIC_KEY_PEM: &str = "-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEyjU9...
-----END PUBLIC KEY-----
";

All other integration tests (signature_pipeline_integration.rs, donor_full_verification_suite.rs) load keys via read_fixture(Path::new("tests/fixtures/keys/...")). Embedding the PEM inline:

• Makes key rotation (if ever needed for the donor fixture) require a code change rather than a file swap.
• Is inconsistent with the established convention.

Suggestion: Extract the key to tests/fixtures/keys/ec/saml-idp-ecdsa-pubkey.pem and load it with include_str! (matching the XML fixture pattern already used here) or read_fixture.

---

⚠️ Warnings

3. Gitignored ROADMAP reference in doc comment (Line 1)

//! Integration tests for real-world SAML response verification (ROADMAP P1-026).

ROADMAP.md is listed in .gitignore. A contributor cloning the repo has no way to look up "P1-026". Consider replacing with a link to this PR or a GitHub issue number (e.g., //! Covers P1-026 – see #44``).

---

4. SHA-1 digest is used by the fixture with no comment

The fixture uses http://www.w3.org/2000/09/xmldsig#sha1 for DigestMethod. This is fine for a donor fixture testing legacy compatibility, but it's worth a brief comment noting it's intentional:

// Fixture intentionally uses SHA-1 digest (real-world SAML IdP legacy interop).
const IDP_RESPONSE_SIGNED_XML: &str =
    include_str!("fixtures/saml/response_signed_by_idp_ecdsa.xml");

Without such a comment, future reviewers may not know if SHA-1 is being exercised deliberately or accidentally.

---

Summary

	Finding
🔴	store_pre_digest: true with no pre-digest assertion — assert it or switch to false
🔴	Inline PEM key breaks the established tests/fixtures/keys/ convention
⚠️	Gitignored ROADMAP P1-026 reference is opaque
⚠️	SHA-1 fixture usage lacks an explanatory comment

---

🧠 Learnings used

Learnt from: polaz
Repo: structured-world/xml-sec PR: 41
File: tests/donor_full_verification_suite.rs:165-178
Timestamp: 2026-04-04T06:14:34.735Z
Learning: In `tests/donor_full_verification_suite.rs` (xml-sec Rust project), the `Expectation::Skip` arm must exercise skipped vectors through `VerifyContext::new().verify(xml)` (no injected PEM key) so that the real key-resolution path runs, and must assert a concrete blocker outcome per case: `Invalid(KeyNotFound)` for vectors with unresolved KeyValue/X.509 key material, and `ParseSignedInfo(UnsupportedAlgorithm)` for DSA-signature vectors. Accepting any `Err` or non-Valid result as a valid skip is insufficient and can mask stale skip reasons or unrelated failures.

[polaz]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.