[codex] OPE-204 enforce SAML assertion signature policy - #200
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63af9cf9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && !matches!( | ||
| verified_signature, | ||
| Some(VerifiedSamlSignature { | ||
| element: SamlSignedElement::Assertion | ||
| }) |
There was a problem hiding this comment.
Use namespace-aware assertion-signature detection
For IdPs that emit a standards-valid assertion signature with a custom XMLDSig prefix such as <sig:Signature>, this check can reject valid logins even after signature verification succeeds. The parsed.signature.assertion flag that feeds VerifiedSamlSignature.element is prefix-sensitive in crates/rustauth-saml/src/saml/assertions.rs (assertion_signature_present only recognizes <ds:Signature> or unprefixed <Signature>), so the ACS fast path classifies those assertion signatures as Response and this new matches! returns SAML_ASSERTION_SIGNATURE_REQUIRED. Please make the element classification namespace-aware before enforcing the policy on it.
Useful? React with 👍 / 👎.
Linear: OPE-204
Summary
This fixes the SAML assertion-signature policy check in the ACS validation path. When
want_assertions_signedis enabled, a verified response-level signature no longer satisfies the assertion-signature requirement; the verified signature must be attached to the assertion.Root Cause
The previous validation only checked whether any verified SAML signature existed. That allowed
want_assertions_signedto be satisfied by a non-assertion signature, which weakened deployments that explicitly require assertion-level signatures.Changes
VerifiedSamlSignature.element == SamlSignedElement::Assertionwhenwant_assertions_signedis enabled.Validation
cargo fmt --all --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo nextest run -p rustauth-sso --all-features