Refactor to support encrypted assertions #571
Conversation
- Update CHANGELOG.md
|
@h-bragg, @tngan , can you help review this PR. To run tests we need to: 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: |
| verified = sig.checkSignature(doc.toString()); | ||
|
|
||
| // immediately throw error when any one of the signature is failed to get verified | ||
| if (!verified) { | ||
| throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE'); | ||
| continue; | ||
| // throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE'); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@mastermatt do you plan on proposing any new changes i.e. error handling. |
|
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. |
|
Ok, I will ask maintainer to release this. |
|
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. |
|
This is also impacting us, thanks for putting together a fix so quickly. |
|
Maintainer tngan, says he will review over the weekend. In the meantime, I recommend sponsoring him: https://github.com/sponsors/tngan |
|
bugbot run |
🚨 BugBot failed to runRemote branch not found for this Pull Request. It may have been merged or deleted (requestId: serverGenReqId_92011afe-6408-4450-b1f9-c523883ac891). |
@tngan looks like this failed, thanks for the help |
Support encrypted assertions which was broken during security patch