From 4a34322e19a7cbe074f36d0c50c4ee33d6e1ea19 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Mon, 13 Apr 2020 11:21:10 -0300 Subject: [PATCH 1/5] SAML assertion signature is not mandatory --- app/meteor-accounts-saml/server/saml_utils.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/meteor-accounts-saml/server/saml_utils.js b/app/meteor-accounts-saml/server/saml_utils.js index 5c68621d281b..c4e4655a96c2 100644 --- a/app/meteor-accounts-saml/server/saml_utils.js +++ b/app/meteor-accounts-saml/server/saml_utils.js @@ -329,7 +329,7 @@ SAML.prototype.validateSignature = function(xml, cert, signature) { return sig.checkSignature(xml); }; -SAML.prototype.validateSignatureChildren = function(xml, cert, parent) { +SAML.prototype.validateSignatureChildren = function(xml, cert, parent, mandatory = true) { const xpathSigQuery = ".//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']"; const signatures = xmlCrypto.xpath(parent, xpathSigQuery); let signature = null; @@ -347,6 +347,10 @@ SAML.prototype.validateSignatureChildren = function(xml, cert, parent) { signature = sign; } + if (!signature && !mandatory) { + return true; + } + return this.validateSignature(xml, cert, signature); }; @@ -355,7 +359,7 @@ SAML.prototype.validateResponseSignature = function(xml, cert, response) { }; SAML.prototype.validateAssertionSignature = function(xml, cert, assertion) { - return this.validateSignatureChildren(xml, cert, assertion); + return this.validateSignatureChildren(xml, cert, assertion, false); }; SAML.prototype.validateLogoutRequest = function(samlRequest, callback) { From fac7e33d41b01b11eef7d7ac4052de303ba0cf90 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Mon, 13 Apr 2020 11:25:40 -0300 Subject: [PATCH 2/5] Return false when no signature found --- app/meteor-accounts-saml/server/saml_utils.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/meteor-accounts-saml/server/saml_utils.js b/app/meteor-accounts-saml/server/saml_utils.js index c4e4655a96c2..21cb98195b10 100644 --- a/app/meteor-accounts-saml/server/saml_utils.js +++ b/app/meteor-accounts-saml/server/saml_utils.js @@ -347,8 +347,8 @@ SAML.prototype.validateSignatureChildren = function(xml, cert, parent, mandatory signature = sign; } - if (!signature && !mandatory) { - return true; + if (!signature) { + return !mandatory; } return this.validateSignature(xml, cert, signature); From 94885697123ae2dfb297c563605c3ae39052d913 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Mon, 13 Apr 2020 12:17:59 -0300 Subject: [PATCH 3/5] Add new setting to determine what saml signatures to validate --- .../server/saml_rocketchat.js | 15 +++++++ app/meteor-accounts-saml/server/saml_utils.js | 39 ++++++++++++------- packages/rocketchat-i18n/i18n/en.i18n.json | 6 +++ server/startup/migrations/index.js | 1 + server/startup/migrations/v183.js | 16 ++++++++ 5 files changed, 64 insertions(+), 13 deletions(-) create mode 100644 server/startup/migrations/v183.js diff --git a/app/meteor-accounts-saml/server/saml_rocketchat.js b/app/meteor-accounts-saml/server/saml_rocketchat.js index c64e00ef5919..52b0a5856db6 100644 --- a/app/meteor-accounts-saml/server/saml_rocketchat.js +++ b/app/meteor-accounts-saml/server/saml_rocketchat.js @@ -62,6 +62,19 @@ Meteor.methods({ multiline: true, i18nLabel: 'SAML_Custom_Public_Cert', }); + settings.add(`SAML_Custom_${ name }_signature_validation_type`, 'All', { + type: 'select', + values: [ + { key: 'None', i18nLabel: 'SAML_Custom_signature_validation_none' }, + { key: 'Response', i18nLabel: 'SAML_Custom_signature_validation_response' }, + { key: 'Assertion', i18nLabel: 'SAML_Custom_signature_validation_assertion' }, + { key: 'All', i18nLabel: 'SAML_Custom_signature_validation_all' }, + ], + group: 'SAML', + section: name, + i18nLabel: 'SAML_Custom_signature_validation_type', + i18nDescription: 'SAML_Custom_signature_validation_type_description', + }); settings.add(`SAML_Custom_${ name }_private_key`, '', { type: 'string', group: 'SAML', @@ -238,6 +251,7 @@ const getSamlConfigs = function(service) { // People often overlook the instruction to remove the header and footer of the certificate on this specific setting, so let's do it for them. cert: normalizeCert(settings.get(`${ service.key }_cert`)), }, + signatureValidationType: settings.get(`${ service.key }_signature_validation_type`), userDataFieldMap: settings.get(`${ service.key }_user_data_fieldmap`), allowedClockDrift: settings.get(`${ service.key }_allowed_clock_drift`), }; @@ -290,6 +304,7 @@ const configureSamlService = function(samlConfigs) { roleAttributeName: samlConfigs.roleAttributeName, roleAttributeSync: samlConfigs.roleAttributeSync, allowedClockDrift: samlConfigs.allowedClockDrift, + signatureValidationType: samlConfigs.signatureValidationType, }; }; diff --git a/app/meteor-accounts-saml/server/saml_utils.js b/app/meteor-accounts-saml/server/saml_utils.js index 21cb98195b10..5e9c2f52f76e 100644 --- a/app/meteor-accounts-saml/server/saml_utils.js +++ b/app/meteor-accounts-saml/server/saml_utils.js @@ -329,7 +329,7 @@ SAML.prototype.validateSignature = function(xml, cert, signature) { return sig.checkSignature(xml); }; -SAML.prototype.validateSignatureChildren = function(xml, cert, parent, mandatory = true) { +SAML.prototype.validateSignatureChildren = function(xml, cert, parent) { const xpathSigQuery = ".//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']"; const signatures = xmlCrypto.xpath(parent, xpathSigQuery); let signature = null; @@ -348,7 +348,7 @@ SAML.prototype.validateSignatureChildren = function(xml, cert, parent, mandatory } if (!signature) { - return !mandatory; + return false; } return this.validateSignature(xml, cert, signature); @@ -359,7 +359,7 @@ SAML.prototype.validateResponseSignature = function(xml, cert, response) { }; SAML.prototype.validateAssertionSignature = function(xml, cert, assertion) { - return this.validateSignatureChildren(xml, cert, assertion, false); + return this.validateSignatureChildren(xml, cert, assertion); }; SAML.prototype.validateLogoutRequest = function(samlRequest, callback) { @@ -568,19 +568,32 @@ SAML.prototype.verifySignatures = function(response, assertion, xml) { return; } - debugLog('Verify Document Signature'); - if (!this.validateResponseSignature(xml, this.options.cert, response)) { - debugLog('Document Signature WRONG'); - throw new Error('Invalid Signature'); + const signatureType = this.options.signatureValidationType; + + if (signatureType === 'None') { + return; } - debugLog('Document Signature OK'); - debugLog('Verify Assertion Signature'); - if (!this.validateAssertionSignature(xml, this.options.cert, assertion)) { - debugLog('Assertion Signature WRONG'); - throw new Error('Invalid Assertion signature'); + const checkResponse = signatureType === 'Response' || signatureType === 'All'; + const checkAssertion = signatureType === 'Assertion' || signatureType === 'All'; + + if (checkResponse) { + debugLog('Verify Document Signature'); + if (!this.validateResponseSignature(xml, this.options.cert, response)) { + debugLog('Document Signature WRONG'); + throw new Error('Invalid Signature'); + } + debugLog('Document Signature OK'); + } + + if (checkAssertion) { + debugLog('Verify Assertion Signature'); + if (!this.validateAssertionSignature(xml, this.options.cert, assertion)) { + debugLog('Assertion Signature WRONG'); + throw new Error('Invalid Assertion signature'); + } + debugLog('Assertion Signature OK'); } - debugLog('Assertion Signature OK'); }; SAML.prototype.getSubject = function(assertion) { diff --git a/packages/rocketchat-i18n/i18n/en.i18n.json b/packages/rocketchat-i18n/i18n/en.i18n.json index 39ea3a6ed4e6..e7480185edda 100644 --- a/packages/rocketchat-i18n/i18n/en.i18n.json +++ b/packages/rocketchat-i18n/i18n/en.i18n.json @@ -2931,6 +2931,12 @@ "SAML_Custom_Private_Key": "Private Key Contents", "SAML_Custom_Provider": "Custom Provider", "SAML_Custom_EMail_Field": "E-Mail field name", + "SAML_Custom_signature_validation_none": "No validation", + "SAML_Custom_signature_validation_all": "Validate All Signatures", + "SAML_Custom_signature_validation_assertion": "Validate Assertion Signature", + "SAML_Custom_signature_validation_response": "Validate Response Signature", + "SAML_Custom_signature_validation_type": "Signature Validation Type", + "SAML_Custom_signature_validation_type_description": "This setting will be ignored if not Custom Certificate is provided.", "SAML_Custom_user_data_fieldmap": "User Data Field Map", "SAML_Custom_user_data_fieldmap_description": "Configure how user account fields (like email) are populated from a record in SAML (once found).
As an example, `{\"cn\":\"name\", \"mail\":\"email\"}` will choose a person's human readable name from the cn attribute, and their email from the mail attribute.
Available fields in Rocket.Chat: `name`, `email` and `username`, everything else will be saved as `customFields`.
You can also use a regex to get the field value, like this: `{\"NameID\": { \"field\": \"username\", \"regex\": \"(.*)@.+$\"}, \"email\": \"email\"}`", "SAML_Custom_Username_Field": "Username field name", diff --git a/server/startup/migrations/index.js b/server/startup/migrations/index.js index 60ee4afe9979..948bc590af93 100644 --- a/server/startup/migrations/index.js +++ b/server/startup/migrations/index.js @@ -180,4 +180,5 @@ import './v179'; import './v180'; import './v181'; import './v182'; +import './v183'; import './xrun'; diff --git a/server/startup/migrations/v183.js b/server/startup/migrations/v183.js new file mode 100644 index 000000000000..a0ed13d85a6d --- /dev/null +++ b/server/startup/migrations/v183.js @@ -0,0 +1,16 @@ +import { Migrations } from '../../../app/migrations'; +import { Settings } from '../../../app/models/server'; + +Migrations.add({ + version: 183, + up() { + // Set SAML signature validation type to 'Response' + Settings.upsert({ + _id: 'SAML_Custom_Default_signature_validation_type', + }, { + $set: { + value: 'Response', + }, + }); + }, +}); From ab588e7723bbb8983b1ef1428095d6d787fa2439 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Tue, 14 Apr 2020 12:19:23 -0300 Subject: [PATCH 4/5] Adds 'Either' option to signature validation --- .../server/saml_rocketchat.js | 2 +- app/meteor-accounts-saml/server/saml_utils.js | 31 +++++++++++++------ packages/rocketchat-i18n/i18n/en.i18n.json | 2 +- server/startup/migrations/v183.js | 4 +-- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/app/meteor-accounts-saml/server/saml_rocketchat.js b/app/meteor-accounts-saml/server/saml_rocketchat.js index 52b0a5856db6..b8c788399527 100644 --- a/app/meteor-accounts-saml/server/saml_rocketchat.js +++ b/app/meteor-accounts-saml/server/saml_rocketchat.js @@ -65,9 +65,9 @@ Meteor.methods({ settings.add(`SAML_Custom_${ name }_signature_validation_type`, 'All', { type: 'select', values: [ - { key: 'None', i18nLabel: 'SAML_Custom_signature_validation_none' }, { key: 'Response', i18nLabel: 'SAML_Custom_signature_validation_response' }, { key: 'Assertion', i18nLabel: 'SAML_Custom_signature_validation_assertion' }, + { key: 'Either', i18nLabel: 'SAML_Custom_signature_validation_either' }, { key: 'All', i18nLabel: 'SAML_Custom_signature_validation_all' }, ], group: 'SAML', diff --git a/app/meteor-accounts-saml/server/saml_utils.js b/app/meteor-accounts-saml/server/saml_utils.js index 5e9c2f52f76e..532af1f00fa4 100644 --- a/app/meteor-accounts-saml/server/saml_utils.js +++ b/app/meteor-accounts-saml/server/saml_utils.js @@ -570,18 +570,20 @@ SAML.prototype.verifySignatures = function(response, assertion, xml) { const signatureType = this.options.signatureValidationType; - if (signatureType === 'None') { - return; - } - - const checkResponse = signatureType === 'Response' || signatureType === 'All'; - const checkAssertion = signatureType === 'Assertion' || signatureType === 'All'; + const checkEither = signatureType === 'Either'; + const checkResponse = signatureType === 'Response' || signatureType === 'All' || checkEither; + const checkAssertion = signatureType === 'Assertion' || signatureType === 'All' || checkEither; + let anyValidSignature = false; if (checkResponse) { debugLog('Verify Document Signature'); if (!this.validateResponseSignature(xml, this.options.cert, response)) { - debugLog('Document Signature WRONG'); - throw new Error('Invalid Signature'); + if (!checkEither) { + debugLog('Document Signature WRONG'); + throw new Error('Invalid Signature'); + } + } else { + anyValidSignature = true; } debugLog('Document Signature OK'); } @@ -589,11 +591,20 @@ SAML.prototype.verifySignatures = function(response, assertion, xml) { if (checkAssertion) { debugLog('Verify Assertion Signature'); if (!this.validateAssertionSignature(xml, this.options.cert, assertion)) { - debugLog('Assertion Signature WRONG'); - throw new Error('Invalid Assertion signature'); + if (!checkEither) { + debugLog('Assertion Signature WRONG'); + throw new Error('Invalid Assertion signature'); + } + } else { + anyValidSignature = true; } debugLog('Assertion Signature OK'); } + + if (checkEither && !anyValidSignature) { + debugLog('No Valid Signature'); + throw new Error('No valid SAML Signature found'); + } }; SAML.prototype.getSubject = function(assertion) { diff --git a/packages/rocketchat-i18n/i18n/en.i18n.json b/packages/rocketchat-i18n/i18n/en.i18n.json index e7480185edda..befb943aa8ab 100644 --- a/packages/rocketchat-i18n/i18n/en.i18n.json +++ b/packages/rocketchat-i18n/i18n/en.i18n.json @@ -2931,9 +2931,9 @@ "SAML_Custom_Private_Key": "Private Key Contents", "SAML_Custom_Provider": "Custom Provider", "SAML_Custom_EMail_Field": "E-Mail field name", - "SAML_Custom_signature_validation_none": "No validation", "SAML_Custom_signature_validation_all": "Validate All Signatures", "SAML_Custom_signature_validation_assertion": "Validate Assertion Signature", + "SAML_Custom_signature_validation_either": "Validate Either Signature", "SAML_Custom_signature_validation_response": "Validate Response Signature", "SAML_Custom_signature_validation_type": "Signature Validation Type", "SAML_Custom_signature_validation_type_description": "This setting will be ignored if not Custom Certificate is provided.", diff --git a/server/startup/migrations/v183.js b/server/startup/migrations/v183.js index a0ed13d85a6d..69f6fce29d9e 100644 --- a/server/startup/migrations/v183.js +++ b/server/startup/migrations/v183.js @@ -4,12 +4,12 @@ import { Settings } from '../../../app/models/server'; Migrations.add({ version: 183, up() { - // Set SAML signature validation type to 'Response' + // Set SAML signature validation type to 'Either' Settings.upsert({ _id: 'SAML_Custom_Default_signature_validation_type', }, { $set: { - value: 'Response', + value: 'Either', }, }); }, From b31af573b22872795c1398e5f11b55d60e1818a6 Mon Sep 17 00:00:00 2001 From: Diego Sampaio Date: Tue, 14 Apr 2020 13:02:05 -0300 Subject: [PATCH 5/5] Move migration to 184 --- server/startup/migrations/index.js | 2 +- server/startup/migrations/{v183.js => v184.js} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename server/startup/migrations/{v183.js => v184.js} (95%) diff --git a/server/startup/migrations/index.js b/server/startup/migrations/index.js index 948bc590af93..3bc74224f487 100644 --- a/server/startup/migrations/index.js +++ b/server/startup/migrations/index.js @@ -180,5 +180,5 @@ import './v179'; import './v180'; import './v181'; import './v182'; -import './v183'; +import './v184'; import './xrun'; diff --git a/server/startup/migrations/v183.js b/server/startup/migrations/v184.js similarity index 95% rename from server/startup/migrations/v183.js rename to server/startup/migrations/v184.js index 69f6fce29d9e..033cc48f4a68 100644 --- a/server/startup/migrations/v183.js +++ b/server/startup/migrations/v184.js @@ -2,7 +2,7 @@ import { Migrations } from '../../../app/migrations'; import { Settings } from '../../../app/models/server'; Migrations.add({ - version: 183, + version: 184, up() { // Set SAML signature validation type to 'Either' Settings.upsert({