Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Implement login blocking based on SAML attributes #8052

Merged
merged 2 commits into from
Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8052.feature
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.
11 changes: 11 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1577,6 +1577,17 @@ saml2_config:
#
#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.
#
#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.
#
Expand Down
49 changes: 49 additions & 0 deletions synapse/config/_util.py
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)
Copy link
Member

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?)

Copy link
Member Author

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.

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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gives error messages like:

ERROR: Unable to parse configuration: None is not of type 'string' at saml2_config.attribute_requirements.<item 0>.attribute

)
50 changes: 50 additions & 0 deletions synapse/config/saml2_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)

Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 (foo1 == bar1 && foo2 == bar2) || (foo3 == bar3 && foo4 == bar4).

I don't think it's worth over-thinking: I'm reasonably happy we could extend it if we needed to.

Copy link
Member

Choose a reason for hiding this comment

The 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.
#
Expand Down Expand Up @@ -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]
42 changes: 36 additions & 6 deletions synapse/handlers/saml_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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__)


Expand All @@ -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()
Expand All @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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:
Expand All @@ -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
)
Expand Down Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The 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?)

Copy link
Member Author

Choose a reason for hiding this comment

The 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)),))
)
Expand Down
17 changes: 12 additions & 5 deletions synapse/res/templates/saml_error.html
Original file line number Diff line number Diff line change
Expand Up @@ -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 #}
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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>