openstack
/
keystone
Code Issues Proposed changes

Validate SAML keyfile & certfile options 

These options are not allowed to contain commas because we have to pass
them to the xmlsec1 binary as a comma-delimited pair. Despite the note
in the help text, there was no validation that the configuration was
usable before we tried to execute the resulting xmlsec1 command.

Change-Id: I88eba566bbe19fa7121e77ffd3f92e4f3d15bbcf
This commit is contained in:
Dolph Mathews 2016-07-13 19:26:36 +00:00
parent e5c816a4a2
commit 459dd8b287
2 changed files with 51 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
keystone/federation/idp.py
View File

@ -387,11 +387,11 @@ def _sign_assertion(assertion):
This method utilizes ``xmlsec1`` binary and signs SAML assertions in a
separate process. ``xmlsec1`` cannot read input data from stdin so the
prepared assertion needs to be serialized and stored in a temporary
file. This file will be deleted immediately after ``xmlsec1`` returns.
The signed assertion is redirected to a standard output and read using
subprocess.PIPE redirection. A ``saml.Assertion`` class is created
from the signed string again and returned.
prepared assertion needs to be serialized and stored in a temporary file.
This file will be deleted immediately after ``xmlsec1`` returns. The signed
assertion is redirected to a standard output and read using
``subprocess.PIPE`` redirection. A ``saml.Assertion`` class is created from
the signed string again and returned.
Parameters that are required in the CONF::
* xmlsec_binary
@ -400,18 +400,25 @@ def _sign_assertion(assertion):
:returns: XML <Assertion> object
"""
xmlsec_binary = CONF.saml.xmlsec1_binary
idp_private_key = CONF.saml.keyfile
idp_public_key = CONF.saml.certfile
# Ensure that the configured certificate paths do not contain any commas,
# before we string format a comma in between them and cause xmlsec1 to
# explode like a thousand fiery supernovas made entirely of unsigned SAML.
for option in ('keyfile', 'certfile'):
if ',' in getattr(CONF.saml, option, ''):
raise exception.UnexpectedError(_LE(
'The configuration value in `keystone.conf [saml] %s` cannot '
'contain a comma (`,`). Please fix your configuration.') %
option)
# xmlsec1 --sign --privkey-pem privkey,cert --id-attr:ID <tag> <file>
certificates = '%(idp_private_key)s,%(idp_public_key)s' % {
'idp_public_key': idp_public_key,
'idp_private_key': idp_private_key
'idp_public_key': CONF.saml.certfile,
'idp_private_key': CONF.saml.keyfile,
}
command_list = [xmlsec_binary, '--sign', '--privkey-pem', certificates,
'--id-attr:ID', 'Assertion']
command_list = [
CONF.saml.xmlsec1_binary, '--sign', '--privkey-pem', certificates,
'--id-attr:ID', 'Assertion']
file_path = None
try:

32
keystone/tests/unit/test_v3_federation.py
View File

@ -3004,6 +3004,38 @@ class SAMLGenerationTests(test_v3.RestfulTestCase):
self.assertEqual(self.PROJECT_DOMAIN,
project_domain_attribute.attribute_value[0].text)
def test_comma_in_certfile_path(self):
self.config_fixture.config(
group='saml',
certfile=CONF.saml.certfile + ',')
generator = keystone_idp.SAMLGenerator()
self.assertRaises(
exception.UnexpectedError,
generator.samlize_token,
self.ISSUER,
self.RECIPIENT,
self.SUBJECT,
self.SUBJECT_DOMAIN,
self.ROLES,
self.PROJECT,
self.PROJECT_DOMAIN)
def test_comma_in_keyfile_path(self):
self.config_fixture.config(
group='saml',
keyfile=CONF.saml.keyfile + ',')
generator = keystone_idp.SAMLGenerator()
self.assertRaises(
exception.UnexpectedError,
generator.samlize_token,
self.ISSUER,
self.RECIPIENT,
self.SUBJECT,
self.SUBJECT_DOMAIN,
self.ROLES,
self.PROJECT,
self.PROJECT_DOMAIN)
def test_verify_assertion_object(self):
"""Test that the Assertion object is built properly.