Implement login blocking based on SAML attributes #8052

richvdh · 2020-08-07T18:59:36Z

Hopefully this mostly speaks for itself. I also did a bit of cleaning up of the
error handling.

Hopefully this mostly speaks for itself. I also did a bit of cleaning up of the error handling. Fixes #8047

richvdh · 2020-08-07T19:06:41Z

synapse/config/_util.py

+                path.append(str(p))
+
+        raise ConfigError(
+            "Unable to parse configuration: %s at %s" % (e.message, ".".join(path))


this gives error messages like:

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

clokep · 2020-08-07T19:08:29Z

synapse/handlers/saml_handler.py

+        except saml2.response.UnsolicitedResponse:
+            raise SynapseError(400, "Unexpected SAML2 login.")


Will this make #7056 harder to solve? (Should we log the SAML session ID?)

fair. Have added a log line.

clokep · 2020-08-07T19:11:57Z

synapse/res/templates/saml_error.html

 </head>
 <body>
-    <p>Oops! Something went wrong during authentication<span id="errormsg"></span>.</p>
+{# a 403 means we have actively rejected their login #}


I looked briefly, but did you check if we need similar changes on the fallback login page?

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.

clokep

Looks great overall! I left a few questions, but I don't think any are hard-blockers for this.

clokep · 2020-08-10T14:55:30Z

synapse/config/_util.py

+    try:
+        jsonschema.validate(config, json_schema)
+    except jsonschema.ValidationError as e:
+        path = list(config_path)


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

it's to avoid modifying the inputs when we append to the list. will add a comment.

clokep · 2020-08-10T14:59:31Z

synapse/config/saml2_config.py

+          # `attribute_requirements` as shown below. All of the listed attributes must
+          # match for the login to be permitted.


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?

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.

That's reasonable. If we're confident we think we can extend it that sounds good.

clokep · 2020-08-10T15:01:00Z

synapse/handlers/saml_handler.py

+def _check_attribute_requirement(ava: dict, req: SamlAttributeRequirement):
+    values = ava.get(req.attribute, [])
+    for v in values:
+        if v == req.value:


Should we be ignoring whitespace here? (Or maybe the SAML2 code already strips all that out?)

I'm not entirely sure we should ignore whitespace, but yes pysaml2 strips leading and trailing whitespace.

@lugino-emeritus

Synapse 1.19.0rc1 (2020-08-13) ============================== Removal warning --------------- As outlined in the [previous release](https://github.com/matrix-org/synapse/releases/tag/v1.18.0), we are no longer publishing Docker images with the `-py3` tag suffix. On top of that, we have also removed the `latest-py3` tag. Please see [the announcement in the upgrade notes for 1.18.0](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). Features -------- - Add option to allow server admins to join rooms which fail complexity checks. Contributed by @lugino-emeritus. ([\#7902](#7902)) - Add an option to purge room or not with delete room admin endpoint (`POST /_synapse/admin/v1/rooms/<room_id>/delete`). Contributed by @dklimpel. ([\#7964](#7964)) - Add rate limiting to users joining rooms. ([\#8008](#8008)) - Add a `/health` endpoint to every configured HTTP listener that can be used as a health check endpoint by load balancers. ([\#8048](#8048)) - Allow login to be blocked based on the values of SAML attributes. ([\#8052](#8052)) - Allow guest access to the `GET /_matrix/client/r0/rooms/{room_id}/members` endpoint, according to MSC2689. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#7314](#7314)) Bugfixes -------- - Fix a bug introduced in Synapse v1.7.2 which caused inaccurate membership counts in the room directory. ([\#7977](#7977)) - Fix a long standing bug: 'Duplicate key value violates unique constraint "event_relations_id"' when message retention is configured. ([\#7978](#7978)) - Fix "no create event in auth events" when trying to reject invitation after inviter leaves. Bug introduced in Synapse v1.10.0. ([\#7980](#7980)) - Fix various comments and minor discrepencies in server notices code. ([\#7996](#7996)) - Fix a long standing bug where HTTP HEAD requests resulted in a 400 error. ([\#7999](#7999)) - Fix a long-standing bug which caused two copies of some log lines to be written when synctl was used along with a MemoryHandler logger. ([\#8011](#8011), [\#8012](#8012)) Updates to the Docker image --------------------------- - We no longer publish Docker images with the `-py3` tag suffix, as [announced in the upgrade notes](https://github.com/matrix-org/synapse/blob/develop/UPGRADE.rst#upgrading-to-v1180). ([\#8056](#8056)) Improved Documentation ---------------------- - Document how to set up a client .well-known file and fix several pieces of outdated documentation. ([\#7899](#7899)) - Improve workers docs. ([\#7990](#7990), [\#8000](#8000)) - Fix typo in `docs/workers.md`. ([\#7992](#7992)) - Add documentation for how to undo a room shutdown. ([\#7998](#7998), [\#8010](#8010)) Internal Changes ---------------- - Reduce the amount of whitespace in JSON stored and sent in responses. Contributed by David Vo. ([\#7372](#7372)) - Switch to the JSON implementation from the standard library and bump the minimum version of the canonicaljson library to 1.2.0. ([\#7936](#7936), [\#7979](#7979)) - Convert various parts of the codebase to async/await. ([\#7947](#7947), [\#7948](#7948), [\#7949](#7949), [\#7951](#7951), [\#7963](#7963), [\#7973](#7973), [\#7975](#7975), [\#7976](#7976), [\#7981](#7981), [\#7987](#7987), [\#7989](#7989), [\#8003](#8003), [\#8014](#8014), [\#8016](#8016), [\#8027](#8027), [\#8031](#8031), [\#8032](#8032), [\#8035](#8035), [\#8042](#8042), [\#8044](#8044), [\#8045](#8045), [\#8061](#8061), [\#8062](#8062), [\#8063](#8063), [\#8066](#8066), [\#8069](#8069), [\#8070](#8070)) - Move some database-related log lines from the default logger to the database/transaction loggers. ([\#7952](#7952)) - Add a script to detect source code files using non-unix line terminators. ([\#7965](#7965), [\#7970](#7970)) - Log the SAML session ID during creation. ([\#7971](#7971)) - Implement new experimental push rules for some users. ([\#7997](#7997)) - Remove redundant and unreliable signature check for v1 Identity Service lookup responses. ([\#8001](#8001)) - Improve the performance of the register endpoint. ([\#8009](#8009)) - Reduce less useful output in the newsfragment CI step. Add a link to the changelog section of the contributing guide on error. ([\#8024](#8024)) - Rename storage layer objects to be more sensible. ([\#8033](#8033)) - Change the default log config to reduce disk I/O and storage for new servers. ([\#8040](#8040)) - Add an assertion on `prev_events` in `create_new_client_event`. ([\#8041](#8041)) - Add a comment to `ServerContextFactory` about the use of `SSLv23_METHOD`. ([\#8043](#8043)) - Log `OPTIONS` requests at `DEBUG` rather than `INFO` level to reduce amount logged at `INFO`. ([\#8049](#8049)) - Reduce amount of outbound request logging at `INFO` level. ([\#8050](#8050)) - It is no longer necessary to explicitly define `filters` in the logging configuration. (Continuing to do so is redundant but harmless.) ([\#8051](#8051)) - Add and improve type hints. ([\#8058](#8058), [\#8064](#8064), [\#8060](#8060), [\#8067](#8067))

* commit 'db131b6b2': Change the default log config to reduce disk I/O and storage (#8040) Implement login blocking based on SAML attributes (#8052) Add an assertion on prev_events in create_new_client_event (#8041) Typo Lint why mypy why Lint Incorporate review Incorporate review Fix PUT /pushrules to use the right rule IDs Back out the database hack and replace it with a temporary config setting Fix cache name Fix cache invalidation calls Lint Changelog Implement new experimental push rules with a database hack to enable them

Implement login blocking based on SAML attributes

0b9211c

Hopefully this mostly speaks for itself. I also did a bit of cleaning up of the error handling. Fixes #8047

richvdh force-pushed the rav/block_saml_loging branch from 13ee827 to 0b9211c Compare August 7, 2020 19:04

richvdh requested a review from a team August 7, 2020 19:05

richvdh commented Aug 7, 2020

View reviewed changes

clokep reviewed Aug 7, 2020

View reviewed changes

clokep self-requested a review August 10, 2020 12:37

clokep reviewed Aug 10, 2020

View reviewed changes

address review comments

c4ffd49

richvdh requested a review from a team August 11, 2020 14:27

clokep approved these changes Aug 11, 2020

View reviewed changes

richvdh merged commit 0cb1699 into develop Aug 11, 2020

richvdh deleted the rav/block_saml_loging branch August 11, 2020 15:08

hex-m mentioned this pull request Aug 31, 2020

SAML auth needs doc #5764

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement login blocking based on SAML attributes #8052

Implement login blocking based on SAML attributes #8052

richvdh commented Aug 7, 2020

richvdh Aug 7, 2020

clokep Aug 7, 2020

richvdh Aug 11, 2020

clokep Aug 7, 2020

richvdh Aug 11, 2020

clokep left a comment

clokep Aug 10, 2020

richvdh Aug 11, 2020

clokep Aug 10, 2020

richvdh Aug 11, 2020

clokep Aug 11, 2020

clokep Aug 10, 2020

richvdh Aug 11, 2020

		except saml2.response.UnsolicitedResponse:
		raise SynapseError(400, "Unexpected SAML2 login.")

		# `attribute_requirements` as shown below. All of the listed attributes must
		# match for the login to be permitted.

Implement login blocking based on SAML attributes #8052

Implement login blocking based on SAML attributes #8052

Conversation

richvdh commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment