Merge pull request #69 from open-craft/edx4-metadata-key-fix

Allow metadata signing with SP key specified as config value, not file

Author: pitbulk

src/onelogin/saml2/settings.py

@@ -565,9 +565,21 @@ def get_sp_metadata(self):
         # Sign metadata
         if 'signMetadata' in self.__security and self.__security['signMetadata'] is not False:
             if self.__security['signMetadata'] is True:
-                key_file_name = 'sp.key'
-                cert_file_name = 'sp.crt'
+                # Use the SP's normal key to sign the metadata:
+                if not cert:
+                    raise OneLogin_Saml2_Error(
+                        'Cannot sign metadata: missing SP public key certificate.',
+                        OneLogin_Saml2_Error.PUBLIC_CERT_FILE_NOT_FOUND
+                    )
+                cert_metadata = cert
+                key_metadata = self.get_sp_key()
+                if not key_metadata:
+                    raise OneLogin_Saml2_Error(
+                        'Cannot sign metadata: missing SP private key.',
+                        OneLogin_Saml2_Error.PRIVATE_KEY_FILE_NOT_FOUND
+                    )
             else:
+                # Use a custom key to sign the metadata:
                 if ('keyFileName' not in self.__security['signMetadata'] or
                         'certFileName' not in self.__security['signMetadata']):
                     raise OneLogin_Saml2_Error(
@@ -576,30 +588,28 @@ def get_sp_metadata(self):
                     )
                 key_file_name = self.__security['signMetadata']['keyFileName']
                 cert_file_name = self.__security['signMetadata']['certFileName']
-            key_metadata_file = self.__paths['cert'] + key_file_name
-            cert_metadata_file = self.__paths['cert'] + cert_file_name
-
-            if not exists(key_metadata_file):
-                raise OneLogin_Saml2_Error(
-                    'Private key file not found: %s',
-                    OneLogin_Saml2_Error.PRIVATE_KEY_FILE_NOT_FOUND,
-                    key_metadata_file
-                )
+                key_metadata_file = self.__paths['cert'] + key_file_name
+                cert_metadata_file = self.__paths['cert'] + cert_file_name
 
-            if not exists(cert_metadata_file):
-                raise OneLogin_Saml2_Error(
-                    'Public cert file not found: %s',
-                    OneLogin_Saml2_Error.PUBLIC_CERT_FILE_NOT_FOUND,
-                    cert_metadata_file
-                )
-
-            f_metadata_key = open(key_metadata_file, 'r')
-            key_metadata = f_metadata_key.read()
-            f_metadata_key.close()
+                try:
+                    with open(key_metadata_file, 'r') as f_metadata_key:
+                        key_metadata = f_metadata_key.read()
+                except IOError:
+                    raise OneLogin_Saml2_Error(
+                        'Private key file not readable: %s',
+                        OneLogin_Saml2_Error.PRIVATE_KEY_FILE_NOT_FOUND,
+                        key_metadata_file
+                    )
 
-            f_metadata_cert = open(cert_metadata_file, 'r')
-            cert_metadata = f_metadata_cert.read()
-            f_metadata_cert.close()
+                try:
+                    with open(cert_metadata_file, 'r') as f_metadata_cert:
+                        cert_metadata = f_metadata_cert.read()
+                except IOError:
+                    raise OneLogin_Saml2_Error(
+                        'Public cert file not readable: %s',
+                        OneLogin_Saml2_Error.PUBLIC_CERT_FILE_NOT_FOUND,
+                        cert_metadata_file
+                    )
 
             metadata = OneLogin_Saml2_Metadata.sign_metadata(metadata, key_metadata, cert_metadata)
 

tests/src/OneLogin/saml2_tests/settings_test.py

@@ -7,6 +7,7 @@
 from os.path import dirname, join, exists, sep
 import unittest
 
+from onelogin.saml2.errors import OneLogin_Saml2_Error
 from onelogin.saml2.settings import OneLogin_Saml2_Settings
 from onelogin.saml2.utils import OneLogin_Saml2_Utils
 
@@ -371,8 +372,24 @@ def testGetSPMetadataSigned(self):
         if 'security' not in settings_info:
             settings_info['security'] = {}
         settings_info['security']['signMetadata'] = True
-        settings = OneLogin_Saml2_Settings(settings_info)
+        self.generateAndCheckMetadata(settings_info)
+
+        # Now try again with SP keys set directly in settings and not from files:
+        del settings_info['custom_base_path']
+        # Now the keys should not be found, so metadata generation won't work:
+        with self.assertRaises(OneLogin_Saml2_Error):
+            OneLogin_Saml2_Settings(settings_info).get_sp_metadata()
+        # Set the keys in the settings:
+        settings_info['sp']['x509cert'] = self.file_contents(join(self.data_path, 'customPath', 'certs', 'sp.crt'))
+        settings_info['sp']['privateKey'] = self.file_contents(join(self.data_path, 'customPath', 'certs', 'sp.key'))
+        self.generateAndCheckMetadata(settings_info)
 
+    def generateAndCheckMetadata(self, settings):
+        """
+        Helper method: Given some settings, generate metadata and validate it
+        """
+        if not isinstance(settings, OneLogin_Saml2_Settings):
+            settings = OneLogin_Saml2_Settings(settings)
         metadata = settings.get_sp_metadata()
         self.assertIn('<md:SPSSODescriptor', metadata)
         self.assertIn('entityID="http://stuff.com/endpoints/metadata.php"', metadata)
@@ -385,6 +402,7 @@ def testGetSPMetadataSigned(self):
         self.assertIn('<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>', metadata)
         self.assertIn('<ds:Reference', metadata)
         self.assertIn('<ds:KeyInfo><ds:X509Data><ds:X509Certificate>', metadata)
+        return metadata
 
     def testGetSPMetadataSignedNoMetadataCert(self):
         """
@@ -411,7 +429,7 @@ def testGetSPMetadataSignedNoMetadataCert(self):
             settings.get_sp_metadata()
             self.assertTrue(False)
         except Exception as e:
-            self.assertIn('Private key file not found', e.message)
+            self.assertIn('Private key file not readable', e.message)
 
         settings_info['security']['signMetadata'] = {
             'keyFileName': 'sp.key',
@@ -422,7 +440,7 @@ def testGetSPMetadataSignedNoMetadataCert(self):
             settings.get_sp_metadata()
             self.assertTrue(False)
         except Exception as e:
-            self.assertIn('Public cert file not found', e.message)
+            self.assertIn('Public cert file not readable', e.message)
 
         settings_info['security']['signMetadata'] = 'invalid_value'
         settings = OneLogin_Saml2_Settings(settings_info)