From 932551e849f6be861da9a08a5b71fa29f2c48fe3 Mon Sep 17 00:00:00 2001 From: Robin Houston Date: Tue, 5 Feb 2019 16:38:09 +0000 Subject: [PATCH] Support verification of assertion issuer Add an entity_id option to IdentityProvider. If provided, assertions are required to have an `` that matches this value. This commit also adds tests, and bumps the minor version number. --- README.md | 1 + lib/saml2.coffee | 13 ++++- test/data/response_with_issuer.xml | 29 ++++++++++++ test/data/response_without_issuer.xml | 28 +++++++++++ test/saml2.coffee | 68 +++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 2 deletions(-) create mode 100644 test/data/response_with_issuer.xml create mode 100644 test/data/response_without_issuer.xml diff --git a/README.md b/README.md index f764798..3462568 100644 --- a/README.md +++ b/README.md @@ -193,6 +193,7 @@ An object that can contain the below options. All options are strings, unless s - `sso_login_url` - **Required** - Login url to use during a login request. - `sso_logout_url` - **Required** - Logout url to use during a logout request. - `certificates` - **Required** - (PEM format string or array of PEM format strings) - Certificate or certificates (array of certificate) for the identity provider. +- `entity_id` - (String) - the identifier of the identity provider. If provided, assertions are required to have an `` that matches this value. - `force_authn` - (Boolean) - If true, forces re-authentication of users even if the user has a SSO session with the [IdP](#IdentityProvider). This can also be configured on the [SP](#ServiceProvider) or on a per-method basis. - `sign_get_request` - (Boolean) - If true, signs the request. This can also be configured on the [[SP](#ServiceProvider) or on a per-method basis. - `allow_unencrypted_assertion` - (Boolean) - If true, allows unencrypted assertions. This can also be configured on the [SP](#ServiceProvider) or on a per-method basis. diff --git a/lib/saml2.coffee b/lib/saml2.coffee index defac31..10a8fe2 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -422,7 +422,7 @@ add_namespaces_to_child_assertions = (xml_string) -> # Takes a DOM of a saml_response, private keys with which to attempt decryption and the # certificate(s) of the identity provider that issued it and will return a user object containing # the attributes or an error if keys are incorrect or the response is invalid. -parse_authn_response = (saml_response, sp_private_keys, idp_certificates, allow_unencrypted, ignore_signature, require_session_index, cb) -> +parse_authn_response = (saml_response, sp_private_keys, idp_certificates, allow_unencrypted, ignore_signature, require_session_index, idp_entity_id, cb) -> user = {} async.waterfall [ @@ -466,6 +466,14 @@ parse_authn_response = (saml_response, sp_private_keys, idp_certificates, allow_ assertion_attributes = parse_assertion_attributes decrypted_assertion user = _.extend user, pretty_assertion_attributes(assertion_attributes) user = _.extend user, attributes: assertion_attributes + + if idp_entity_id + issuer = decrypted_assertion.getElementsByTagNameNS(XMLNS.SAML, 'Issuer') + if issuer.length < 1 + return cb_wf new Error("Assertion in the SAML Response did not have a required Issuer") + if issuer[0].textContent != idp_entity_id + return cb_wf new Error("Issuer in the Assertion in the SAML Response is wrong") + cb_wf null, { user } catch err return cb_wf err @@ -634,6 +642,7 @@ module.exports.ServiceProvider = options.allow_unencrypted_assertion, options.ignore_signature, options.require_session_index, + identity_provider.entity_id, cb_wf) when saml_response.getElementsByTagNameNS(XMLNS.SAMLP, 'LogoutResponse').length is 1 @@ -714,7 +723,7 @@ module.exports.ServiceProvider = module.exports.IdentityProvider = class IdentityProvider constructor: (options) -> - {@sso_login_url, @sso_logout_url, @certificates} = options + {@entity_id, @sso_login_url, @sso_logout_url, @certificates} = options @certificates = [ @certificates ] unless _.isArray(@certificates) @shared_options = _.pick(options, "force_authn", "sign_get_request", "allow_unencrypted_assertion") diff --git a/test/data/response_with_issuer.xml b/test/data/response_with_issuer.xml new file mode 100644 index 0000000..93d0b6b --- /dev/null +++ b/test/data/response_with_issuer.xml @@ -0,0 +1,29 @@ +PHNhbWxwOlJlc3BvbnNlIHhtbG5zOnNhbWxwPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6 +cHJvdG9jb2wiIElEPSJfMiIgVmVyc2lvbj0iMi4wIiBJblJlc3BvbnNlVG89Il8xIiBEZXN0aW5h +dGlvbj0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9hc3NlcnQiPgogICAgPHNhbWxwOlN0YXR1cz4K +ICAgICAgICA8c2FtbHA6U3RhdHVzQ29kZSBWYWx1ZT0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6 +Mi4wOnN0YXR1czpTdWNjZXNzIiAvPgogICAgPC9zYW1scDpTdGF0dXM+CiAgICA8QXNzZXJ0aW9u +IHhtbG5zPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiBJRD0iXzMiIElz +c3VlSW5zdGFudD0iMjAxNC0wMy0xMlQyMTozNTowNS4zOTJaIiBWZXJzaW9uPSIyLjAiPgogICAg +ICAgIDxEYXRhIElEPSJfNSI+VGhpcyBkYXRhIGhhcyBubyBtZWFuaW5nLjwvRGF0YT4KICAgICAg +ICA8SXNzdWVyPmh0dHBzOi8vaWRwLmV4YW1wbGUuY29tL21ldGFkYXRhLnhtbDwvSXNzdWVyPgog +ICAgICAgIDxTdWJqZWN0PgogICAgICAgICAgICA8TmFtZUlEIEZvcm1hdD0idXJuOm9hc2lzOm5h +bWVzOnRjOlNBTUw6Mi4wOm5hbWVpZC1mb3JtYXQ6dHJhbnNpZW50Ij50c3R1ZGVudDwvTmFtZUlE +PgogICAgICAgICAgICA8U3ViamVjdENvbmZpcm1hdGlvbiBNZXRob2Q9InVybjpvYXNpczpuYW1l +czp0YzpTQU1MOjIuMDpjbTpiZWFyZXIiPgogICAgICAgICAgICAgICAgPFN1YmplY3RDb25maXJt +YXRpb25EYXRhIEluUmVzcG9uc2VUbz0iXzQiIE5vdE9uT3JBZnRlcj0iMjAxNC0wMy0xMlQyMTo0 +MDowNS4zOTJaIiBSZWNpcGllbnQ9Imh0dHBzOi8vc3AuZXhhbXBsZS5jb20vYXNzZXJ0IiAvPgog +ICAgICAgICAgICA8L1N1YmplY3RDb25maXJtYXRpb24+CiAgICAgICAgPC9TdWJqZWN0PgogICAg +ICAgIDxDb25kaXRpb25zPgogICAgICAgICAgICA8QXVkaWVuY2VSZXN0cmljdGlvbj4KICAgICAg +ICAgICAgICAgIDxBdWRpZW5jZT5odHRwczovL3NwLmV4YW1wbGUuY29tL21ldGFkYXRhLnhtbDwv +QXVkaWVuY2U+CiAgICAgICAgICAgIDwvQXVkaWVuY2VSZXN0cmljdGlvbj4KICAgICAgICA8L0Nv +bmRpdGlvbnM+CiAgICAgICAgPEF0dHJpYnV0ZVN0YXRlbWVudD4KICAgICAgICAgICAgPEF0dHJp +YnV0ZSBOYW1lPSJodHRwOi8vc2NoZW1hcy54bWxzb2FwLm9yZy93cy8yMDA1LzA1L2lkZW50aXR5 +L2NsYWltcy9naXZlbm5hbWUiPgogICAgICAgICAgICAgICAgPEF0dHJpYnV0ZVZhbHVlPlRlc3Q8 +L0F0dHJpYnV0ZVZhbHVlPgogICAgICAgICAgICA8L0F0dHJpYnV0ZT4KICAgICAgICA8L0F0dHJp +YnV0ZVN0YXRlbWVudD4KICAgICAgICA8QXV0aG5TdGF0ZW1lbnQgQXV0aG5JbnN0YW50PSIyMDE0 +LTAzLTEyVDIxOjM1OjA1LjM1NFoiIFNlc3Npb25JbmRleD0iXzMiPgogICAgICAgICAgICA8QXV0 +aG5Db250ZXh0PgogICAgICAgICAgICAgICAgPEF1dGhuQ29udGV4dENsYXNzUmVmPnVybjpvYXNp +czpuYW1lczp0YzpTQU1MOjIuMDphYzpjbGFzc2VzOlBhc3N3b3JkUHJvdGVjdGVkVHJhbnNwb3J0 +PC9BdXRobkNvbnRleHRDbGFzc1JlZj4KICAgICAgICAgICAgPC9BdXRobkNvbnRleHQ+CiAgICAg +ICAgPC9BdXRoblN0YXRlbWVudD4KICAgIDwvQXNzZXJ0aW9uPgo8L3NhbWxwOlJlc3BvbnNlPgo= diff --git a/test/data/response_without_issuer.xml b/test/data/response_without_issuer.xml new file mode 100644 index 0000000..feeea62 --- /dev/null +++ b/test/data/response_without_issuer.xml @@ -0,0 +1,28 @@ +PHNhbWxwOlJlc3BvbnNlIHhtbG5zOnNhbWxwPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6 +cHJvdG9jb2wiIElEPSJfMiIgVmVyc2lvbj0iMi4wIiBJblJlc3BvbnNlVG89Il8xIiBEZXN0aW5h +dGlvbj0iaHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9hc3NlcnQiPgogICAgPHNhbWxwOlN0YXR1cz4K +ICAgICAgICA8c2FtbHA6U3RhdHVzQ29kZSBWYWx1ZT0idXJuOm9hc2lzOm5hbWVzOnRjOlNBTUw6 +Mi4wOnN0YXR1czpTdWNjZXNzIiAvPgogICAgPC9zYW1scDpTdGF0dXM+CiAgICA8QXNzZXJ0aW9u +IHhtbG5zPSJ1cm46b2FzaXM6bmFtZXM6dGM6U0FNTDoyLjA6YXNzZXJ0aW9uIiBJRD0iXzMiIElz +c3VlSW5zdGFudD0iMjAxNC0wMy0xMlQyMTozNTowNS4zOTJaIiBWZXJzaW9uPSIyLjAiPgogICAg +ICAgIDxEYXRhIElEPSJfNSI+VGhpcyBkYXRhIGhhcyBubyBtZWFuaW5nLjwvRGF0YT4KICAgICAg +ICA8U3ViamVjdD4KICAgICAgICAgICAgPE5hbWVJRCBGb3JtYXQ9InVybjpvYXNpczpuYW1lczp0 +YzpTQU1MOjIuMDpuYW1laWQtZm9ybWF0OnRyYW5zaWVudCI+dHN0dWRlbnQ8L05hbWVJRD4KICAg +ICAgICAgICAgPFN1YmplY3RDb25maXJtYXRpb24gTWV0aG9kPSJ1cm46b2FzaXM6bmFtZXM6dGM6 +U0FNTDoyLjA6Y206YmVhcmVyIj4KICAgICAgICAgICAgICAgIDxTdWJqZWN0Q29uZmlybWF0aW9u +RGF0YSBJblJlc3BvbnNlVG89Il80IiBOb3RPbk9yQWZ0ZXI9IjIwMTQtMDMtMTJUMjE6NDA6MDUu +MzkyWiIgUmVjaXBpZW50PSJodHRwczovL3NwLmV4YW1wbGUuY29tL2Fzc2VydCIgLz4KICAgICAg +ICAgICAgPC9TdWJqZWN0Q29uZmlybWF0aW9uPgogICAgICAgIDwvU3ViamVjdD4KICAgICAgICA8 +Q29uZGl0aW9ucz4KICAgICAgICAgICAgPEF1ZGllbmNlUmVzdHJpY3Rpb24+CiAgICAgICAgICAg +ICAgICA8QXVkaWVuY2U+aHR0cHM6Ly9zcC5leGFtcGxlLmNvbS9tZXRhZGF0YS54bWw8L0F1ZGll +bmNlPgogICAgICAgICAgICA8L0F1ZGllbmNlUmVzdHJpY3Rpb24+CiAgICAgICAgPC9Db25kaXRp +b25zPgogICAgICAgIDxBdHRyaWJ1dGVTdGF0ZW1lbnQ+CiAgICAgICAgICAgIDxBdHRyaWJ1dGUg +TmFtZT0iaHR0cDovL3NjaGVtYXMueG1sc29hcC5vcmcvd3MvMjAwNS8wNS9pZGVudGl0eS9jbGFp +bXMvZ2l2ZW5uYW1lIj4KICAgICAgICAgICAgICAgIDxBdHRyaWJ1dGVWYWx1ZT5UZXN0PC9BdHRy +aWJ1dGVWYWx1ZT4KICAgICAgICAgICAgPC9BdHRyaWJ1dGU+CiAgICAgICAgPC9BdHRyaWJ1dGVT +dGF0ZW1lbnQ+CiAgICAgICAgPEF1dGhuU3RhdGVtZW50IEF1dGhuSW5zdGFudD0iMjAxNC0wMy0x +MlQyMTozNTowNS4zNTRaIiBTZXNzaW9uSW5kZXg9Il8zIj4KICAgICAgICAgICAgPEF1dGhuQ29u +dGV4dD4KICAgICAgICAgICAgICAgIDxBdXRobkNvbnRleHRDbGFzc1JlZj51cm46b2FzaXM6bmFt +ZXM6dGM6U0FNTDoyLjA6YWM6Y2xhc3NlczpQYXNzd29yZFByb3RlY3RlZFRyYW5zcG9ydDwvQXV0 +aG5Db250ZXh0Q2xhc3NSZWY+CiAgICAgICAgICAgIDwvQXV0aG5Db250ZXh0PgogICAgICAgIDwv +QXV0aG5TdGF0ZW1lbnQ+CiAgICA8L0Fzc2VydGlvbj4KPC9zYW1scDpSZXNwb25zZT4K diff --git a/test/saml2.coffee b/test/saml2.coffee index 99ac4ee..c48598c 100644 --- a/test/saml2.coffee +++ b/test/saml2.coffee @@ -1166,3 +1166,71 @@ describe 'saml2', -> it 'can create metadata', (done) -> done() + + describe 'when an entity_id is provided for the IdP', -> + sp_options = + entity_id: 'https://sp.example.com/metadata.xml' + private_key: get_test_file('test.pem') + certificate: get_test_file('test.crt') + assert_endpoint: 'https://sp.example.com/assert' + + sp = new saml2.ServiceProvider sp_options + + it 'will raise an error if there is no issuer in the assertion', (done) -> + idp_options = + entity_id: 'https://idp.example.com/metadata.xml' + sso_login_url: 'https://idp.example.com/login' + sso_logout_url: 'https://idp.example.com/logout' + certificates: [ get_test_file('test.crt') ] + + idp = new saml2.IdentityProvider idp_options + + request_options = + ignore_signature: true + allow_unencrypted_assertion: true + request_body: + SAMLResponse: get_test_file("response_without_issuer.xml") + + sp.post_assert idp, request_options, (err, response) -> + assert (err instanceof Error), "Did not get expected error." + assert (/Assertion in the SAML Response did not have a required Issuer/.test(err.message)), "Unexpected error message:" + err.message + done() + + it 'will raise an error if the issuer doesn\'t match that in the IdentityProvider', (done) -> + idp_options = + entity_id: 'https://some-nonsense.example.com/metadata.xml' + sso_login_url: 'https://idp.example.com/login' + sso_logout_url: 'https://idp.example.com/logout' + certificates: [ get_test_file('test.crt') ] + + idp = new saml2.IdentityProvider idp_options + + request_options = + ignore_signature: true + allow_unencrypted_assertion: true + request_body: + SAMLResponse: get_test_file("response_with_issuer.xml") + + sp.post_assert idp, request_options, (err, response) -> + assert (err instanceof Error), "Did not get expected error." + assert (/Issuer in the Assertion in the SAML Response is wrong/.test(err.message)), "Unexpected error message:" + err.message + done() + + it 'will not raise an error if issuer in the assertion is correct', (done) -> + idp_options = + entity_id: 'https://idp.example.com/metadata.xml' + sso_login_url: 'https://idp.example.com/login' + sso_logout_url: 'https://idp.example.com/logout' + certificates: [ get_test_file('test.crt') ] + + idp = new saml2.IdentityProvider idp_options + + request_options = + ignore_signature: true + allow_unencrypted_assertion: true + request_body: + SAMLResponse: get_test_file("response_with_issuer.xml") + + sp.post_assert idp, request_options, (err, response) -> + assert !err + done()