OpenCloud
/
MatrixSynapse
mirror of https://github.com/matrix-org/synapse
1
0
Fork 0
Code Issues Releases Wiki Activity

Rework UI Auth session validation for registration (#7455) 

Be less strict about validation of UI authentication sessions during
registration to match client expecations.
pull/7489/head
Patrick Cloke 2020-05-08 16:08:58 -04:00 committed by GitHub
parent aa5aa6f96a
commit 0ad6d28b0d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 291 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
changelog.d/7455.bugfix Normal file
View File

@ -0,0 +1 @@
Ensure that a user inteactive authentication session is tied to a single request.

36
synapse/handlers/auth.py
View File

@ -252,6 +252,7 @@ class AuthHandler(BaseHandler):
clientdict: Dict[str, Any],
clientip: str,
description: str,
validate_clientdict: bool = True,
) -> Tuple[dict, dict, str]:
"""
Takes a dictionary sent by the client in the login / registration
@ -277,6 +278,10 @@ class AuthHandler(BaseHandler):
description: A human readable string to be displayed to the user that
describes the operation happening on their account.
validate_clientdict: Whether to validate that the operation happening
on the account has not changed. If this is false,
the client dict is persisted instead of validated.
Returns:
A tuple of (creds, params, session_id).
@ -317,7 +322,9 @@ class AuthHandler(BaseHandler):
except StoreError:
raise SynapseError(400, "Unknown session ID: %s" % (sid,))
if not clientdict:
# If the client provides parameters, update what is persisted,
# otherwise use whatever was last provided.
#
# This was designed to allow the client to omit the parameters
# and just supply the session in subsequent calls so it split
# auth between devices by just sharing the session, (eg. so you
@ -325,22 +332,41 @@ class AuthHandler(BaseHandler):
# email auth link on there). It's probably too open to abuse
# because it lets unauthenticated clients store arbitrary objects
# on a homeserver.
#
# Revisit: Assuming the REST APIs do sensible validation, the data
# isn't arbitrary.
#
# Note that the registration endpoint explicitly removes the
# "initial_device_display_name" parameter if it is provided
# without a "password" parameter. See the changes to
# synapse.rest.client.v2_alpha.register.RegisterRestServlet.on_POST
# in commit 544722bad23fc31056b9240189c3cbbbf0ffd3f9.
if not clientdict:
clientdict = session.clientdict
# Ensure that the queried operation does not vary between stages of
# the UI authentication session. This is done by generating a stable
# comparator based on the URI, method, and body (minus the auth dict)
# and storing it during the initial query. Subsequent queries ensure
# that this comparator has not changed.
# comparator based on the URI, method, and client dict (minus the
# auth dict) and storing it during the initial query. Subsequent
# queries ensure that this comparator has not changed.
if validate_clientdict:
session_comparator = (session.uri, session.method, session.clientdict)
comparator = (uri, method, clientdict)
if (session.uri, session.method, session.clientdict) != comparator:
else:
session_comparator = (session.uri, session.method) # type: ignore
comparator = (uri, method) # type: ignore
if session_comparator != comparator:
raise SynapseError(
403,
"Requested operation has changed during the UI authentication session.",
)
# For backwards compatibility the registration endpoint persists
# changes to the client dict instead of validating them.
if not validate_clientdict:
await self.store.set_ui_auth_clientdict(sid, clientdict)
if not authdict:
raise InteractiveAuthIncompleteError(
self._auth_dict_for_flows(flows, session.session_id)

1
synapse/rest/client/v2_alpha/register.py
View File

@ -516,6 +516,7 @@ class RegisterRestServlet(RestServlet):
body,
self.hs.get_ip_from_request(request),
"register a new account",
validate_clientdict=False,
)
# Check that we're not trying to register a denied 3pid.

21
synapse/storage/data_stores/main/ui_auth.py
View File

@ -172,6 +172,27 @@ class UIAuthWorkerStore(SQLBaseStore):
return results
async def set_ui_auth_clientdict(
self, session_id: str, clientdict: JsonDict
) -> None:
"""
Store an updated clientdict for a given session ID.
Args:
session_id: The ID of this session as returned from check_auth
clientdict:
The dictionary from the client root level, not the 'auth' key.
"""
# The clientdict gets stored as JSON.
clientdict_json = json.dumps(clientdict)
self.db.simple_update_one(
table="ui_auth_sessions",
keyvalues={"session_id": session_id},
updatevalues={"clientdict": clientdict_json},
desc="set_ui_auth_client_dict",
)
async def set_ui_auth_session_data(self, session_id: str, key: str, value: Any):
"""
Store a key-value pair into the sessions data associated with this

326
tests/rest/client/v2_alpha/test_auth.py
View File

@ -12,16 +12,20 @@
# 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 List, Union
from twisted.internet.defer import succeed
import synapse.rest.admin
from synapse.api.constants import LoginType
from synapse.handlers.ui_auth.checkers import UserInteractiveAuthChecker
from synapse.rest.client.v2_alpha import auth, register
from synapse.http.site import SynapseRequest
from synapse.rest.client.v1 import login
from synapse.rest.client.v2_alpha import auth, devices, register
from synapse.types import JsonDict
from tests import unittest
from tests.server import FakeChannel
class DummyRecaptchaChecker(UserInteractiveAuthChecker):
@ -34,11 +38,15 @@ class DummyRecaptchaChecker(UserInteractiveAuthChecker):
return succeed(True)
class DummyPasswordChecker(UserInteractiveAuthChecker):
def check_auth(self, authdict, clientip):
return succeed(authdict["identifier"]["user"])
class FallbackAuthTests(unittest.HomeserverTestCase):
servlets = [
auth.register_servlets,
synapse.rest.admin.register_servlets_for_client_rest_resource,
register.register_servlets,
]
hijack_auth = False
@ -59,18 +67,51 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
auth_handler = hs.get_auth_handler()
auth_handler.checkers[LoginType.RECAPTCHA] = self.recaptcha_checker
@unittest.INFO
def test_fallback_captcha(self):
def register(self, expected_response: int, body: JsonDict) -> FakeChannel:
"""Make a register request."""
request, channel = self.make_request(
"POST", "register", body
) # type: SynapseRequest, FakeChannel
self.render(request)
self.assertEqual(request.code, expected_response)
return channel
def recaptcha(
self, session: str, expected_post_response: int, post_session: str = None
) -> None:
"""Get and respond to a fallback recaptcha. Returns the second request."""
if post_session is None:
post_session = session
request, channel = self.make_request(
"GET", "auth/m.login.recaptcha/fallback/web?session=" + session
) # type: SynapseRequest, FakeChannel
self.render(request)
self.assertEqual(request.code, 200)
request, channel = self.make_request(
"POST",
"register",
{"username": "user", "type": "m.login.password", "password": "bar"},
"auth/m.login.recaptcha/fallback/web?session="
+ post_session
+ "&g-recaptcha-response=a",
)
self.render(request)
self.assertEqual(request.code, expected_post_response)
# The recaptcha handler is called with the response given
attempts = self.recaptcha_checker.recaptcha_attempts
self.assertEqual(len(attempts), 1)
self.assertEqual(attempts[0][0]["response"], "a")
@unittest.INFO
def test_fallback_captcha(self):
"""Ensure that fallback auth via a captcha works."""
# Returns a 401 as per the spec
self.assertEqual(request.code, 401)
channel = self.register(
401, {"username": "user", "type": "m.login.password", "password": "bar"},
)
# Grab the session
session = channel.json_body["session"]
# Assert our configured public key is being given
@ -78,60 +119,32 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake"
)
request, channel = self.make_request(
"GET", "auth/m.login.recaptcha/fallback/web?session=" + session
)
self.render(request)
self.assertEqual(request.code, 200)
request, channel = self.make_request(
"POST",
"auth/m.login.recaptcha/fallback/web?session="
+ session
+ "&g-recaptcha-response=a",
)
self.render(request)
self.assertEqual(request.code, 200)
# The recaptcha handler is called with the response given
attempts = self.recaptcha_checker.recaptcha_attempts
self.assertEqual(len(attempts), 1)
self.assertEqual(attempts[0][0]["response"], "a")
# Complete the recaptcha step.
self.recaptcha(session, 200)
# also complete the dummy auth
request, channel = self.make_request(
"POST", "register", {"auth": {"session": session, "type": "m.login.dummy"}}
)
self.render(request)
self.register(200, {"auth": {"session": session, "type": "m.login.dummy"}})
# Now we should have fulfilled a complete auth flow, including
# the recaptcha fallback step, we can then send a
# request to the register API with the session in the authdict.
request, channel = self.make_request(
"POST", "register", {"auth": {"session": session}}
)
self.render(request)
self.assertEqual(channel.code, 200)
channel = self.register(200, {"auth": {"session": session}})
# We're given a registered user.
self.assertEqual(channel.json_body["user_id"], "@user:test")
def test_cannot_change_operation(self):
def test_legacy_registration(self):
"""
The initial requested operation cannot be modified during the user interactive authentication session.
Registration allows the parameters to vary through the process.
"""
# Make the initial request to register. (Later on a different password
# will be used.)
request, channel = self.make_request(
"POST",
"register",
{"username": "user", "type": "m.login.password", "password": "bar"},
)
self.render(request)
# Returns a 401 as per the spec
self.assertEqual(request.code, 401)
channel = self.register(
401, {"username": "user", "type": "m.login.password", "password": "bar"},
)
# Grab the session
session = channel.json_body["session"]
# Assert our configured public key is being given
@ -139,65 +152,39 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake"
)
request, channel = self.make_request(
"GET", "auth/m.login.recaptcha/fallback/web?session=" + session
)
self.render(request)
self.assertEqual(request.code, 200)
request, channel = self.make_request(
"POST",
"auth/m.login.recaptcha/fallback/web?session="
+ session
+ "&g-recaptcha-response=a",
)
self.render(request)
self.assertEqual(request.code, 200)
# The recaptcha handler is called with the response given
attempts = self.recaptcha_checker.recaptcha_attempts
self.assertEqual(len(attempts), 1)
self.assertEqual(attempts[0][0]["response"], "a")
# Complete the recaptcha step.
self.recaptcha(session, 200)
# also complete the dummy auth
request, channel = self.make_request(
"POST", "register", {"auth": {"session": session, "type": "m.login.dummy"}}
)
self.render(request)
self.register(200, {"auth": {"session": session, "type": "m.login.dummy"}})
# Now we should have fulfilled a complete auth flow, including
# the recaptcha fallback step. Make the initial request again, but
# with a different password. This causes the request to fail since the
# operaiton was modified during the ui auth session.
request, channel = self.make_request(
"POST",
"register",
# with a changed password. This still completes.
channel = self.register(
200,
{
"username": "user",
"type": "m.login.password",
"password": "foo", # Note this doesn't match the original request.
"password": "foo", # Note that this is different.
"auth": {"session": session},
},
)
self.render(request)
self.assertEqual(channel.code, 403)
# We're given a registered user.
self.assertEqual(channel.json_body["user_id"], "@user:test")
def test_complete_operation_unknown_session(self):
"""
Attempting to mark an invalid session as complete should error.
"""
# Make the initial request to register. (Later on a different password
# will be used.)
request, channel = self.make_request(
"POST",
"register",
{"username": "user", "type": "m.login.password", "password": "bar"},
)
self.render(request)
# Returns a 401 as per the spec
self.assertEqual(request.code, 401)
channel = self.register(
401, {"username": "user", "type": "m.login.password", "password": "bar"}
)
# Grab the session
session = channel.json_body["session"]
# Assert our configured public key is being given
@ -205,19 +192,160 @@ class FallbackAuthTests(unittest.HomeserverTestCase):
channel.json_body["params"]["m.login.recaptcha"]["public_key"], "brokencake"
)
request, channel = self.make_request(
"GET", "auth/m.login.recaptcha/fallback/web?session=" + session
)
self.render(request)
self.assertEqual(request.code, 200)
# Attempt to complete the recaptcha step with an unknown session.
# This results in an error.
self.recaptcha(session, 400, session + "unknown")
# Attempt to complete an unknown session, which should return an error.
unknown_session = session + "unknown"
class UIAuthTests(unittest.HomeserverTestCase):
servlets = [
auth.register_servlets,
devices.register_servlets,
login.register_servlets,
synapse.rest.admin.register_servlets_for_client_rest_resource,
register.register_servlets,
]
def prepare(self, reactor, clock, hs):
auth_handler = hs.get_auth_handler()
auth_handler.checkers[LoginType.PASSWORD] = DummyPasswordChecker(hs)
self.user_pass = "pass"
self.user = self.register_user("test", self.user_pass)
self.user_tok = self.login("test", self.user_pass)
def get_device_ids(self) -> List[str]:
# Get the list of devices so one can be deleted.
request, channel = self.make_request(
"POST",
"auth/m.login.recaptcha/fallback/web?session="
+ unknown_session
+ "&g-recaptcha-response=a",
)
"GET", "devices", access_token=self.user_tok,
) # type: SynapseRequest, FakeChannel
self.render(request)
self.assertEqual(request.code, 400)
# Get the ID of the device.
self.assertEqual(request.code, 200)
return [d["device_id"] for d in channel.json_body["devices"]]
def delete_device(
self, device: str, expected_response: int, body: Union[bytes, JsonDict] = b""
) -> FakeChannel:
"""Delete an individual device."""
request, channel = self.make_request(
"DELETE", "devices/" + device, body, access_token=self.user_tok
) # type: SynapseRequest, FakeChannel
self.render(request)
# Ensure the response is sane.
self.assertEqual(request.code, expected_response)
return channel
def delete_devices(self, expected_response: int, body: JsonDict) -> FakeChannel:
"""Delete 1 or more devices."""
# Note that this uses the delete_devices endpoint so that we can modify
# the payload half-way through some tests.
request, channel = self.make_request(
"POST", "delete_devices", body, access_token=self.user_tok,
) # type: SynapseRequest, FakeChannel
self.render(request)
# Ensure the response is sane.
self.assertEqual(request.code, expected_response)
return channel
def test_ui_auth(self):
"""
Test user interactive authentication outside of registration.
"""
device_id = self.get_device_ids()[0]
# Attempt to delete this device.
# Returns a 401 as per the spec
channel = self.delete_device(device_id, 401)
# Grab the session
session = channel.json_body["session"]
# Ensure that flows are what is expected.
self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
# Make another request providing the UI auth flow.
self.delete_device(
device_id,
200,
{
"auth": {
"type": "m.login.password",
"identifier": {"type": "m.id.user", "user": self.user},
"password": self.user_pass,
"session": session,
},
},
)
def test_cannot_change_body(self):
"""
The initial requested client dict cannot be modified during the user interactive authentication session.
"""
# Create a second login.
self.login("test", self.user_pass)
device_ids = self.get_device_ids()
self.assertEqual(len(device_ids), 2)
# Attempt to delete the first device.
# Returns a 401 as per the spec
channel = self.delete_devices(401, {"devices": [device_ids[0]]})
# Grab the session
session = channel.json_body["session"]
# Ensure that flows are what is expected.
self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
# Make another request providing the UI auth flow, but try to delete the
# second device. This results in an error.
self.delete_devices(
403,
{
"devices": [device_ids[1]],
"auth": {
"type": "m.login.password",
"identifier": {"type": "m.id.user", "user": self.user},
"password": self.user_pass,
"session": session,
},
},
)
def test_cannot_change_uri(self):
"""
The initial requested URI cannot be modified during the user interactive authentication session.
"""
# Create a second login.
self.login("test", self.user_pass)
device_ids = self.get_device_ids()
self.assertEqual(len(device_ids), 2)
# Attempt to delete the first device.
# Returns a 401 as per the spec
channel = self.delete_device(device_ids[0], 401)
# Grab the session
session = channel.json_body["session"]
# Ensure that flows are what is expected.
self.assertIn({"stages": ["m.login.password"]}, channel.json_body["flows"])
# Make another request providing the UI auth flow, but try to delete the
# second device. This results in an error.
self.delete_device(
device_ids[1],
403,
{
"auth": {
"type": "m.login.password",
"identifier": {"type": "m.id.user", "user": self.user},
"password": self.user_pass,
"session": session,
},
},
)

1
tox.ini
View File

@ -207,6 +207,7 @@ commands = mypy \
synapse/util/caches/stream_change_cache.py \
tests/replication/tcp/streams \
tests/test_utils \
tests/rest/client/v2_alpha/test_auth.py \
tests/util/test_stream_change_cache.py
# To find all folders that pass mypy you run: