Skip to content

Commit 337a56e

Browse files
Refactor to support encrypted assertions (#571)
* Refactor to support encrypted assertions - Update CHANGELOG.md * Refactor: move authenio to devDependencies * Upgrade xml-crypto to 6.1.2 * Fix: CHANGELOG.md
1 parent 73cae30 commit 337a56e

File tree

8 files changed

+79
-70
lines changed

8 files changed

+79
-70
lines changed

CHANGELOG.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
# 2.10.1
2+
3+
* Changes to libsaml.ts verifySignature. This is an internal function, but we still document changes
4+
- Does not raise error when signature is missing/invalid. Instead it now returns false. This is to simplify logic
5+
- When there are encrypted assertions, returns the entire response, as the "verifiedAssertionNode"
6+
7+
* Fix logic around handling encrypted assertions

package.json

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "samlify",
3-
"version": "2.10.0",
3+
"version": "2.10.1",
44
"description": "High-level API for Single Sign On (SAML 2.0)",
55
"main": "build/index.js",
66
"keywords": [
@@ -39,11 +39,12 @@
3939
"pako": "^1.0.10",
4040
"uuid": "^8.3.2",
4141
"xml": "^1.0.1",
42-
"xml-crypto": "^6.1.0",
42+
"xml-crypto": "^6.1.2",
4343
"xml-escape": "^1.1.0",
4444
"xpath": "^0.0.32"
4545
},
4646
"devDependencies": {
47+
"@authenio/samlify-xsd-schema-validator": "^1.0.5",
4748
"@ava/typescript": "^1.1.1",
4849
"@types/node": "^11.11.3",
4950
"@types/node-forge": "^1.0.1",

src/flow.ts

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -208,34 +208,35 @@ async function postFlow(options): Promise<FlowResult> {
208208

209209
// verify the signatures (the response is encrypted then signed, then verify first then decrypt)
210210
if (
211-
checkSignature &&
212-
from.entitySetting.messageSigningOrder === MessageSignatureOrder.ETS
211+
checkSignature
213212
) {
213+
// VerifiedAssertionNode is signed. Depending on use case, it may actually be a Response Node
214214
const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions);
215-
if (!verified) {
216-
return Promise.reject('ERR_FAIL_TO_VERIFY_ETS_SIGNATURE');
217-
}
218-
if (!decryptRequired) {
219-
extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode);
220-
}
221-
}
222215

223-
if (parserType === 'SAMLResponse' && decryptRequired) {
224-
const result = await libsaml.decryptAssertion(self, samlContent);
225-
samlContent = result[0];
226-
extractorFields = getDefaultExtractorFields(parserType, result[1]);
227-
}
228-
229-
// verify the signatures (the response is signed then encrypted, then decrypt first then verify)
230-
if (
231-
checkSignature &&
232-
from.entitySetting.messageSigningOrder === MessageSignatureOrder.STE
233-
) {
234-
const [verified, verifiedAssertionNode] = libsaml.verifySignature(samlContent, verificationOptions);
235-
if (verified) {
216+
// First two cases are encrypted assertion cases
217+
// This case the verifiedAssertionNode is actually a response
218+
if (decryptRequired && verified && parserType === 'SAMLResponse' && verifiedAssertionNode) {
219+
// now it is extracted from solely signed contents
220+
const result = await libsaml.decryptAssertion(self, verifiedAssertionNode);
221+
samlContent = result[0];
222+
// extractor depends on signed content
223+
extractorFields = getDefaultExtractorFields(parserType, result[1]);
224+
} else if (decryptRequired && !verified) {
225+
// Encrypted Assertion, the assertion is signed
226+
const result = await libsaml.decryptAssertion(self, samlContent);
227+
const decryptedDoc = result[0];
228+
const [decryptedDocVerified, verifiedDecryptedAssertion] = libsaml.verifySignature(decryptedDoc, verificationOptions);
229+
if (decryptedDocVerified) {
230+
// extractor depends on signed content
231+
extractorFields = getDefaultExtractorFields(parserType, verifiedDecryptedAssertion);
232+
} else {
233+
return Promise.reject('FAILED_TO_VERIFY_SIGNATURE');
234+
}
235+
} else if (verified) {
236+
// extractor depends on signed content
236237
extractorFields = getDefaultExtractorFields(parserType, verifiedAssertionNode);
237238
} else {
238-
return Promise.reject('ERR_FAIL_TO_VERIFY_STE_SIGNATURE');
239+
return Promise.reject('FAILED_TO_VERIFY_SIGNATURE');
239240
}
240241
}
241242

src/libsaml.ts

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ const libSaml = () => {
366366
* - The first element is `true` if the signature is valid, `false` otherwise.
367367
* - The second element is the cryptographically authenticated assertion node as a string, or `null` if not found.
368368
*/
369-
verifySignature(xml: string, opts: SignatureVerifierOptions) {
369+
verifySignature(xml: string, opts: SignatureVerifierOptions) : [boolean, string | null] {
370370
const { dom } = getContext();
371371
const doc = dom.parseFromString(xml);
372372

@@ -395,10 +395,9 @@ const libSaml = () => {
395395

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

401-
402401
// need to refactor later on
403402
for (const signatureNode of selection){
404403
const sig = new SignedXml();
@@ -457,44 +456,50 @@ const libSaml = () => {
457456

458457
sig.loadSignature(signatureNode);
459458

460-
doc.removeChild(signatureNode);
461-
462459
verified = sig.checkSignature(doc.toString());
463460

464461
// immediately throw error when any one of the signature is failed to get verified
465462
if (!verified) {
466-
throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE');
463+
continue;
464+
// throw new Error('ERR_FAILED_TO_VERIFY_SIGNATURE');
467465
}
468-
// attempt is made to get the signed Reference as a string();
469-
// note, we don't have access to the actual signedReferences API unfortunately
470-
// mainly a sanity check here for SAML. (Although ours would still be secure, if multiple references are used)
471-
if (!(sig.getReferences().length >= 1)) {
466+
// Require there to be at least one reference that was signed
467+
if (!(sig.getSignedReferences().length >= 1)) {
472468
throw new Error('NO_SIGNATURE_REFERENCES')
473469
}
474470
const signedVerifiedXML = sig.getSignedReferences()[0];
475471
const rootNode = docParser.parseFromString(signedVerifiedXML, 'text/xml').documentElement;
476472
// process the verified signature:
477473
// case 1, rootSignedDoc is a response:
478474
if (rootNode.localName === 'Response') {
479-
480475
// try getting the Xml from the first assertion
481476
const assertions = select(
482477
"./*[local-name()='Assertion']",
483478
rootNode
484479
);
480+
481+
const encryptedAssertions = select(
482+
"./*[local-name()='EncryptedAssertion']",
483+
rootNode
484+
);
485485
// now we can process the assertion as an assertion
486486
if (assertions.length === 1) {
487487
return [true, assertions[0].toString()];
488+
} else if (encryptedAssertions.length >= 1) {
489+
return [true, rootNode.toString()]; // we need to return a Response node, which will be decrypted later
490+
} else {
491+
// something has gone seriously wrong here.
492+
// we don't have any assertion to give back
493+
return [true, null]
488494
}
489495
} else if (rootNode.localName === 'Assertion') {
490496
return [true, rootNode.toString()];
491497
} else {
492498
return [true, null]; // signature is valid. But there is no assertion node here. It could be metadata node, hence return null
493499
}
494500
};
501+
return [false, null]; // we didn't verify anything, none of the signatures are valid
495502

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

499504
/*
500505
// response must be signed, either entire document or assertion

test/flow.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1199,7 +1199,7 @@ test('should reject signature wrapped response - case 1', async t => {
11991199
}
12001200
});
12011201

1202-
test('should reject signature wrapped response - case 2', async t => {
1202+
test('should use signed contents in signature wrapped response - case 2', async t => {
12031203
//
12041204
const user = { email: '[email protected]' };
12051205
const { id, context: SAMLResponse } = await idpNoEncrypt.createLoginResponse(sp, sampleRequestInfo, 'post', user, createTemplateCallback(idpNoEncrypt, sp, binding.post, user));
@@ -1216,12 +1216,8 @@ test('should reject signature wrapped response - case 2', async t => {
12161216
//Put stripped version under SubjectConfirmationData of modified version
12171217
const xmlWrapped = outer.replace(/<\/saml:Conditions>/, '</saml:Conditions><saml:Advice>' + stripped.replace('<?xml version="1.0" encoding="UTF-8"?>', '') + '</saml:Advice>');
12181218
const wrappedResponse = Buffer.from(xmlWrapped).toString('base64');
1219-
try {
1220-
await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } });
1221-
t.fail();
1222-
} catch (e) {
1223-
t.is(e.message, 'ERR_POTENTIAL_WRAPPING_ATTACK');
1224-
}
1219+
const {extract} = await sp.parseLoginResponse(idpNoEncrypt, 'post', { body: { SAMLResponse: wrappedResponse } });
1220+
t.is(extract.nameID, '[email protected]');
12251221
});
12261222

12271223
test('should throw two-tiers code error when the response does not return success status', async t => {

test/index.ts

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -208,31 +208,22 @@ test('getAssertionConsumerService with two bindings', t => {
208208
t.is(libsaml.verifySignature(_decodedResponse, { metadata: IdPMetadata })[0], true);
209209
});
210210
test('integrity check for request signed with RSA-SHA1', t => {
211-
try {
212-
libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 });
213-
} catch (e) {
214-
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
215-
}
211+
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA1, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA1 });
212+
t.is(verified, false);
216213
});
217214
test('verify a XML signature signed by RSA-SHA256 with metadata', t => {
218215
t.is(libsaml.verifySignature(_decodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 })[0], true);
219216
});
220217
test('integrity check for request signed with RSA-SHA256', t => {
221-
try {
222-
libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 });
223-
} catch (e) {
224-
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
225-
}
218+
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA256, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA256 });
219+
t.is(verified, false);
226220
});
227221
test('verify a XML signature signed by RSA-SHA512 with metadata', t => {
228222
t.is(libsaml.verifySignature(_decodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 })[0], true);
229223
});
230224
test('integrity check for request signed with RSA-SHA512', t => {
231-
try {
232-
libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 });
233-
} catch (e) {
234-
t.is(e.message, 'ERR_FAILED_TO_VERIFY_SIGNATURE');
235-
}
225+
const [verified, verifiedData] = libsaml.verifySignature(_falseDecodedRequestSHA512, { metadata: SPMetadata, signatureAlgorithm: signatureAlgorithms.RSA_SHA512 });
226+
t.is(verified, false);
236227
});
237228

238229
test('verify a XML signature with metadata but with rolling certificate', t => {

test/issues.ts

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,12 +141,8 @@ test('#31 query param for sso/slo returns error', t => {
141141
});
142142

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

152148
test('#91 idp gets single sign on service from the metadata', t => {

yarn.lock

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,13 @@
1010
"@jridgewell/gen-mapping" "^0.1.0"
1111
"@jridgewell/trace-mapping" "^0.3.9"
1212

13+
"@authenio/samlify-xsd-schema-validator@^1.0.5":
14+
version "1.0.5"
15+
resolved "https://registry.yarnpkg.com/@authenio/samlify-xsd-schema-validator/-/samlify-xsd-schema-validator-1.0.5.tgz#bef7fe43928714e473cd95e1577b46d028b945ed"
16+
integrity sha512-HJjmjM1WbeB/z4nVbYEcmtIWTLPKqjrqRGEpC9lu7s03Usc4nxxfrJGjHgh3M8MvBJy4neVUoeM9rP4ym3GLgg==
17+
dependencies:
18+
"@authenio/xsd-schema-validator" "^0.7.3"
19+
1320
"@authenio/xml-encryption@^2.0.2":
1421
version "2.0.2"
1522
resolved "https://registry.yarnpkg.com/@authenio/xml-encryption/-/xml-encryption-2.0.2.tgz#df1f491dacb9b1f65bc7a9a554c189644f72bbe0"
@@ -19,6 +26,11 @@
1926
escape-html "^1.0.3"
2027
xpath "0.0.32"
2128

29+
"@authenio/xsd-schema-validator@^0.7.3":
30+
version "0.7.3"
31+
resolved "https://registry.yarnpkg.com/@authenio/xsd-schema-validator/-/xsd-schema-validator-0.7.3.tgz#abbf5710705bfab3394aca8b9d5a9e8429873897"
32+
integrity sha512-Jhc/Hxv90bacZr0Fv+u+PEb440zPh4mO6rw+bzEAIBiFLKCtRa/BvKGRxPdCAwsGRPuwl2hFqQGF+Lfz6Q8kFg==
33+
2234
"@ava/typescript@^1.1.1":
2335
version "1.1.1"
2436
resolved "https://registry.yarnpkg.com/@ava/typescript/-/typescript-1.1.1.tgz#3dcaba3aced8026fdb584d927d809752854dc6e6"
@@ -2522,10 +2534,10 @@ write-file-atomic@^4.0.1:
25222534
imurmurhash "^0.1.4"
25232535
signal-exit "^3.0.7"
25242536

2525-
xml-crypto@^6.1.0:
2526-
version "6.1.0"
2527-
resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.0.tgz#c8224808525e5f15478c50b9fe706112a4e6ef1b"
2528-
integrity sha512-0TYPBRPwXLnRGc2F0f9Zc/H076YcP7tkCa2US4jpguuPTEx7TWFqSysIfJ1hP4r2KF82IYzhnzepnsUEsOjlOw==
2537+
xml-crypto@^6.1.1:
2538+
version "6.1.2"
2539+
resolved "https://registry.yarnpkg.com/xml-crypto/-/xml-crypto-6.1.2.tgz#ed93e87d9538f92ad1ad2db442e9ec586723d07d"
2540+
integrity sha512-leBOVQdVi8FvPJrMYoum7Ici9qyxfE4kVi+AkpUoYCSXaQF4IlBm1cneTK9oAxR61LpYxTx7lNcsnBIeRpGW2w==
25292541
dependencies:
25302542
"@xmldom/is-dom-node" "^1.0.1"
25312543
"@xmldom/xmldom" "^0.8.10"

0 commit comments

Comments
 (0)