Merge pull request #9471 from edx/jibsheet/third-party-auth-settings

Allow configuring sensitive third_party_auth settings via lms.auth.json

common/djangoapps/third_party_auth/admin.py

@@ -9,7 +9,17 @@ from config_models.admin import ConfigurationModelAdmin, KeyedConfigurationModel
 from .models import OAuth2ProviderConfig, SAMLProviderConfig, SAMLConfiguration, SAMLProviderData
 from .tasks import fetch_saml_metadata
 
-admin.site.register(OAuth2ProviderConfig, KeyedConfigurationModelAdmin)
+
+class OAuth2ProviderConfigAdmin(KeyedConfigurationModelAdmin):
+    """ Django Admin class for OAuth2ProviderConfig """
+    def get_list_display(self, request):
+        """ Don't show every single field in the admin change list """
+        return (
+            'name', 'enabled', 'backend_name', 'secondary', 'skip_registration_form',
+            'skip_email_verification', 'change_date', 'changed_by', 'edit_link',
+        )
+
+admin.site.register(OAuth2ProviderConfig, OAuth2ProviderConfigAdmin)
 
 
 class SAMLProviderConfigAdmin(KeyedConfigurationModelAdmin):
@@ -55,10 +65,12 @@ class SAMLConfigurationAdmin(ConfigurationModelAdmin):
 
     def key_summary(self, inst):
         """ Short summary of the key pairs configured """
-        if not inst.public_key or not inst.private_key:
+        public_key = inst.get_setting('SP_PUBLIC_CERT')
+        private_key = inst.get_setting('SP_PRIVATE_KEY')
+        if not public_key or not private_key:
             return u'<em>Key pair incomplete/missing</em>'
-        pub1, pub2 = inst.public_key[0:10], inst.public_key[-10:]
-        priv1, priv2 = inst.private_key[0:10], inst.private_key[-10:]
+        pub1, pub2 = public_key[0:10], public_key[-10:]
+        priv1, priv2 = private_key[0:10], private_key[-10:]
         return u'Public: {}…{}<br>Private: {}…{}'.format(pub1, pub2, priv1, priv2)
     key_summary.allow_tags = True
 

common/djangoapps/third_party_auth/models.py

@@ -178,7 +178,16 @@ class OAuth2ProviderConfig(ProviderConfig):
         )
     )
     key = models.TextField(blank=True, verbose_name="Client ID")
-    secret = models.TextField(blank=True, verbose_name="Client Secret")
+    secret = models.TextField(
+        blank=True,
+        verbose_name="Client Secret",
+        help_text=(
+            'For increased security, you can avoid storing this in your database by leaving '
+            ' this field blank and setting '
+            'SOCIAL_AUTH_OAUTH_SECRETS = {"(backend name)": "secret", ...} '
+            'in your instance\'s Django settings (or lms.auth.json)'
+        )
+    )
     other_settings = models.TextField(blank=True, help_text="Optional JSON object with advanced settings, if any.")
 
     class Meta(object):  # pylint: disable=missing-docstring
@@ -192,8 +201,13 @@ class OAuth2ProviderConfig(ProviderConfig):
 
     def get_setting(self, name):
         """ Get the value of a setting, or raise KeyError """
-        if name in ("KEY", "SECRET"):
-            return getattr(self, name.lower())
+        if name == "KEY":
+            return self.key
+        if name == "SECRET":
+            if self.secret:
+                return self.secret
+            # To allow instances to avoid storing secrets in the DB, the secret can also be set via Django:
+            return getattr(settings, 'SOCIAL_AUTH_OAUTH_SECRETS', {}).get(self.backend_name, '')
         if self.other_settings:
             other_settings = json.loads(self.other_settings)
             assert isinstance(other_settings, dict), "other_settings should be a JSON object (dictionary)"
@@ -310,10 +324,22 @@ class SAMLConfiguration(ConfigurationModel):
         help_text=(
             'To generate a key pair as two files, run '
             '"openssl req -new -x509 -days 3652 -nodes -out saml.crt -keyout saml.key". '
-            'Paste the contents of saml.key here.'
-        )
+            'Paste the contents of saml.key here. '
+            'For increased security, you can avoid storing this in your database by leaving '
+            'this field blank and setting it via the SOCIAL_AUTH_SAML_SP_PRIVATE_KEY setting '
+            'in your instance\'s Django settings (or lms.auth.json).'
+        ),
+        blank=True,
+    )
+    public_key = models.TextField(
+        help_text=(
+            'Public key certificate. '
+            'For increased security, you can avoid storing this in your database by leaving '
+            'this field blank and setting it via the SOCIAL_AUTH_SAML_SP_PUBLIC_CERT setting '
+            'in your instance\'s Django settings (or lms.auth.json).'
+        ),
+        blank=True,
     )
-    public_key = models.TextField(help_text="Public key certificate.")
     entity_id = models.CharField(max_length=255, default="http://saml.example.com", verbose_name="Entity ID")
     org_info_str = models.TextField(
         verbose_name="Organization Info",
@@ -360,9 +386,15 @@ class SAMLConfiguration(ConfigurationModel):
         if name == "SP_ENTITY_ID":
             return self.entity_id
         if name == "SP_PUBLIC_CERT":
-            return self.public_key
+            if self.public_key:
+                return self.public_key
+            # To allow instances to avoid storing keys in the DB, the key pair can also be set via Django:
+            return getattr(settings, 'SOCIAL_AUTH_SAML_SP_PUBLIC_CERT', '')
         if name == "SP_PRIVATE_KEY":
-            return self.private_key
+            if self.private_key:
+                return self.private_key
+            # To allow instances to avoid storing keys in the DB, the private key can also be set via Django:
+            return getattr(settings, 'SOCIAL_AUTH_SAML_SP_PRIVATE_KEY', '')
         other_config = json.loads(self.other_config_str)
         if name in ("TECHNICAL_CONTACT", "SUPPORT_CONTACT"):
             contact = {

common/djangoapps/third_party_auth/tests/test_provider.py

@@ -23,7 +23,7 @@ class RegistryTest(testutil.TestCase):
         enabled_providers = provider.Registry.enabled()
         self.assertEqual(len(enabled_providers), 1)
         self.assertEqual(enabled_providers[0].name, "Google")
-        self.assertEqual(enabled_providers[0].secret, "opensesame")
+        self.assertEqual(enabled_providers[0].get_setting("SECRET"), "opensesame")
 
         self.configure_google_provider(enabled=False)
         enabled_providers = provider.Registry.enabled()
@@ -32,7 +32,17 @@ class RegistryTest(testutil.TestCase):
         self.configure_google_provider(enabled=True, secret="alohomora")
         enabled_providers = provider.Registry.enabled()
         self.assertEqual(len(enabled_providers), 1)
-        self.assertEqual(enabled_providers[0].secret, "alohomora")
+        self.assertEqual(enabled_providers[0].get_setting("SECRET"), "alohomora")
+
+    def test_secure_configuration(self):
+        """ Test that some sensitive values can be configured via Django settings """
+        self.configure_google_provider(enabled=True, secret="")
+        enabled_providers = provider.Registry.enabled()
+        self.assertEqual(len(enabled_providers), 1)
+        self.assertEqual(enabled_providers[0].name, "Google")
+        self.assertEqual(enabled_providers[0].get_setting("SECRET"), "")
+        with self.settings(SOCIAL_AUTH_OAUTH_SECRETS={'google-oauth2': 'secret42'}):
+            self.assertEqual(enabled_providers[0].get_setting("SECRET"), "secret42")
 
     def test_cannot_load_arbitrary_backends(self):
         """ Test that only backend_names listed in settings.AUTHENTICATION_BACKENDS can be used """

common/djangoapps/third_party_auth/tests/test_views.py

@@ -4,6 +4,7 @@ Test the views served by third_party_auth.
 # pylint: disable=no-member
 import ddt
 from lxml import etree
+from onelogin.saml2.errors import OneLogin_Saml2_Error
 import unittest
 from .testutil import AUTH_FEATURE_ENABLED, SAMLTestCase
 
@@ -26,8 +27,7 @@ class SAMLMetadataTest(SAMLTestCase):
         response = self.client.get(self.METADATA_URL)
         self.assertEqual(response.status_code, 404)
 
-    @ddt.data('saml_key', 'saml_key_alt')  # Test two slightly different key pair export formats
-    def test_metadata(self, key_name):
+    def test_metadata(self):
         self.enable_saml()
         doc = self._fetch_metadata()
         # Check the ACS URL:
@@ -62,13 +62,44 @@ class SAMLMetadataTest(SAMLTestCase):
             support_email="joe@example.com"
         )
 
-    def test_signed_metadata(self):
+    @ddt.data(
+        # Test two slightly different key pair export formats
+        ('saml_key', 'MIICsDCCAhmgAw'),
+        ('saml_key_alt', 'MIICWDCCAcGgAw'),
+    )
+    @ddt.unpack
+    def test_signed_metadata(self, key_name, pub_key_starts_with):
         self.enable_saml(
+            private_key=self._get_private_key(key_name),
+            public_key=self._get_public_key(key_name),
             other_config_str='{"SECURITY_CONFIG": {"signMetadata": true} }',
         )
+        self._validate_signed_metadata(pub_key_starts_with=pub_key_starts_with)
+
+    def test_secure_key_configuration(self):
+        """ Test that the SAML private key can be stored in Django settings and not the DB """
+        self.enable_saml(
+            public_key='',
+            private_key='',
+            other_config_str='{"SECURITY_CONFIG": {"signMetadata": true} }',
+        )
+        with self.assertRaises(OneLogin_Saml2_Error):
+            self._fetch_metadata()  # OneLogin_Saml2_Error: Cannot sign metadata: missing SP private key.
+        with self.settings(
+            SOCIAL_AUTH_SAML_SP_PRIVATE_KEY=self._get_private_key('saml_key'),
+            SOCIAL_AUTH_SAML_SP_PUBLIC_CERT=self._get_public_key('saml_key'),
+        ):
+            self._validate_signed_metadata()
+
+    def _validate_signed_metadata(self, pub_key_starts_with='MIICsDCCAhmgAw'):
+        """ Fetch the SAML metadata and do some validation """
         doc = self._fetch_metadata()
         sig_node = doc.find(".//{}".format(etree.QName(XMLDSIG_XML_NS, 'SignatureValue')))
         self.assertIsNotNone(sig_node)
+        # Check that the right public key was used:
+        pub_key_node = doc.find(".//{}".format(etree.QName(XMLDSIG_XML_NS, 'X509Certificate')))
+        self.assertIsNotNone(pub_key_node)
+        self.assertIn(pub_key_starts_with, pub_key_node.text)
 
     def _fetch_metadata(self):
         """ Fetch and parse the metadata XML at self.METADATA_URL """

lms/envs/aws.py

@@ -559,6 +559,15 @@ if FEATURES.get('ENABLE_THIRD_PARTY_AUTH'):
     # The reduced session expiry time during the third party login pipeline. (Value in seconds)
     SOCIAL_AUTH_PIPELINE_TIMEOUT = ENV_TOKENS.get('SOCIAL_AUTH_PIPELINE_TIMEOUT', 600)
 
+    # Most provider configuration is done via ConfigurationModels but for a few sensitive values
+    # we allow configuration via AUTH_TOKENS instead (optionally).
+    # The SAML private/public key values do not need the delimiter lines (such as
+    # "-----BEGIN PRIVATE KEY-----", "-----END PRIVATE KEY-----" etc.) but they may be included
+    # if you want (though it's easier to format the key values as JSON without the delimiters).
+    SOCIAL_AUTH_SAML_SP_PRIVATE_KEY = AUTH_TOKENS.get('SOCIAL_AUTH_SAML_SP_PRIVATE_KEY', '')
+    SOCIAL_AUTH_SAML_SP_PUBLIC_CERT = AUTH_TOKENS.get('SOCIAL_AUTH_SAML_SP_PUBLIC_CERT', '')
+    SOCIAL_AUTH_OAUTH_SECRETS = AUTH_TOKENS.get('SOCIAL_AUTH_OAUTH_SECRETS', {})
+
     # third_party_auth config moved to ConfigurationModels. This is for data migration only:
     THIRD_PARTY_AUTH_OLD_CONFIG = AUTH_TOKENS.get('THIRD_PARTY_AUTH', None)