Refactor to support encrypted assertions

[ahacker1-securesaml]

Support encrypted assertions which was broken during security patch

[ahacker1-securesaml]

@h-bragg, @tngan , can you help review this PR.

To run tests we need to:
yarn add @authenio/samlify-xsd-schema-validator
yarn run build

Now this passes all the test cases. (last time, I forgot to run yarn add schema validator, which is why it didn't detect the failing test cases for the encrypted assertions).

To review:
extractor depends solely on verified XML i.e. getSignedReferences()

[mastermatt]

Confusingly, checkSignature now sometimes returns false, but usually throws an error when unable to validate the signature.
When I upgraded from 2.9.1 to 2.10.0, I had tests start to fail because some error handling was looking for ERR_FAILED_TO_VERIFY_SIGNATURE (to provide custom messaging). Is the expectation now that the xml-crypto errors are free to bubble up? or should checkSignature be wrapped in a try/catch and the xml-crypto error passed as a cause?

[ahacker1-securesaml]

https://github.com/node-saml/xml-crypto/blob/9b91edf61cbc83853d824933be0dafa1f6114de4/src/signed-xml.ts#L336-L339

We upgraded from 3.0 xml-crypto to 6.0. In 3.0 they used to throw error, however the 6.0 they did some changes to return false.

When I gave the security patches for xml-crypto 6.0.1, I had to keep it to return false. The downside being for libraries like samlify which were previously on 3.0, it now returns false.

When I upgraded from 2.9.1 to 2.10.0, I had tests start to fail because some error handling was looking for ERR_FAILED_TO_VERIFY_SIGNATURE (to provide custom messaging). Is the expectation now that the xml-crypto errors are free to bubble up? or should checkSignature be wrapped in a try/catch and the xml-crypto error passed as a cause?

I'm not sure what is the best way here. You can propose patches in the libsaml.verifySignature on how to handle errors.

[ahacker1-securesaml]

@mastermatt do you plan on proposing any new changes i.e. error handling.
If not, I will apply the current feedback and then start the release process for a new version.

[avocadojesus]

This is also breaking for services our teams use that leverage encrypted assertions, and we will not be able to bump until this is released.

[ahacker1-securesaml]

Ok, I will ask maintainer to release this.

[mastermatt]

I'm fine with the new error handling, I just wanted to make sure it was understood that thrown errors have changed. Whether intentional or not.

[noahg]

This is also impacting us, thanks for putting together a fix so quickly.

[ahacker1-securesaml]

Maintainer tngan, says he will review over the weekend. In the meantime, I recommend sponsoring him: https://github.com/sponsors/tngan

[h-bragg]

LGTM

thanks for the proper fix

[tngan]

bugbot run

[noahg]

bugbot run

@tngan looks like this failed, thanks for the help

[davecardwell]

@tngan is there a path to getting this live? Anything we can do to help get it over the line?

[ahacker1-securesaml]

@davecardwell There's a sponsor link:
https://github.com/sponsors/tngan

Here are the tiers:

$50 a month
🎩Professional - feel free to consider this tier if you are an individual and you use my work for a commercial product that is ready for sale.

I will prioritise your requests and issues on my open source projects

$100 a month
👥Partner - feel free to consider this tier if your team and your company use my work in your products.

I will

prioritise your requests and issues on my open source projects
review some of your projects and help you improve

[mastermatt]

@ahacker1-securesaml I didn't catch this on first review, but I'm having test failures now; is there a reason why you didn't overwrite samlContent on this line? As is, samlContent has an Assertion node for encrypt-then-sign flows, but still has the EncryptedAssertion node for sign-then-encrypt flows.

[ahacker1-securesaml]

Hi,
Thanks for reviewing. Can you share your test failure. I'm not getting any test errors when I run yarn test in my local machine.

I don't think overwriting the samlContent will change the behavior.

Here's how libsaml extracts the fields, given an assertion, and a response node.
https://github.com/SecureSAML/samlifyfork/blob/59bf3df3cf2c7882613c674f25ccb41e03f75472/src/extractor.ts#L100-L148

Currently samlContent stores a Response node (that contains an assertion or encrypted assertion nodes).
In the first case:

      // now it is extracted from solely signed contents
      const result = await libsaml.decryptAssertion(self, verifiedAssertionNode); // verifiedAssertionNode is a response node
      samlContent = result[0]; // result[0] returns the doc

This is actually the response node with decrypted asesertion

In second case, where I didn't change samlContent, samlContent would be the original SAML Response with a Response with encrypted Assertion

Even with a response with EncryptedAssertion, we can extract the fields for the Response node. What matters is extracting the fields for the assertion. As you can see:

      const [decryptedDocVerified, verifiedDecryptedAssertion] = libsaml.verifySignature(decryptedDoc, verificationOptions);
      if (decryptedDocVerified) {
        // extractor depends on signed content
        extractorFields = getDefaultExtractorFields(parserType, verifiedDecryptedAssertion);
      } else {
        return Promise.reject('FAILED_TO_VERIFY_SIGNATURE');
      }

The assertion in the extracted fields is the decrypted and signed assertion.

<Response>
<status></status>
<EncryptedAssertion/>

</Response>

[mastermatt]

Sorry, the test failures I'm seeing are in my applications that use samlify. I wasn't referring to tests in this lib. Although I'll argue this feature is missing test coverage currently.
And the extracted fields returned in FlowResult are working as expected in all cases.

The part that I'm calling out is the samlContent property of FlowResult. It never contained the EncryptedAssertion before.
Having access to the decoded and decrypted XML content, after calling parseLoginResponse, is extremely helpful when debugging payloads from third parties.

[ahacker1-securesaml]

Thanks for the clarification. I do agree that the samlContent should be the decrypted response, i.e. set the samlContent to results[0] in the second branch.

Thanks again.

[ahacker1-securesaml]

cc @tngan @mastermatt , just released the fix here:
#575

[GuillaumeCisco]

Why don't we see the 2.10.1 version as the last release on github but we do see it on npm?
Thank you,