-
-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decrypt assertion broken #495
Comments
Any Idea how to fix this? |
I also have the same problem, have you found a possible solution? |
@isanttila and @jsgsdev Tempory solved using an old version of samplify |
@ambigus9 ohhhhhh yeah bro, fix the problem thank you! |
@isanttila the commit doesn't seem to be the only culprit as using an overrides with @xmldom/xmldom 0.8.5, which doesn't contain the backported commit, doesnt fix the problem :
❯ npm test
Error: error:1E08010C:DECODER routines::unsupported
at Object.privateDecrypt (node:internal/crypto/cipher:79:12)
at decryptKeyInfoWithScheme (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:258:26)
at decryptKeyInfo (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:246:14)
at Object.decrypt (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:187:24)
at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:568:31
at new Promise (<anonymous>)
at Object.decryptAssertion (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:553:20)
at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:222:60
at step (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:33:23)
at Object.next (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:14:53) {
library: 'DECODER routines',
reason: 'unsupported',
code: 'ERR_OSSL_UNSUPPORTED'
}
Error: ERR_EXCEPTION_OF_ASSERTION_DECRYPTION
at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:573:39
at Object.decrypt (/Users/nlecam/Developpement/tests/samlify/node_modules/@authenio/xml-encryption/lib/xmlenc.js:214:12)
at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:568:31
at new Promise (<anonymous>)
at Object.decryptAssertion (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/libsaml.js:553:20)
at /Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:222:60
at step (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:33:23)
at Object.next (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:14:53)
at fulfilled (/Users/nlecam/Developpement/tests/samlify/node_modules/samlify/build/src/flow.js:5:58) Using samlify 2.7.7 fixes the problem :
❯ npm test
<nameID> {
Role: [
'offline_access',
'view-profile',
'default-roles-master',
'manage-account-links',
'uma_authorization',
'manage-account'
]
} I'll check to see if I find a more recent xmldom version which works |
@KuSh Ok, good to know. For us, Samlify 2.8.6 with xmldom downgraded to 0.8.3 is working, but this may depend on the exact details of the SAML Response in each case. |
Using samlify 2.8.6 with xmldom 0.8.3 gives me |
Hi everyone, check if the private key file you are using, see if there are
Edit: This is for the error |
It is better to get fixed soon, coz the upgraded version includes security patches. |
@isanttila Do you have a sample code snippet or saml document without sensitive information? Thanks for reporting. |
It has but without new lines, I'll reformat it to see if it fixes anything, but the same code works without changes just by downgrading samlify 2.7.7 |
Ok. |
This is what I can get easily. It's a sample response, before and after decryption. |
I also suffer from this bug. In addition, switching to samlify 2.7.7 does not work for me, because the IDP I'm using does not accept the request in that case. It's a shame because inside a debugger I can already see all the decrypted data just fine, the code really trips at the finish line. Still, thank you a lot for a very well documented library! The best I've seen for saml2 @tngan |
I debugged the issue today and I'm able to overcome the problem by changing the function
If I don't use the parentNode, then I get the error as described in the original post. If I use the parentNode but try to do it with a single line with EDIT: link to changes I made in my fork that I will use https://github.com/sellonen/samlify/blob/95a9edf02dd8205580f3b2357b989a447e575521/src/libsaml.ts#L680 |
@sellonen const parent = assertions[0].parentNode;
if (parent) {
parent.appendChild(encryptAssertionNode);
parent.removeChild(assertions[0]);
}
// doc.replaceChild(encryptAssertionNode, assertions[0]); Could you please update your fork (libsaml.ts:637) to include this change as well? |
@yanngrandgirard Thanks, I just did, even though it worked in my case even before the change. For anyone stumbling here before the issue is fixed in the original repository, you may temporarily put this line in your package.json dependencies and then use it like any other package. (But the versioning is total crap, I don't recommend this in the long term)
In the case of |
@sellonen Thanks, it will greatly help until this gets fixed in the original repo! However, I think what I found could be of interest. From what I have learned so far, the two lines below create a // encryptAssertion(), libsaml.ts:607
const doc = new dom().parseFromString(xml);
// decryptAssertion(), libsaml.ts:661
const xml = new dom().parseFromString(entireXML); So, I would say the As for this message: // encryptAssertion(), libsaml.ts:636
const encryptAssertionNode = new dom().parseFromString(...);
// decryptAssertion(), libsaml.ts:679
const assertionNode = new dom().parseFromString(res); The two lines above also create Putting it all together, another way to fix this bug could be: // encryptAssertion(), libsaml.ts:637
// doc.replaceChild(encryptAssertionNode, assertions[0]);
doc.lastChild.replaceChild(encryptAssertionNode.firstChild, assertions[0]);
// decryptAssertion(), libsaml.ts:680
// xml.replaceChild(assertionNode, encryptedAssertions[0]);
xml.lastChild.replaceChild(assertionNode.firstChild, encryptedAssertions[0]); Although I am not sure Disclaimer: my use case does not call |
`@xmldom/xmldom` 0.8.6 included: fix: Properly check nodes before replacement. xmldom/xmldom#457 Which caused the `.replaceChild` calls in `encryptAssertion` and `decryptAssertion` to error with: > "Not found: child not in parent" The root cause is a subtle difference in `@xmldom/xmldom` `Document` vs `Element` instances. The previous code was asking xmldom to replace an element from a top-level Document with another top-level Document. The patch to xmldom 0.8.6 started the enforcement of `.replaceChild` being passed `Nodes` who share a common parent Node. e.g. in the case of `encryptAssertion`: `doc.replaceChild(encryptAssertionNode, rawAssertionNode)`. It's important to distinguish that neither `doc` nor `encryptAssertionNode` are `Element` nodes, but instead `Document` Nodes. Meaning `doc` does _not_ refer to the `<samlp:Response>` node, but instead a meta object one level up. To reference the Response tag, you instead use the `Document#documentElement` attribute. Changing that line to the following as the same intended affect using the correct node references. `doc.documentElement.replaceChild(encryptAssertionNode.documentElement, rawAssertionNode)` I renamed a few of the variables in an attempt to clarify which are `Documents`. Also fixes an issue where the `ERR_NO_ASSERTION` and `ERR_UNDEFINED_ENCRYPTED_ASSERTION` errors were not being thrown if exactly zero nodes were found. fixes: tngan#495
I opened #511 just now to fix this. |
Since a tightened checking was introduced in xmldom a week ago, decryptAssertion in libsaml has been broken.
Here is the change that affects how replaceChild behaves: xmldom/xmldom@3bc6ccf
Because of this change,
xml.replaceChild(assertionNode, encryptedAssertions[0])
fails with the error 'Not found: child not in parent' and an ERR_EXCEPTION_OF_ASSERTION_DECRYPTION is thrown.This happens at least when the SAML response XML contains a header in the beginning (e.g.
<?xml version="1.0" encoding="UTF-8"?>
). When this is the case, entireXML contains the header as the first element, and Response as the second element, and EncryptedAssertion is a child of Response. Therefore, EncryptedAssertion is not a direct child of entireXML, and replaceChild fails due to the tightened checking.The text was updated successfully, but these errors were encountered: