Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 18 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/15388.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stable support for [MSC3882](https://github.com/matrix-org/matrix-spec-proposals/pull/3882) to allow an existing device/session to generate a login token for use on a new device/session.
64 changes: 42 additions & 22 deletions docs/usage/configuration/config_documentation.md
Original file line number Diff line number Diff line change
Expand Up @@ -2570,7 +2570,49 @@ Example configuration:
```yaml
nonrefreshable_access_token_lifetime: 24h
```
---
### `ui_auth`

The amount of time to allow a user-interactive authentication session to be active.

This defaults to 0, meaning the user is queried for their credentials
before every action, but this can be overridden to allow a single
validation to be re-used. This weakens the protections afforded by
the user-interactive authentication process, by allowing for multiple
(and potentially different) operations to use the same validation session.

This is ignored for potentially "dangerous" operations (including
deactivating an account, modifying an account password, adding a 3PID,
and minting additional login tokens).

Use the `session_timeout` sub-option here to change the time allowed for credential validation.

Example configuration:
```yaml
ui_auth:
session_timeout: "15s"
```
---
### `login_via_existing_session`

Matrix supports the ability of an existing session to mint a login token for
another client.

Synapse disables this by default as it has security ramifications.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where can we read more about these security ramifications (can we link it)? What is this hinting at?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should probably be spelled out more, but allowing an existing session to login new sessions is a pretty straightforward security concern. As the MSC says:

A malicious client could use the mechanism to spawn more than one session.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is that bad or different than the session the bad client already has?

More to clean-up?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you have access to someone's computer for a short period of time you may be able to create your own session that you transfer to a (physical) device you control or such.

@hughns hughns Jun 1, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But the default setting of login_via_existing_session.require_ui_auth means that UIA is required before a token is returned. So, the account takeover is limited that way.

As such, I would argue that login_via_existing_session.enabled could be true by default.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As such, I would argue that login_via_existing_session.enabled could be true by default.

I don't have strong feelings about it, I just defaulted to the safe way. I'd be fine changing this if you think it is fine to.


The duration of time the generated token is valid for can be configured with the
`token_timeout` sub-option.

User-interactive authentication is required when this is enabled unless the
`require_ui_auth` sub-option is set to `False`.

Example configuration:
```yaml
login_via_existing_session:
enabled: true
require_ui_auth: false
token_timeout: "5m"
```
---
## Metrics
Config options related to metrics.
Expand Down Expand Up @@ -3415,28 +3457,6 @@ password_config:
require_uppercase: true
```
---
### `ui_auth`

The amount of time to allow a user-interactive authentication session to be active.

This defaults to 0, meaning the user is queried for their credentials
before every action, but this can be overridden to allow a single
validation to be re-used. This weakens the protections afforded by
the user-interactive authentication process, by allowing for multiple
(and potentially different) operations to use the same validation session.

This is ignored for potentially "dangerous" operations (including
deactivating an account, modifying an account password, and
adding a 3PID).

Use the `session_timeout` sub-option here to change the time allowed for credential validation.

Example configuration:
```yaml
ui_auth:
session_timeout: "15s"
```
---
## Push
Configuration settings related to push notifications

Expand Down
10 changes: 10 additions & 0 deletions synapse/config/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,13 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
self.ui_auth_session_timeout = self.parse_duration(
ui_auth.get("session_timeout", 0)
)

# Logging in with an existing session.
login_via_existing = config.get("login_via_existing_session", {})
self.login_via_existing_enabled = login_via_existing.get("enabled", False)
self.login_via_existing_require_ui_auth = login_via_existing.get(
"require_ui_auth", True
)
self.login_via_existing_token_timeout = self.parse_duration(
login_via_existing.get("token_timeout", "5m")
)
13 changes: 3 additions & 10 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,10 @@ def check_config_conflicts(self, root: RootConfig) -> None:
("captcha", "enable_registration_captcha"),
)

if root.experimental.msc3882_enabled:
if root.auth.login_via_existing_enabled:
raise ConfigError(
"MSC3882 cannot be enabled when OAuth delegation is enabled",
("experimental_features", "msc3882_enabled"),
"Login via existing session cannot be enabled when OAuth delegation is enabled",
("login_via_existing_session", "enabled"),
)

if root.registration.refresh_token_lifetime:
Expand Down Expand Up @@ -319,13 +319,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# MSC3881: Remotely toggle push notifications for another client
self.msc3881_enabled: bool = experimental.get("msc3881_enabled", False)

# MSC3882: Allow an existing session to sign in a new session
self.msc3882_enabled: bool = experimental.get("msc3882_enabled", False)
self.msc3882_ui_auth: bool = experimental.get("msc3882_ui_auth", True)
self.msc3882_token_timeout = self.parse_duration(
experimental.get("msc3882_token_timeout", "5m")
)

# MSC3874: Filtering /messages with rel_types / not_rel_types.
self.msc3874_enabled: bool = experimental.get("msc3874_enabled", False)

Expand Down
3 changes: 3 additions & 0 deletions synapse/rest/client/capabilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
"m.3pid_changes": {
"enabled": self.config.registration.enable_3pid_changes
},
"m.get_login_token": {
"enabled": self.config.auth.login_via_existing_enabled,
},
}
}

Expand Down
23 changes: 15 additions & 8 deletions synapse/rest/client/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ def __init__(self, hs: "HomeServer"):
and hs.config.experimental.msc3866.require_approval_for_new_accounts
)

# Whether get login token is enabled.
self._get_login_token_enabled = hs.config.auth.login_via_existing_enabled

self.auth = hs.get_auth()

self.clock = hs.get_clock()
Expand Down Expand Up @@ -142,6 +145,9 @@ def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
# to SSO.
flows.append({"type": LoginRestServlet.CAS_TYPE})

# The login token flow requires m.login.token to be advertised.
support_login_token_flow = self._get_login_token_enabled

if self.cas_enabled or self.saml2_enabled or self.oidc_enabled:
flows.append(
{
Expand All @@ -153,14 +159,15 @@ def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
}
)

# While it's valid for us to advertise this login type generally,
# synapse currently only gives out these tokens as part of the
# SSO login flow.
# Generally we don't want to advertise login flows that clients
# don't know how to implement, since they (currently) will always
# fall back to the fallback API if they don't understand one of the
# login flow types returned.
Comment thread
clokep marked this conversation as resolved.
flows.append({"type": LoginRestServlet.TOKEN_TYPE})
# SSO requires a login token to be generated, so we need to advertise that flow
support_login_token_flow = True

if support_login_token_flow:
tokenTypeFlow: Dict[str, Any] = {"type": LoginRestServlet.TOKEN_TYPE}
# If the login token flow is enabled advertise the get_login_token flag.
if self._get_login_token_enabled:
tokenTypeFlow["get_login_token"] = True
flows.append(tokenTypeFlow)

flows.extend({"type": t} for t in self.auth_handler.get_supported_login_types())

Expand Down
44 changes: 31 additions & 13 deletions synapse/rest/client/login_token_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import logging
from typing import TYPE_CHECKING, Tuple

from synapse.api.ratelimiting import Ratelimiter
from synapse.http.server import HttpServer
from synapse.http.servlet import RestServlet, parse_json_object_from_request
from synapse.http.site import SynapseRequest
Expand All @@ -33,7 +34,7 @@ class LoginTokenRequestServlet(RestServlet):

Request:

POST /login/token HTTP/1.1
POST /login/get_token HTTP/1.1
Content-Type: application/json

{}
Expand All @@ -43,30 +44,42 @@ class LoginTokenRequestServlet(RestServlet):
HTTP/1.1 200 OK
{
"login_token": "ABDEFGH",
"expires_in": 3600,
"expires_in_ms": 3600000,
}
"""

PATTERNS = client_patterns(
"/org.matrix.msc3882/login/token$", releases=[], v1=False, unstable=True
)
PATTERNS = [
*client_patterns(
"/login/get_token$", releases=["v1"], v1=False, unstable=False
),
# TODO: this is no longer needed once unstable MSC3882 does not need to be supported:
*client_patterns(
"/org.matrix.msc3882/login/token$", releases=[], v1=False, unstable=True
),
]

def __init__(self, hs: "HomeServer"):
super().__init__()
self.auth = hs.get_auth()
self.store = hs.get_datastores().main
self.clock = hs.get_clock()
self.server_name = hs.config.server.server_name
self._main_store = hs.get_datastores().main
self.auth_handler = hs.get_auth_handler()
self.token_timeout = hs.config.experimental.msc3882_token_timeout
self.ui_auth = hs.config.experimental.msc3882_ui_auth
self.token_timeout = hs.config.auth.login_via_existing_token_timeout
self._require_ui_auth = hs.config.auth.login_via_existing_require_ui_auth

# An aggressive ratelimiter.
self._ratelimiter = Ratelimiter(
store=self._main_store,
clock=hs.get_clock(),
rate_hz=1 / 60,
burst_count=1,
)
Comment thread
clokep marked this conversation as resolved.
Outdated

@interactive_auth_handler
async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request)
body = parse_json_object_from_request(request)

if self.ui_auth:
if self._require_ui_auth:
await self.auth_handler.validate_user_via_ui_auth(
requester,
request,
Expand All @@ -75,21 +88,26 @@ async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
can_skip_ui_auth=False, # Don't allow skipping of UI auth
)

# Ensure that this endpoint isn't being used too often. (Ensure this is
# done *after* UI auth.)
await self._ratelimiter.ratelimit(None, requester.user.to_string().lower())

login_token = await self.auth_handler.create_login_token_for_user_id(
user_id=requester.user.to_string(),
auth_provider_id="org.matrix.msc3882.login_token_request",
duration_ms=self.token_timeout,
)

return (
200,
{
"login_token": login_token,
# TODO: this is no longer needed once unstable MSC3882 does not need to be supported:
"expires_in": self.token_timeout // 1000,
"expires_in_ms": self.token_timeout,
},
)


def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None:
if hs.config.experimental.msc3882_enabled:
if hs.config.auth.login_via_existing_enabled:
LoginTokenRequestServlet(hs).register(http_server)
4 changes: 2 additions & 2 deletions synapse/rest/client/versions.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ def on_GET(self, request: Request) -> Tuple[int, JsonDict]:
"fi.mau.msc2815": self.config.experimental.msc2815_enabled,
# Adds a ping endpoint for appservices to check HS->AS connection
"fi.mau.msc2659.stable": True, # TODO: remove when "v1.7" is added above
# Adds support for login token requests as per MSC3882
"org.matrix.msc3882": self.config.experimental.msc3882_enabled,
# TODO: this is no longer needed once unstable MSC3882 does not need to be supported:
"org.matrix.msc3882": self.config.auth.login_via_existing_enabled,
# Adds support for remotely enabling/disabling pushers, as per MSC3881
"org.matrix.msc3881": self.config.experimental.msc3881_enabled,
# Adds support for filtering /messages by event relation.
Expand Down
4 changes: 2 additions & 2 deletions tests/config/test_oauth_delegation.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,8 @@ def test_jwt_auth_cannot_be_enabled(self) -> None:
with self.assertRaises(ConfigError):
self.parse_config()

def test_msc3882_auth_cannot_be_enabled(self) -> None:
self.config_dict["experimental_features"]["msc3882_enabled"] = True
def test_login_via_existing_session_cannot_be_enabled(self) -> None:
self.config_dict["login_via_existing_session"] = {"enabled": True}
with self.assertRaises(ConfigError):
self.parse_config()

Expand Down
28 changes: 28 additions & 0 deletions tests/rest/client/test_capabilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,31 @@ def test_get_does_include_msc3244_fields_when_enabled(self) -> None:
self.assertGreater(len(details["support"]), 0)
for room_version in details["support"]:
self.assertTrue(room_version in KNOWN_ROOM_VERSIONS, str(room_version))

def test_get_get_token_login_fields_when_disabled(self) -> None:
"""By default login via an existing session is disabled."""
access_token = self.get_success(
self.auth_handler.create_access_token_for_user_id(
self.user, device_id=None, valid_until_ms=None
)
)

channel = self.make_request("GET", self.url, access_token=access_token)
capabilities = channel.json_body["capabilities"]

self.assertEqual(channel.code, HTTPStatus.OK)
self.assertFalse(capabilities["m.get_login_token"]["enabled"])

@override_config({"login_via_existing_session": {"enabled": True}})
def test_get_get_token_login_fields_when_enabled(self) -> None:
access_token = self.get_success(
self.auth_handler.create_access_token_for_user_id(
self.user, device_id=None, valid_until_ms=None
)
)

channel = self.make_request("GET", self.url, access_token=access_token)
capabilities = channel.json_body["capabilities"]

self.assertEqual(channel.code, HTTPStatus.OK)
self.assertTrue(capabilities["m.get_login_token"]["enabled"])
23 changes: 23 additions & 0 deletions tests/rest/client/test_login.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,29 @@ def test_require_approval(self) -> None:
ApprovalNoticeMedium.NONE, channel.json_body["approval_notice_medium"]
)

def test_get_login_flows_with_login_via_existing_disabled(self) -> None:
"""GET /login should return m.login.token without get_login_token true"""
channel = self.make_request("GET", "/_matrix/client/r0/login")
self.assertEqual(channel.code, 200, channel.result)

flows = {flow["type"]: flow for flow in channel.json_body["flows"]}
self.assertNotIn("m.login.token", flows)

@override_config({"login_via_existing_session": {"enabled": True}})
def test_get_login_flows_with_login_via_existing_enabled(self) -> None:
"""GET /login should return m.login.token without get_login_token true"""
Comment thread
clokep marked this conversation as resolved.
Outdated
channel = self.make_request("GET", "/_matrix/client/r0/login")
self.assertEqual(channel.code, 200, channel.result)

self.assertCountEqual(
channel.json_body["flows"],
[
{"type": "m.login.token", "get_login_token": True},
{"type": "m.login.password"},
{"type": "m.login.application_service"},
],
)


@skip_unless(has_saml2 and HAS_OIDC, "Requires SAML2 and OIDC")
class MultiSSOTestCase(unittest.HomeserverTestCase):
Expand Down
Loading