Merge pull request #1656 from matrix-org/rav/remove_time_caveat

Stop putting a time caveat on access tokens
This commit is contained in:
Richard van der Hoff 2016-11-30 16:53:20 +00:00 committed by GitHub
commit 321fe5c44c
7 changed files with 26 additions and 36 deletions

View File

@ -794,9 +794,6 @@ class Auth(object):
type_string(str): The kind of token required (e.g. "access", "refresh", type_string(str): The kind of token required (e.g. "access", "refresh",
"delete_pusher") "delete_pusher")
verify_expiry(bool): Whether to verify whether the macaroon has expired. verify_expiry(bool): Whether to verify whether the macaroon has expired.
This should really always be True, but there exist access tokens
in the wild which expire when they should not, so we can't
enforce expiry yet.
user_id (str): The user_id required user_id (str): The user_id required
""" """
v = pymacaroons.Verifier() v = pymacaroons.Verifier()
@ -809,11 +806,24 @@ class Auth(object):
v.satisfy_exact("type = " + type_string) v.satisfy_exact("type = " + type_string)
v.satisfy_exact("user_id = %s" % user_id) v.satisfy_exact("user_id = %s" % user_id)
v.satisfy_exact("guest = true") v.satisfy_exact("guest = true")
# verify_expiry should really always be True, but there exist access
# tokens in the wild which expire when they should not, so we can't
# enforce expiry yet (so we have to allow any caveat starting with
# 'time < ' in access tokens).
#
# On the other hand, short-term login tokens (as used by CAS login, for
# example) have an expiry time which we do want to enforce.
if verify_expiry: if verify_expiry:
v.satisfy_general(self._verify_expiry) v.satisfy_general(self._verify_expiry)
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):

View File

@ -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.

View File

@ -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()

View File

@ -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(

View File

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

View File

@ -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):

View File

@ -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')