Improve error checking for OIDC/SAML mapping providers (#8774)

Checks that the localpart returned by mapping providers for SAML and
OIDC are valid before registering new users.

Extends the OIDC tests for existing users and invalid data.

UPGRADE.rst

@@ -75,6 +75,36 @@ for example:
      wget https://packages.matrix.org/debian/pool/main/m/matrix-synapse-py3/matrix-synapse-py3_1.3.0+stretch1_amd64.deb
      dpkg -i matrix-synapse-py3_1.3.0+stretch1_amd64.deb
 
+Upgrading to v1.24.0
+====================
+
+Custom OpenID Connect mapping provider breaking change
+------------------------------------------------------
+
+This release allows the OpenID Connect mapping provider to perform normalisation
+of the localpart of the Matrix ID. This allows for the mapping provider to
+specify different algorithms, instead of the [default way](https://matrix.org/docs/spec/appendices#mapping-from-other-character-sets).
+
+If your Synapse configuration uses a custom mapping provider
+(`oidc_config.user_mapping_provider.module` is specified and not equal to
+`synapse.handlers.oidc_handler.JinjaOidcMappingProvider`) then you *must* ensure
+that `map_user_attributes` of the mapping provider performs some normalisation
+of the `localpart` returned. To match previous behaviour you can use the
+`map_username_to_mxid_localpart` function provided by Synapse. An example is
+shown below:
+
+.. code-block:: python
+
+  from synapse.types import map_username_to_mxid_localpart
+
+  class MyMappingProvider:
+      def map_user_attributes(self, userinfo, token):
+          # ... your custom logic ...
+          sso_user_id = ...
+          localpart = map_username_to_mxid_localpart(sso_user_id)
+
+          return {"localpart": localpart}
+
 Upgrading to v1.23.0
 ====================
 

changelog.d/8774.misc

@@ -0,0 +1 @@
+Add additional error checking for OpenID Connect and SAML mapping providers.

docs/sso_mapping_providers.md

@@ -15,8 +15,15 @@ where SAML mapping providers come into play.
 SSO mapping providers are currently supported for OpenID and SAML SSO
 configurations. Please see the details below for how to implement your own.
 
+It is the responsibility of the mapping provider to normalise the SSO attributes
+and map them to a valid Matrix ID. The
+[specification for Matrix IDs](https://matrix.org/docs/spec/appendices#user-identifiers)
+has some information about what is considered valid. Alternately an easy way to
+ensure it is valid is to use a Synapse utility function:
+`synapse.types.map_username_to_mxid_localpart`.
+
 External mapping providers are provided to Synapse in the form of an external
-Python module. You can retrieve this module from [PyPi](https://pypi.org) or elsewhere,
+Python module. You can retrieve this module from [PyPI](https://pypi.org) or elsewhere,
 but it must be importable via Synapse (e.g. it must be in the same virtualenv
 as Synapse). The Synapse config is then modified to point to the mapping provider
 (and optionally provide additional configuration for it).

synapse/handlers/oidc_handler.py

@@ -38,7 +38,12 @@ from synapse.handlers._base import BaseHandler
 from synapse.handlers.sso import MappingException
 from synapse.http.site import SynapseRequest
 from synapse.logging.context import make_deferred_yieldable
-from synapse.types import JsonDict, UserID, map_username_to_mxid_localpart
+from synapse.types import (
+    JsonDict,
+    UserID,
+    contains_invalid_mxid_characters,
+    map_username_to_mxid_localpart,
+)
 from synapse.util import json_decoder
 
 if TYPE_CHECKING:
@@ -885,10 +890,12 @@ class OidcHandler(BaseHandler):
             "Retrieved user attributes from user mapping provider: %r", attributes
         )
 
-        if not attributes["localpart"]:
-            raise MappingException("localpart is empty")
-
-        localpart = map_username_to_mxid_localpart(attributes["localpart"])
+        localpart = attributes["localpart"]
+        if not localpart:
+            raise MappingException(
+                "Error parsing OIDC response: OIDC mapping provider plugin "
+                "did not return a localpart value"
+            )
 
         user_id = UserID(localpart, self.server_name).to_string()
         users = await self.store.get_users_by_id_case_insensitive(user_id)
@@ -908,6 +915,11 @@ class OidcHandler(BaseHandler):
                 # This mxid is taken
                 raise MappingException("mxid '{}' is already taken".format(user_id))
         else:
+            # Since the localpart is provided via a potentially untrusted module,
+            # ensure the MXID is valid before registering.
+            if contains_invalid_mxid_characters(localpart):
+                raise MappingException("localpart is invalid: %s" % (localpart,))
+
             # It's the first time this user is logging in and the mapped mxid was
             # not taken, register the user
             registered_user_id = await self._registration_handler.register_user(
@@ -1076,6 +1088,9 @@ class JinjaOidcMappingProvider(OidcMappingProvider[JinjaOidcMappingConfig]):
     ) -> UserAttribute:
         localpart = self._config.localpart_template.render(user=userinfo).strip()
 
+        # Ensure only valid characters are included in the MXID.
+        localpart = map_username_to_mxid_localpart(localpart)
+
         display_name = None  # type: Optional[str]
         if self._config.display_name_template is not None:
             display_name = self._config.display_name_template.render(

synapse/handlers/saml_handler.py

@@ -31,6 +31,7 @@ from synapse.http.site import SynapseRequest
 from synapse.module_api import ModuleApi
 from synapse.types import (
     UserID,
+    contains_invalid_mxid_characters,
     map_username_to_mxid_localpart,
     mxid_localpart_allowed_characters,
 )
@@ -318,6 +319,11 @@ class SamlHandler(BaseHandler):
                     "Unable to generate a Matrix ID from the SAML response"
                 )
 
+            # Since the localpart is provided via a potentially untrusted module,
+            # ensure the MXID is valid before registering.
+            if contains_invalid_mxid_characters(localpart):
+                raise MappingException("localpart is invalid: %s" % (localpart,))
+
             logger.info("Mapped SAML user to local part %s", localpart)
             registered_user_id = await self._registration_handler.register_user(
                 localpart=localpart,

synapse/types.py

@@ -317,14 +317,14 @@ mxid_localpart_allowed_characters = set(
 )
 
 
-def contains_invalid_mxid_characters(localpart):
+def contains_invalid_mxid_characters(localpart: str) -> bool:
     """Check for characters not allowed in an mxid or groupid localpart
 
     Args:
-        localpart (basestring): the localpart to be checked
+        localpart: the localpart to be checked
 
     Returns:
-        bool: True if there are any naughty characters
+        True if there are any naughty characters
     """
     return any(c not in mxid_localpart_allowed_characters for c in localpart)
 

tests/handlers/test_oidc.py

@@ -12,7 +12,6 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-
 import json
 from urllib.parse import parse_qs, urlparse
 
@@ -24,12 +23,8 @@ import pymacaroons
 from twisted.python.failure import Failure
 from twisted.web._newclient import ResponseDone
 
-from synapse.handlers.oidc_handler import (
-    MappingException,
-    OidcError,
-    OidcHandler,
-    OidcMappingProvider,
-)
+from synapse.handlers.oidc_handler import OidcError, OidcHandler, OidcMappingProvider
+from synapse.handlers.sso import MappingException
 from synapse.types import UserID
 
 from tests.unittest import HomeserverTestCase, override_config
@@ -132,14 +127,13 @@ class OidcHandlerTestCase(HomeserverTestCase):
 
         config = self.default_config()
         config["public_baseurl"] = BASE_URL
-        oidc_config = {}
-        oidc_config["enabled"] = True
-        oidc_config["client_id"] = CLIENT_ID
-        oidc_config["client_secret"] = CLIENT_SECRET
-        oidc_config["issuer"] = ISSUER
-        oidc_config["scopes"] = SCOPES
-        oidc_config["user_mapping_provider"] = {
-            "module": __name__ + ".TestMappingProvider",
+        oidc_config = {
+            "enabled": True,
+            "client_id": CLIENT_ID,
+            "client_secret": CLIENT_SECRET,
+            "issuer": ISSUER,
+            "scopes": SCOPES,
+            "user_mapping_provider": {"module": __name__ + ".TestMappingProvider"},
         }
 
         # Update this config with what's in the default config so that
@@ -705,13 +699,13 @@ class OidcHandlerTestCase(HomeserverTestCase):
     def test_map_userinfo_to_existing_user(self):
         """Existing users can log in with OpenID Connect when allow_existing_users is True."""
         store = self.hs.get_datastore()
-        user4 = UserID.from_string("@test_user_4:test")
+        user = UserID.from_string("@test_user:test")
         self.get_success(
-            store.register_user(user_id=user4.to_string(), password_hash=None)
+            store.register_user(user_id=user.to_string(), password_hash=None)
         )
         userinfo = {
-            "sub": "test4",
-            "username": "test_user_4",
+            "sub": "test",
+            "username": "test_user",
         }
         token = {}
         mxid = self.get_success(
@@ -719,4 +713,59 @@ class OidcHandlerTestCase(HomeserverTestCase):
                 userinfo, token, "user-agent", "10.10.10.10"
             )
         )
-        self.assertEqual(mxid, "@test_user_4:test")
+        self.assertEqual(mxid, "@test_user:test")
+
+        # Register some non-exact matching cases.
+        user2 = UserID.from_string("@TEST_user_2:test")
+        self.get_success(
+            store.register_user(user_id=user2.to_string(), password_hash=None)
+        )
+        user2_caps = UserID.from_string("@test_USER_2:test")
+        self.get_success(
+            store.register_user(user_id=user2_caps.to_string(), password_hash=None)
+        )
+
+        # Attempting to login without matching a name exactly is an error.
+        userinfo = {
+            "sub": "test2",
+            "username": "TEST_USER_2",
+        }
+        e = self.get_failure(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertTrue(
+            str(e.value).startswith(
+                "Attempted to login as '@TEST_USER_2:test' but it matches more than one user inexactly:"
+            )
+        )
+
+        # Logging in when matching a name exactly should work.
+        user2 = UserID.from_string("@TEST_USER_2:test")
+        self.get_success(
+            store.register_user(user_id=user2.to_string(), password_hash=None)
+        )
+
+        mxid = self.get_success(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            )
+        )
+        self.assertEqual(mxid, "@TEST_USER_2:test")
+
+    def test_map_userinfo_to_invalid_localpart(self):
+        """If the mapping provider generates an invalid localpart it should be rejected."""
+        userinfo = {
+            "sub": "test2",
+            "username": "föö",
+        }
+        token = {}
+        e = self.get_failure(
+            self.handler._map_userinfo_to_user(
+                userinfo, token, "user-agent", "10.10.10.10"
+            ),
+            MappingException,
+        )
+        self.assertEqual(str(e.value), "localpart is invalid: föö")