-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Implement login blocking based on SAML attributes #8052
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Allow login to be blocked based on the values of SAML attributes. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# -*- coding: utf-8 -*- | ||
# Copyright 2020 The Matrix.org Foundation C.I.C. | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# 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. | ||
from typing import Any, List | ||
|
||
import jsonschema | ||
|
||
from synapse.config._base import ConfigError | ||
from synapse.types import JsonDict | ||
|
||
|
||
def validate_config(json_schema: JsonDict, config: Any, config_path: List[str]) -> None: | ||
"""Validates a config setting against a JsonSchema definition | ||
This can be used to validate a section of the config file against a schema | ||
definition. If the validation fails, a ConfigError is raised with a textual | ||
description of the problem. | ||
Args: | ||
json_schema: the schema to validate against | ||
config: the configuration value to be validated | ||
config_path: the path within the config file. This will be used as a basis | ||
for the error message. | ||
""" | ||
try: | ||
jsonschema.validate(config, json_schema) | ||
except jsonschema.ValidationError as e: | ||
# copy `config_path` before modifying it. | ||
path = list(config_path) | ||
for p in list(e.path): | ||
if isinstance(p, int): | ||
path.append("<item %i>" % p) | ||
else: | ||
path.append(str(p)) | ||
|
||
raise ConfigError( | ||
"Unable to parse configuration: %s at %s" % (e.message, ".".join(path)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this gives error messages like:
|
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,14 +15,17 @@ | |
# limitations under the License. | ||
|
||
import logging | ||
from typing import Any, List | ||
|
||
import attr | ||
import jinja2 | ||
import pkg_resources | ||
|
||
from synapse.python_dependencies import DependencyException, check_requirements | ||
from synapse.util.module_loader import load_module, load_python_module | ||
|
||
from ._base import Config, ConfigError | ||
from ._util import validate_config | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -80,6 +83,11 @@ def read_config(self, config, **kwargs): | |
|
||
self.saml2_enabled = True | ||
|
||
attribute_requirements = saml2_config.get("attribute_requirements") or [] | ||
self.attribute_requirements = _parse_attribute_requirements_def( | ||
attribute_requirements | ||
) | ||
|
||
self.saml2_grandfathered_mxid_source_attribute = saml2_config.get( | ||
"grandfathered_mxid_source_attribute", "uid" | ||
) | ||
|
@@ -341,6 +349,17 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): | |
# | ||
#grandfathered_mxid_source_attribute: upn | ||
# It is possible to configure Synapse to only allow logins if SAML attributes | ||
# match particular values. The requirements can be listed under | ||
# `attribute_requirements` as shown below. All of the listed attributes must | ||
# match for the login to be permitted. | ||
Comment on lines
+354
to
+355
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see how this could be expanded a bit in the future to handle other types of matching (not-matching, regex matching, etc.) by adding a third property to each attribute / value. But I'm wondering if it will be limiting to require all to be matching. I'm not sure of a better / simpler way to do this though, but was this something you considered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we needed to support it, I'd do something like: attribute_requirements:
- match_type: any
matchers:
- match_type: all
matchers:
- { "attribute": "foo1", "value": "bar1" }
- { "attribute": "foo2", "value": "bar2" }
- match_type: all
matchers:
- { "attribute": "foo3", "value": "bar3" }
- { "attribute": "foo4", "value": "bar4" } ... which would mean I don't think it's worth over-thinking: I'm reasonably happy we could extend it if we needed to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's reasonable. If we're confident we think we can extend it that sounds good. |
||
# | ||
#attribute_requirements: | ||
# - attribute: userGroup | ||
# value: "staff" | ||
# - attribute: department | ||
# value: "sales" | ||
# Directory in which Synapse will try to find the template files below. | ||
# If not set, default templates from within the Synapse package will be used. | ||
# | ||
|
@@ -368,3 +387,34 @@ def generate_config_section(self, config_dir_path, server_name, **kwargs): | |
""" % { | ||
"config_dir_path": config_dir_path | ||
} | ||
|
||
|
||
@attr.s(frozen=True) | ||
class SamlAttributeRequirement: | ||
"""Object describing a single requirement for SAML attributes.""" | ||
|
||
attribute = attr.ib(type=str) | ||
value = attr.ib(type=str) | ||
|
||
JSON_SCHEMA = { | ||
"type": "object", | ||
"properties": {"attribute": {"type": "string"}, "value": {"type": "string"}}, | ||
"required": ["attribute", "value"], | ||
} | ||
|
||
|
||
ATTRIBUTE_REQUIREMENTS_SCHEMA = { | ||
"type": "array", | ||
"items": SamlAttributeRequirement.JSON_SCHEMA, | ||
} | ||
|
||
|
||
def _parse_attribute_requirements_def( | ||
attribute_requirements: Any, | ||
) -> List[SamlAttributeRequirement]: | ||
validate_config( | ||
ATTRIBUTE_REQUIREMENTS_SCHEMA, | ||
attribute_requirements, | ||
config_path=["saml2_config", "attribute_requirements"], | ||
) | ||
return [SamlAttributeRequirement(**x) for x in attribute_requirements] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,15 +14,16 @@ | |
# limitations under the License. | ||
import logging | ||
import re | ||
from typing import Callable, Dict, Optional, Set, Tuple | ||
from typing import TYPE_CHECKING, Callable, Dict, Optional, Set, Tuple | ||
|
||
import attr | ||
import saml2 | ||
import saml2.response | ||
from saml2.client import Saml2Client | ||
|
||
from synapse.api.errors import SynapseError | ||
from synapse.api.errors import AuthError, SynapseError | ||
from synapse.config import ConfigError | ||
from synapse.config.saml2_config import SamlAttributeRequirement | ||
from synapse.http.servlet import parse_string | ||
from synapse.http.site import SynapseRequest | ||
from synapse.module_api import ModuleApi | ||
|
@@ -34,6 +35,9 @@ | |
from synapse.util.async_helpers import Linearizer | ||
from synapse.util.iterutils import chunk_seq | ||
|
||
if TYPE_CHECKING: | ||
import synapse.server | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
|
@@ -49,7 +53,7 @@ class Saml2SessionData: | |
|
||
|
||
class SamlHandler: | ||
def __init__(self, hs): | ||
def __init__(self, hs: "synapse.server.HomeServer"): | ||
self._saml_client = Saml2Client(hs.config.saml2_sp_config) | ||
self._auth = hs.get_auth() | ||
self._auth_handler = hs.get_auth_handler() | ||
|
@@ -62,6 +66,7 @@ def __init__(self, hs): | |
self._grandfathered_mxid_source_attribute = ( | ||
hs.config.saml2_grandfathered_mxid_source_attribute | ||
) | ||
self._saml2_attribute_requirements = hs.config.saml2.attribute_requirements | ||
|
||
# plugin to do custom mapping from saml response to mxid | ||
self._user_mapping_provider = hs.config.saml2_user_mapping_provider_class( | ||
|
@@ -73,7 +78,7 @@ def __init__(self, hs): | |
self._auth_provider_id = "saml" | ||
|
||
# a map from saml session id to Saml2SessionData object | ||
self._outstanding_requests_dict = {} | ||
self._outstanding_requests_dict = {} # type: Dict[str, Saml2SessionData] | ||
|
||
# a lock on the mappings | ||
self._mapping_lock = Linearizer(name="saml_mapping", clock=self._clock) | ||
|
@@ -165,11 +170,18 @@ async def _map_saml_response_to_user( | |
saml2.BINDING_HTTP_POST, | ||
outstanding=self._outstanding_requests_dict, | ||
) | ||
except saml2.response.UnsolicitedResponse as e: | ||
# the pysaml2 library helpfully logs an ERROR here, but neglects to log | ||
# the session ID. I don't really want to put the full text of the exception | ||
# in the (user-visible) exception message, so let's log the exception here | ||
# so we can track down the session IDs later. | ||
logger.warning(str(e)) | ||
raise SynapseError(400, "Unexpected SAML2 login.") | ||
except Exception as e: | ||
raise SynapseError(400, "Unable to parse SAML2 response: %s" % (e,)) | ||
raise SynapseError(400, "Unable to parse SAML2 response: %s." % (e,)) | ||
|
||
if saml2_auth.not_signed: | ||
raise SynapseError(400, "SAML2 response was not signed") | ||
raise SynapseError(400, "SAML2 response was not signed.") | ||
|
||
logger.debug("SAML2 response: %s", saml2_auth.origxml) | ||
for assertion in saml2_auth.assertions: | ||
|
@@ -188,6 +200,9 @@ async def _map_saml_response_to_user( | |
saml2_auth.in_response_to, None | ||
) | ||
|
||
for requirement in self._saml2_attribute_requirements: | ||
_check_attribute_requirement(saml2_auth.ava, requirement) | ||
|
||
remote_user_id = self._user_mapping_provider.get_remote_user_id( | ||
saml2_auth, client_redirect_url | ||
) | ||
|
@@ -294,6 +309,21 @@ def expire_sessions(self): | |
del self._outstanding_requests_dict[reqid] | ||
|
||
|
||
def _check_attribute_requirement(ava: dict, req: SamlAttributeRequirement): | ||
values = ava.get(req.attribute, []) | ||
for v in values: | ||
if v == req.value: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we be ignoring whitespace here? (Or maybe the SAML2 code already strips all that out?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely sure we should ignore whitespace, but yes pysaml2 strips leading and trailing whitespace. |
||
return | ||
|
||
logger.info( | ||
"SAML2 attribute %s did not match required value '%s' (was '%s')", | ||
req.attribute, | ||
req.value, | ||
values, | ||
) | ||
raise AuthError(403, "You are not authorized to log in here.") | ||
|
||
|
||
DOT_REPLACE_PATTERN = re.compile( | ||
("[^%s]" % (re.escape("".join(mxid_localpart_allowed_characters)),)) | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,17 @@ | |
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8"> | ||
<title>SSO error</title> | ||
<title>SSO login error</title> | ||
</head> | ||
<body> | ||
<p>Oops! Something went wrong during authentication<span id="errormsg"></span>.</p> | ||
{# a 403 means we have actively rejected their login #} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked briefly, but did you check if we need similar changes on the fallback login page? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think any of the fallback flows are involved here. If we throw a 403 here, we never redirect back to the fallback flows. |
||
{% if code == 403 %} | ||
<p>You are not allowed to log in here.</p> | ||
{% else %} | ||
<p> | ||
There was an error during authentication: | ||
</p> | ||
<div id="errormsg" style="margin:20px 80px">{{ msg }}</div> | ||
<p> | ||
If you are seeing this page after clicking a link sent to you via email, make | ||
sure you only click the confirmation link once, and that you open the | ||
|
@@ -37,9 +44,9 @@ | |
// to print one. | ||
let errorDesc = new URLSearchParams(searchStr).get("error_description") | ||
if (errorDesc) { | ||
|
||
document.getElementById("errormsg").innerText = ` ("${errorDesc}")`; | ||
document.getElementById("errormsg").innerText = errorDesc; | ||
} | ||
</script> | ||
{% endif %} | ||
</body> | ||
</html> | ||
</html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the goal of converting to a list here? (That's what the input type is specified as, if we are expecting other types, can we document that?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's to avoid modifying the inputs when we append to the list. will add a comment.