Implement login blocking based on SAML attributes#8052
Conversation
Hopefully this mostly speaks for itself. I also did a bit of cleaning up of the error handling. Fixes #8047
13ee827 to
0b9211c
Compare
| path.append(str(p)) | ||
|
|
||
| raise ConfigError( | ||
| "Unable to parse configuration: %s at %s" % (e.message, ".".join(path)) |
There was a problem hiding this comment.
this gives error messages like:
ERROR: Unable to parse configuration: None is not of type 'string' at saml2_config.attribute_requirements.<item 0>.attribute
synapse/handlers/saml_handler.py
Outdated
| except saml2.response.UnsolicitedResponse: | ||
| raise SynapseError(400, "Unexpected SAML2 login.") |
There was a problem hiding this comment.
Will this make #7056 harder to solve? (Should we log the SAML session ID?)
There was a problem hiding this comment.
fair. Have added a log line.
| </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.
I looked briefly, but did you check if we need similar changes on the fallback login page?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Looks great overall! I left a few questions, but I don't think any are hard-blockers for this.
| try: | ||
| jsonschema.validate(config, json_schema) | ||
| except jsonschema.ValidationError as e: | ||
| path = list(config_path) |
There was a problem hiding this comment.
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.
it's to avoid modifying the inputs when we append to the list. will add a comment.
| # `attribute_requirements` as shown below. All of the listed attributes must | ||
| # match for the login to be permitted. |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
That's reasonable. If we're confident we think we can extend it that sounds good.
| 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.
Should we be ignoring whitespace here? (Or maybe the SAML2 code already strips all that out?)
There was a problem hiding this comment.
I'm not entirely sure we should ignore whitespace, but yes pysaml2 strips leading and trailing whitespace.
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
Hopefully this mostly speaks for itself. I also did a bit of cleaning up of the
error handling.
Fixes #8047.