forked from MirrorHub/synapse
Stop putting a time caveat on access tokens
The 'time' caveat on the access tokens was something of a lie, since we weren't enforcing it; more pertinently its presence stops us ever adding useful time caveats. Let's move in the right direction by not lying in our caveats.
This commit is contained in:
parent
b6146537d2
commit
1c4f05db41
|
@ -810,6 +810,10 @@ class Auth(object):
|
||||||
else:
|
else:
|
||||||
v.satisfy_general(lambda c: c.startswith("time < "))
|
v.satisfy_general(lambda c: c.startswith("time < "))
|
||||||
|
|
||||||
|
# access_tokens and refresh_tokens include a nonce for uniqueness: any
|
||||||
|
# value is acceptable
|
||||||
|
v.satisfy_general(lambda c: c.startswith("nonce = "))
|
||||||
|
|
||||||
v.verify(macaroon, self.hs.config.macaroon_secret_key)
|
v.verify(macaroon, self.hs.config.macaroon_secret_key)
|
||||||
|
|
||||||
def _verify_expiry(self, caveat):
|
def _verify_expiry(self, caveat):
|
||||||
|
|
|
@ -32,7 +32,6 @@ class RegistrationConfig(Config):
|
||||||
)
|
)
|
||||||
|
|
||||||
self.registration_shared_secret = config.get("registration_shared_secret")
|
self.registration_shared_secret = config.get("registration_shared_secret")
|
||||||
self.user_creation_max_duration = int(config["user_creation_max_duration"])
|
|
||||||
|
|
||||||
self.bcrypt_rounds = config.get("bcrypt_rounds", 12)
|
self.bcrypt_rounds = config.get("bcrypt_rounds", 12)
|
||||||
self.trusted_third_party_id_servers = config["trusted_third_party_id_servers"]
|
self.trusted_third_party_id_servers = config["trusted_third_party_id_servers"]
|
||||||
|
@ -55,11 +54,6 @@ class RegistrationConfig(Config):
|
||||||
# secret, even if registration is otherwise disabled.
|
# secret, even if registration is otherwise disabled.
|
||||||
registration_shared_secret: "%(registration_shared_secret)s"
|
registration_shared_secret: "%(registration_shared_secret)s"
|
||||||
|
|
||||||
# Sets the expiry for the short term user creation in
|
|
||||||
# milliseconds. For instance the bellow duration is two weeks
|
|
||||||
# in milliseconds.
|
|
||||||
user_creation_max_duration: 1209600000
|
|
||||||
|
|
||||||
# Set the number of bcrypt rounds used to generate password hash.
|
# Set the number of bcrypt rounds used to generate password hash.
|
||||||
# Larger numbers increase the work factor needed to generate the hash.
|
# Larger numbers increase the work factor needed to generate the hash.
|
||||||
# The default number of rounds is 12.
|
# The default number of rounds is 12.
|
||||||
|
|
|
@ -538,14 +538,15 @@ class AuthHandler(BaseHandler):
|
||||||
device_id)
|
device_id)
|
||||||
defer.returnValue(refresh_token)
|
defer.returnValue(refresh_token)
|
||||||
|
|
||||||
def generate_access_token(self, user_id, extra_caveats=None,
|
def generate_access_token(self, user_id, extra_caveats=None):
|
||||||
duration_in_ms=(60 * 60 * 1000)):
|
|
||||||
extra_caveats = extra_caveats or []
|
extra_caveats = extra_caveats or []
|
||||||
macaroon = self._generate_base_macaroon(user_id)
|
macaroon = self._generate_base_macaroon(user_id)
|
||||||
macaroon.add_first_party_caveat("type = access")
|
macaroon.add_first_party_caveat("type = access")
|
||||||
now = self.hs.get_clock().time_msec()
|
# Include a nonce, to make sure that each login gets a different
|
||||||
expiry = now + duration_in_ms
|
# access token.
|
||||||
macaroon.add_first_party_caveat("time < %d" % (expiry,))
|
macaroon.add_first_party_caveat("nonce = %s" % (
|
||||||
|
stringutils.random_string_with_symbols(16),
|
||||||
|
))
|
||||||
for caveat in extra_caveats:
|
for caveat in extra_caveats:
|
||||||
macaroon.add_first_party_caveat(caveat)
|
macaroon.add_first_party_caveat(caveat)
|
||||||
return macaroon.serialize()
|
return macaroon.serialize()
|
||||||
|
|
|
@ -369,7 +369,7 @@ class RegistrationHandler(BaseHandler):
|
||||||
defer.returnValue(data)
|
defer.returnValue(data)
|
||||||
|
|
||||||
@defer.inlineCallbacks
|
@defer.inlineCallbacks
|
||||||
def get_or_create_user(self, requester, localpart, displayname, duration_in_ms,
|
def get_or_create_user(self, requester, localpart, displayname,
|
||||||
password_hash=None):
|
password_hash=None):
|
||||||
"""Creates a new user if the user does not exist,
|
"""Creates a new user if the user does not exist,
|
||||||
else revokes all previous access tokens and generates a new one.
|
else revokes all previous access tokens and generates a new one.
|
||||||
|
@ -399,8 +399,7 @@ class RegistrationHandler(BaseHandler):
|
||||||
|
|
||||||
user = UserID(localpart, self.hs.hostname)
|
user = UserID(localpart, self.hs.hostname)
|
||||||
user_id = user.to_string()
|
user_id = user.to_string()
|
||||||
token = self.auth_handler().generate_access_token(
|
token = self.auth_handler().generate_access_token(user_id)
|
||||||
user_id, None, duration_in_ms)
|
|
||||||
|
|
||||||
if need_register:
|
if need_register:
|
||||||
yield self.store.register(
|
yield self.store.register(
|
||||||
|
|
|
@ -384,7 +384,6 @@ class CreateUserRestServlet(ClientV1RestServlet):
|
||||||
def __init__(self, hs):
|
def __init__(self, hs):
|
||||||
super(CreateUserRestServlet, self).__init__(hs)
|
super(CreateUserRestServlet, self).__init__(hs)
|
||||||
self.store = hs.get_datastore()
|
self.store = hs.get_datastore()
|
||||||
self.direct_user_creation_max_duration = hs.config.user_creation_max_duration
|
|
||||||
self.handlers = hs.get_handlers()
|
self.handlers = hs.get_handlers()
|
||||||
|
|
||||||
@defer.inlineCallbacks
|
@defer.inlineCallbacks
|
||||||
|
@ -418,18 +417,8 @@ class CreateUserRestServlet(ClientV1RestServlet):
|
||||||
if "displayname" not in user_json:
|
if "displayname" not in user_json:
|
||||||
raise SynapseError(400, "Expected 'displayname' key.")
|
raise SynapseError(400, "Expected 'displayname' key.")
|
||||||
|
|
||||||
if "duration_seconds" not in user_json:
|
|
||||||
raise SynapseError(400, "Expected 'duration_seconds' key.")
|
|
||||||
|
|
||||||
localpart = user_json["localpart"].encode("utf-8")
|
localpart = user_json["localpart"].encode("utf-8")
|
||||||
displayname = user_json["displayname"].encode("utf-8")
|
displayname = user_json["displayname"].encode("utf-8")
|
||||||
duration_seconds = 0
|
|
||||||
try:
|
|
||||||
duration_seconds = int(user_json["duration_seconds"])
|
|
||||||
except ValueError:
|
|
||||||
raise SynapseError(400, "Failed to parse 'duration_seconds'")
|
|
||||||
if duration_seconds > self.direct_user_creation_max_duration:
|
|
||||||
duration_seconds = self.direct_user_creation_max_duration
|
|
||||||
password_hash = user_json["password_hash"].encode("utf-8") \
|
password_hash = user_json["password_hash"].encode("utf-8") \
|
||||||
if user_json.get("password_hash") else None
|
if user_json.get("password_hash") else None
|
||||||
|
|
||||||
|
@ -438,7 +427,6 @@ class CreateUserRestServlet(ClientV1RestServlet):
|
||||||
requester=requester,
|
requester=requester,
|
||||||
localpart=localpart,
|
localpart=localpart,
|
||||||
displayname=displayname,
|
displayname=displayname,
|
||||||
duration_in_ms=(duration_seconds * 1000),
|
|
||||||
password_hash=password_hash
|
password_hash=password_hash
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
@ -61,14 +61,14 @@ class AuthTestCase(unittest.TestCase):
|
||||||
def verify_type(caveat):
|
def verify_type(caveat):
|
||||||
return caveat == "type = access"
|
return caveat == "type = access"
|
||||||
|
|
||||||
def verify_expiry(caveat):
|
def verify_nonce(caveat):
|
||||||
return caveat == "time < 8600000"
|
return caveat.startswith("nonce =")
|
||||||
|
|
||||||
v = pymacaroons.Verifier()
|
v = pymacaroons.Verifier()
|
||||||
v.satisfy_general(verify_gen)
|
v.satisfy_general(verify_gen)
|
||||||
v.satisfy_general(verify_user)
|
v.satisfy_general(verify_user)
|
||||||
v.satisfy_general(verify_type)
|
v.satisfy_general(verify_type)
|
||||||
v.satisfy_general(verify_expiry)
|
v.satisfy_general(verify_nonce)
|
||||||
v.verify(macaroon, self.hs.config.macaroon_secret_key)
|
v.verify(macaroon, self.hs.config.macaroon_secret_key)
|
||||||
|
|
||||||
def test_short_term_login_token_gives_user_id(self):
|
def test_short_term_login_token_gives_user_id(self):
|
||||||
|
|
|
@ -53,13 +53,12 @@ class RegistrationTestCase(unittest.TestCase):
|
||||||
|
|
||||||
@defer.inlineCallbacks
|
@defer.inlineCallbacks
|
||||||
def test_user_is_created_and_logged_in_if_doesnt_exist(self):
|
def test_user_is_created_and_logged_in_if_doesnt_exist(self):
|
||||||
duration_ms = 200
|
|
||||||
local_part = "someone"
|
local_part = "someone"
|
||||||
display_name = "someone"
|
display_name = "someone"
|
||||||
user_id = "@someone:test"
|
user_id = "@someone:test"
|
||||||
requester = create_requester("@as:test")
|
requester = create_requester("@as:test")
|
||||||
result_user_id, result_token = yield self.handler.get_or_create_user(
|
result_user_id, result_token = yield self.handler.get_or_create_user(
|
||||||
requester, local_part, display_name, duration_ms)
|
requester, local_part, display_name)
|
||||||
self.assertEquals(result_user_id, user_id)
|
self.assertEquals(result_user_id, user_id)
|
||||||
self.assertEquals(result_token, 'secret')
|
self.assertEquals(result_token, 'secret')
|
||||||
|
|
||||||
|
@ -71,12 +70,11 @@ class RegistrationTestCase(unittest.TestCase):
|
||||||
user_id=frank.to_string(),
|
user_id=frank.to_string(),
|
||||||
token="jkv;g498752-43gj['eamb!-5",
|
token="jkv;g498752-43gj['eamb!-5",
|
||||||
password_hash=None)
|
password_hash=None)
|
||||||
duration_ms = 200
|
|
||||||
local_part = "frank"
|
local_part = "frank"
|
||||||
display_name = "Frank"
|
display_name = "Frank"
|
||||||
user_id = "@frank:test"
|
user_id = "@frank:test"
|
||||||
requester = create_requester("@as:test")
|
requester = create_requester("@as:test")
|
||||||
result_user_id, result_token = yield self.handler.get_or_create_user(
|
result_user_id, result_token = yield self.handler.get_or_create_user(
|
||||||
requester, local_part, display_name, duration_ms)
|
requester, local_part, display_name)
|
||||||
self.assertEquals(result_user_id, user_id)
|
self.assertEquals(result_user_id, user_id)
|
||||||
self.assertEquals(result_token, 'secret')
|
self.assertEquals(result_token, 'secret')
|
||||||
|
|
Loading…
Reference in a new issue