[tvdijen]

No description provided.

[tvdijen]

Ben did a great job in figuring out that this has to do with padding. Sent an e-mail to Rob R. to figure out if we're using his library in the wrong way, or we hit a bug.

[monkeyiq]

I might add some of the transcript of that here for reference.

The failing test in question can be executed locally with

phpunit  ./tests/SAML2/SignedElementHelperTest.php --filter=testValidateWithoutModification

Note that to run that test on Fedora (44) you may have to set the following to allow the algo to work.

export OPENSSL_ENABLE_SHA1_SIGNATURES=1

In the specific test, the copying is not the problem. rr xmlsec 4.x will not validate the original element that is presentde here either.

If in configureRSAKey() you set the following at the top of the method then the test passes:

$this->cryptParams['padding'] = 64; 
// or as Tim discovered 16 works too.

Glad I could help a little. It might be that the padding needs to be slightly different on the enc/sign key use or we are making the key with intent to encrypt and not sign or something. That configureRSAKey call is buried inside verifySignature() so I'm not too sure what a caller of the rr lib could do to easily change it.

Also in Utils / validateSignature() you can do something like:

         if( $algo == XMLSecurityKey::RSA_1_5 ) {         
            $key->adjustPadding(16);
        }

with a suitable adjustPadding that sets the padding in xmlseckey again, probably not ideal but avoids stamping the padding for all keys.

public function adjustPadding( $mask )
    {
        $this->cryptParams['padding'] &= ~(RSA::ENCRYPTION_PKCS1);
        $this->cryptParams['padding'] |= $mask;
        return $this;
    }

None of this is a recommendation for changes to the rr lib. My familiarity with the internals there is not where it needs to be to make any such suggestion. These are just things I found while trying to get this particular test case to pass. It might be useful at some stage so I have copied it here.

[tvdijen]

I asked Rob for help and with a little bit of help from AI he did this analysis:

Ok, I know there were some changes in padding from both the change to phpseclibs as well as in some of the v4 hardening so I just threw the problem at some agents to diagnose. They all basically came up with the same answer (v3 was much more forgiving so didn't enforce proper usage everywhere). It does suggest a change to xmlseclibs to make this more explicit so I am going to at least add that check to the lib.

The bug is in their test fixture, not xmlseclibs. The test signs with an RSA encryption key (XMLSecurityKey::RSA_1_5), so the XML signature literally says SignatureMethod=["http://www.w3.org/2001/04/xmlenc#rsa-1_5"](http://www.w3.org/2001/04/xmlenc#rsa-1_5). v3 only passed by accident.

I reproduced it at their PR head (600dff8) with xmlseclibs 4.0.1 on phpseclib 3.0.57 and 3.0.58. testValidateWithoutModification fails with "Unable to validate Signature", coming from Utils.php:230.

Why it fails

    [CertificatesMock.php:102–117](https://github.com/simplesamlphp/saml2/blob/600dff839440565c69dab6104000c75114a20a82/tests/SAML2/CertificatesMock.php#L102-L117) builds both keys as RSA_1_5. [SignedElementHelperTest.php:33–36](https://github.com/simplesamlphp/saml2/blob/600dff839440565c69dab6104000c75114a20a82/tests/SAML2/SignedElementHelperTest.php#L33-L36) then signs with that private key, and [Utils.php:357–387](https://github.com/simplesamlphp/saml2/blob/600dff839440565c69dab6104000c75114a20a82/src/SAML2/Utils.php#L357-L387) doesn't object.
    In v4, RSA_1_5 sets phpseclib's ENCRYPTION_PKCS1 (2), while real RSA-SHA* types set SIGNATURE_PKCS1 (64) ([XMLSecurityKey.php:230–305](https://github.com/robrichards/xmlseclibs/blob/3d910b23ac1f566c2fa48abf795d710f78a3b770/src/XMLSecurityKey.php#L230-L305)). configureRSAKey() ([:880–907](https://github.com/robrichards/xmlseclibs/blob/3d910b23ac1f566c2fa48abf795d710f78a3b770/src/XMLSecurityKey.php#L880-L907)) passes 2 to withPadding(). That only sets the encryption mode, so the signature mode stays at whatever the loaded key defaulted to.
    Those defaults differ: a bare private key loads with PSS, but a key from an X.509 cert loads with PKCS#1 v1.5 ([phpseclib X509.php:2205–2234](https://github.com/phpseclib/phpseclib/blob/d17e0ddaeaf6f22f7e007cbb437d78792fe2a0e4/phpseclib/File/X509.php#L2205-L2234)). So the test signs with PSS and verifies with PKCS#1 v1.5.

Why "16" makes it pass. In phpseclib 3, 16 is SIGNATURE_PSS ([constants](https://github.com/phpseclib/phpseclib/blob/d17e0ddaeaf6f22f7e007cbb437d78792fe2a0e4/phpseclib/Crypt/RSA.php#L90-L158)). Forcing it in configureRSAKey() just makes verify match the PSS signature. Forcing 64 works too. Neither is a real fix: the signature still declares an encryption URI, and patching configureRSAKey() globally would also change encryption callers.

Why v3 passed. v3.1.5 ([XMLSecurityKey.php:560–594](https://github.com/robrichards/xmlseclibs/blob/5e79a25c52ce219f3b42b9e554de0adb9bfc43bb/src/XMLSecurityKey.php#L560-L594)) called openssl_sign/openssl_verify with their defaults, SHA-1 and PKCS#1 v1.5, no matter what the key type said.



Their fix. Sign with an RSA_SHA256 private key built from the same PEM, verify with their existing getPublicKeySha256(), and keep the RSA_1_5 helpers for encryption tests. I tried that locally as a test-only change, and all 5 tests (13 assertions) pass on unmodified xmlseclibs 4.0.1 with phpseclib 3.0.57.

For xmlseclibs. No padding compatibility change is needed. It would be worth a small hardening change, though: reject encryption-only key types (RSA_1_5, RSA_OAEP*) in signData()/verifySignature() and before DSig writes the SignatureMethod, with tests. Right now v4 doesn't cleanly reject this misuse. It silently produces a signature it can't verify, which is why the error is so confusing for them.

I only ran that one test class, not their full suite. Their checkout is on local branch investigate-426-padding in /workspace/saml2-426, and the fixture patch and logs are in /workspace/saml2-426-results/. Nothing was posted or pushed. I can draft a reply for their PR, or prototype the xmlseclibs rejection on a local branch, if either would help.

In other words, the failing unit test was broken in the first place. Fine, we should fix it or disable it and get going with this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.82%. Comparing base (dfaa73a) to head (bbc67e1).
⚠️ Report is 4 commits behind head on release-4.x.

Additional details and impacted files

@@                Coverage Diff                @@
##             release-4.x     #426      +/-   ##
=================================================
+ Coverage          65.78%   65.82%   +0.04%     
- Complexity          1882     1884       +2     
=================================================
  Files                141      141              
  Lines               4986     4992       +6     
=================================================
+ Hits                3280     3286       +6     
  Misses              1706     1706              

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[monkeyiq]

This update allows all tests to pass locally for me here. The codecov seems stuck on github but other tests seem ok.

[monkeyiq]

Longer term we might like to differentiate with a
getPrivateKeySha256 -> getPrivateKeyForSigning
getPublicKeySha256 -> getPublicKeyForSignatureVerification
or something.