Skip to content

Refactor to support encrypted assertions #571

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.

Sign up for GitHub

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

Merged
tngan merged 4 commits into from
Jul 6, 2025
Merged

Refactor to support encrypted assertions #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ee13ab
Refactor to support encrypted assertions
ahacker1-securesaml May 26, 2025
43dbb40
Refactor: move authenio to devDependencies
ahacker1-securesaml Jun 3, 2025
0702e7e
Upgrade xml-crypto to 6.1.2
ahacker1-securesaml Jun 3, 2025
59bf3df
Fix: CHANGELOG.md
ahacker1-securesaml Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# 2.10.1

* Changes to libsaml.ts verifySignature. This is an internal function, but we still document changes
- Does not raise error when signature is missing/invalid. Instead it now returns false. This is to simplify logic
- When there are encrypted assertions, returns the entire response, as the "verifiedAssertionNode"

* Fix logic around handling encrypted assertions
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "samlify",
"version": "2.10.0",
"version": "2.10.1",
"description": "High-level API for Single Sign On (SAML 2.0)",
"main": "build/index.js",
"keywords": [
Expand Down Expand Up @@ -39,11 +39,12 @@
"pako": "^1.0.10",
"uuid": "^8.3.2",
"xml": "^1.0.1",
"xml-crypto": "^6.1.0",
"xml-crypto": "^6.1.2",
"xml-escape": "^1.1.0",
"xpath": "^0.0.32"
},
"devDependencies": {
"@authenio/samlify-xsd-schema-validator": "^1.0.5",
"@ava/typescript": "^1.1.1",
"@types/node": "^11.11.3",
"@types/node-forge": "^1.0.1",
Expand Down
47 changes: 24 additions & 23 deletions src/flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,34 +208,35 @@ async function postFlow(options): Promise<FlowResult> {

// verify the signatures (the response is encrypted then signed, then verify first then decrypt)
if (
checkSignature &&
from.entitySetting.messageSigningOrder === MessageSignatureOrder.ETS
checkSignature
) {
// VerifiedAssertionNode is signed. Depending on use case, it may actually be a Response Node
const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions);
if (!verified) {
return Promise.reject('ERR_FAIL_TO_VERIFY_ETS_SIGNATURE');
}
if (!decryptRequired) {
extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode);
}
}

if (parserType === 'SAMLResponse' && decryptRequired) {
const result = await libsaml.decryptAssertion(self, samlContent);
samlContent = result[0];
extractorFields = getDefaultExtractorFields(parserType, result[1]);
}

// verify the signatures (the response is signed then encrypted, then decrypt first then verify)
if (
checkSignature &&
from.entitySetting.messageSigningOrder === MessageSignatureOrder.STE
) {
const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions);
if (verified) {
// First two cases are encrypted assertion cases
// This case the verifiedAssertionNode is actually a response
if (decryptRequired && verified && parserType === 'SAMLResponse' && verifiedAssertionNode) {
// now it is extracted from solely signed contents
const result = await libsaml.decryptAssertion(self, verifiedAssertionNode);
samlContent = result[0];
// extractor depends on signed content
extractorFields = getDefaultExtractorFields(parserType, result[1]);
} else if (decryptRequired && !verified) {
// Encrypted Assertion, the assertion is signed
const result = await libsaml.decryptAssertion(self, samlContent);
const decryptedDoc = result[0];

@mastermatt mastermatt Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ahacker1-securesaml Jul 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mastermatt Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ahacker1-securesaml Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ahacker1-securesaml Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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');
}
} else if (verified) {
// extractor depends on signed content
extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode);
} else {
return Promise.reject('ERR_FAIL_TO_VERIFY_STE_SIGNATURE');
return Promise.reject('FAILED_TO_VERIFY_SIGNATURE');
}
}

Expand Down
31 changes: 18 additions & 13 deletions src/libsaml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ const libSaml = () => {
* - The first element is `true` if the signature is valid, `false` otherwise.
* - The second element is the cryptographically authenticated assertion node as a string, or `null` if not found.
*/
verifySignature(xml: string, opts: SignatureVerifierOptions) {
verifySignature(xml: string, opts: SignatureVerifierOptions) : [boolean, string | null] {
const { dom } = getContext();
const doc = dom.parseFromString(xml);

Expand Down Expand Up @@ -395,10 +395,9 @@ const libSaml = () => {

// guarantee to have a signature in saml response
if (selection.length === 0) {
throw new Error('ERR_ZERO_SIGNATURE');
return [false, null]; // we return false now
}


// need to refactor later on
for (const signatureNode of selection){
const sig = new SignedXml();
Expand Down Expand Up @@ -457,44 +456,50 @@ const libSaml = () => {

sig.loadSignature(signatureNode);

doc.removeChild(signatureNode);

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');
}
Comment on lines 459 to 465

@mastermatt mastermatt May 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ahacker1-securesaml May 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// attempt is made to get the signed Reference as a string();
// note, we don't have access to the actual signedReferences API unfortunately
// mainly a sanity check here for SAML. (Although ours would still be secure, if multiple references are used)
if (!(sig.getReferences().length >= 1)) {
// Require there to be at least one reference that was signed
if (!(sig.getSignedReferences().length >= 1)) {
throw new Error('NO_SIGNATURE_REFERENCES')
}
const signedVerifiedXML = sig.getSignedReferences()[0];
const rootNode = docParser.parseFromString(signedVerifiedXML, 'text/xml').documentElement;
// process the verified signature:
// case 1, rootSignedDoc is a response:
if (rootNode.localName === 'Response') {

// try getting the Xml from the first assertion
const assertions = select(
"./*[local-name()='Assertion']",
rootNode
);

const encryptedAssertions = select(
"./*[local-name()='EncryptedAssertion']",
rootNode
);
// now we can process the assertion as an assertion
if (assertions.length === 1) {
return [true, assertions[0].toString()];
} else if (encryptedAssertions.length >= 1) {
return [true, rootNode.toString()]; // we need to return a Response node, which will be decrypted later
} else {
// something has gone seriously wrong here.
// we don't have any assertion to give back
return [true, null]
}
} else if (rootNode.localName === 'Assertion') {
return [true, rootNode.toString()];
} else {
return [true, null]; // signature is valid. But there is no assertion node here. It could be metadata node, hence return null
}
};
return [false, null]; // we didn't verify anything, none of the signatures are valid

// something has gone seriously wrong if we are still here
throw new Error('ERR_ZERO_SIGNATURE');

/*
// response must be signed, either entire document or assertion
Expand Down
10 changes: 3 additions & 7 deletions test/flow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1199,7 +1199,7 @@ test('should reject signature wrapped response - case 1', async t => {
}
});

test('should reject signature wrapped response - case 2', async t => {
test('should use signed contents in signature wrapped response - case 2', async t => {
//
const user = { email: 'user@esaml2.com' };
const { id, context: SAMLResponse } = await idpNoEncrypt.createLoginResponse(sp, sampleRequestInfo, 'post', user, createTemplateCallback(idpNoEncrypt, sp, binding.post, user));
Expand All @@ -1216,12 +1216,8 @@ test('should reject signature wrapped response - case 2', async t => {
//Put stripped version under SubjectConfirmationData of modified version
const xmlWrapped = outer.replace(/<\/saml:Conditions>/, '</saml:Conditions><saml:Advice>' + stripped.replace('<?xml version="1.0" encoding="UTF-8"?>', '') + '</saml:Advice>');
const wrappedResponse = Buffer.from(xmlWrapped).toString('base64');
try {
await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } });
t.fail();
} catch (e) {
t.is(e.message, 'ERR_POTENTIAL_WRAPPING_ATTACK');
}
const {extract} = await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } });
t.is(extract.nameID, 'user@esaml2.com');
});

test('should throw two-tiers code error when the response does not return success status', async t => {
Expand Down
21 changes: 6 additions & 15 deletions test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,31 +208,22 @@ test('getAssertionConsumerService with two bindings', t => {
t.is(libsaml.verifySignature(_decodedResponse, { metadata: IdPMetadata })[0], true);
});
test('integrity check for request signed with RSA-SHA1', t => {
try {
libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 });
} catch (e) {
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
}
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 });
t.is(verified, false);
});
test('verify a XML signature signed by RSA-SHA256 with metadata', t => {
t.is(libsaml.verifySignature(_decodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 })[0], true);
});
test('integrity check for request signed with RSA-SHA256', t => {
try {
libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 });
} catch (e) {
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
}
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 });
t.is(verified, false);
});
test('verify a XML signature signed by RSA-SHA512 with metadata', t => {
t.is(libsaml.verifySignature(_decodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 })[0], true);
});
test('integrity check for request signed with RSA-SHA512', t => {
try {
libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 });
} catch (e) {
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
}
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 });
t.is(verified, false);
});

test('verify a XML signature with metadata but with rolling certificate', t => {
Expand Down
8 changes: 2 additions & 6 deletions test/issues.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,8 @@ test('#31 query param for sso/slo returns error', t => {
});

test('#87 add existence check for signature verification', t => {
try {
libsaml.verifySignature(readFileSync('./test/misc/response.xml').toString(), {});
t.fail();
} catch ({ message }) {
t.is(message, 'ERR_ZERO_SIGNATURE');
}
const res = libsaml.verifySignature(readFileSync('./test/misc/response.xml').toString(), {});
t.is(res[0], false) // signature is invalid because one doesn't exist
});

test('#91 idp gets single sign on service from the metadata', t => {
Expand Down
20 changes: 16 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@
"@jridgewell/gen-mapping" "^0.1.0"
"@jridgewell/trace-mapping" "^0.3.9"

"@authenio/samlify-xsd-schema-validator@^1.0.5":
version "1.0.5"
resolved "https://registry.yarnpkg.com/@authenio/samlify-xsd-schema-validator/-/samlify-xsd-schema-validator-1.0.5.tgz#bef7fe43928714e473cd95e1577b46d028b945ed"
integrity sha512-HJjmjM1WbeB/z4nVbYEcmtIWTLPKqjrqRGEpC9lu7s03Usc4nxxfrJGjHgh3M8MvBJy4neVUoeM9rP4ym3GLgg==
dependencies:
"@authenio/xsd-schema-validator" "^0.7.3"

"@authenio/xml-encryption@^2.0.2":
version "2.0.2"
resolved "https://registry.yarnpkg.com/@authenio/xml-encryption/-/xml-encryption-2.0.2.tgz#df1f491dacb9b1f65bc7a9a554c189644f72bbe0"
Expand All @@ -19,6 +26,11 @@
escape-html "^1.0.3"
xpath "0.0.32"

"@authenio/xsd-schema-validator@^0.7.3":
version "0.7.3"
resolved "https://registry.yarnpkg.com/@authenio/xsd-schema-validator/-/xsd-schema-validator-0.7.3.tgz#abbf5710705bfab3394aca8b9d5a9e8429873897"
integrity sha512-Jhc/Hxv90bacZr0Fv+u+PEb440zPh4mO6rw+bzEAIBiFLKCtRa/BvKGRxPdCAwsGRPuwl2hFqQGF+Lfz6Q8kFg==

"@ava/typescript@^1.1.1":
version "1.1.1"
resolved "https://registry.yarnpkg.com/@ava/typescript/-/typescript-1.1.1.tgz#3dcaba3aced8026fdb584d927d809752854dc6e6"
Expand Down Expand Up @@ -2522,10 +2534,10 @@ write-file-atomic@^4.0.1:
imurmurhash "^0.1.4"
signal-exit "^3.0.7"

xml-crypto@^6.1.0:
version "6.1.0"
resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.0.tgz#c8224808525e5f15478c50b9fe706112a4e6ef1b"
integrity sha512-0TYPBRPwXLnRGc2F0f9Zc/H076YcP7tkCa2US4jpguuPTEx7TWFqSysIfJ1hP4r2KF82IYzhnzepnsUEsOjlOw==
xml-crypto@^6.1.1:
version "6.1.2"
resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.2.tgz#ed93e87d9538f92ad1ad2db442e9ec586723d07d"
integrity sha512-leBOVQdVi8FvPJrMYoum7Ici9qyxfE4kVi+AkpUoYCSXaQF4IlBm1cneTK9oAxR61LpYxTx7lNcsnBIeRpGW2w==
dependencies:
"@xmldom/is-dom-node" "^1.0.1"
"@xmldom/xmldom" "^0.8.10"
Expand Down