Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Remove non-functional 'expire_access_token' setting (#5782)
Browse files Browse the repository at this point in the history
The `expire_access_token` didn't do what it sounded like it should do. What it
actually did was make Synapse enforce the 'time' caveat on macaroons used as
access tokens, but since our access token macaroons never contained such a
caveat, it was always a no-op.

(The code to add 'time' caveats was removed back in v0.18.5, in #1656)
  • Loading branch information
richvdh committed Jul 30, 2019
1 parent 865077f commit 8c97f64
Show file tree
Hide file tree
Showing 8 changed files with 9 additions and 37 deletions.
1 change: 1 addition & 0 deletions changelog.d/5782.removal
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove non-functional 'expire_access_token' setting.
4 changes: 0 additions & 4 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -942,10 +942,6 @@ uploads_path: "DATADIR/uploads"
#
# macaroon_secret_key: <PRIVATE STRING>

# Used to enable access token expiration.
#
#expire_access_token: False

# a secret which is used to calculate HMACs for form values, to stop
# falsification of values. Must be specified for the User Consent
# forms to work.
Expand Down
28 changes: 5 additions & 23 deletions synapse/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -410,21 +410,16 @@ def _parse_and_validate_macaroon(self, token, rights="access"):
try:
user_id = self.get_user_id_from_macaroon(macaroon)

has_expiry = False
guest = False
for caveat in macaroon.caveats:
if caveat.caveat_id.startswith("time "):
has_expiry = True
elif caveat.caveat_id == "guest = true":
if caveat.caveat_id == "guest = true":
guest = True

self.validate_macaroon(
macaroon, rights, self.hs.config.expire_access_token, user_id=user_id
)
self.validate_macaroon(macaroon, rights, user_id=user_id)
except (pymacaroons.exceptions.MacaroonException, TypeError, ValueError):
raise InvalidClientTokenError("Invalid macaroon passed.")

if not has_expiry and rights == "access":
if rights == "access":
self.token_cache[token] = (user_id, guest)

return user_id, guest
Expand All @@ -450,15 +445,14 @@ def get_user_id_from_macaroon(self, macaroon):
return caveat.caveat_id[len(user_prefix) :]
raise InvalidClientTokenError("No user caveat in macaroon")

def validate_macaroon(self, macaroon, type_string, verify_expiry, user_id):
def validate_macaroon(self, macaroon, type_string, user_id):
"""
validate that a Macaroon is understood by and was signed by this server.
Args:
macaroon(pymacaroons.Macaroon): The macaroon to validate
type_string(str): The kind of token required (e.g. "access",
"delete_pusher")
verify_expiry(bool): Whether to verify whether the macaroon has expired.
user_id (str): The user_id required
"""
v = pymacaroons.Verifier()
Expand All @@ -471,19 +465,7 @@ def validate_macaroon(self, macaroon, type_string, verify_expiry, user_id):
v.satisfy_exact("type = " + type_string)
v.satisfy_exact("user_id = %s" % user_id)
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:
v.satisfy_general(self._verify_expiry)
else:
v.satisfy_general(lambda c: c.startswith("time < "))
v.satisfy_general(self._verify_expiry)

# access_tokens include a nonce for uniqueness: any value is acceptable
v.satisfy_general(lambda c: c.startswith("nonce = "))
Expand Down
6 changes: 0 additions & 6 deletions synapse/config/key.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,6 @@ def read_config(self, config, config_dir_path, **kwargs):
seed = bytes(self.signing_key[0])
self.macaroon_secret_key = hashlib.sha256(seed).digest()

self.expire_access_token = config.get("expire_access_token", False)

# a secret which is used to calculate HMACs for form values, to stop
# falsification of values
self.form_secret = config.get("form_secret", None)
Expand All @@ -144,10 +142,6 @@ def generate_config_section(
#
%(macaroon_secret_key)s
# Used to enable access token expiration.
#
#expire_access_token: False
# a secret which is used to calculate HMACs for form values, to stop
# falsification of values. Must be specified for the User Consent
# forms to work.
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,7 @@ def validate_short_term_login_token_and_get_user_id(self, login_token):
try:
macaroon = pymacaroons.Macaroon.deserialize(login_token)
user_id = auth_api.get_user_id_from_macaroon(macaroon)
auth_api.validate_macaroon(macaroon, "login", True, user_id)
auth_api.validate_macaroon(macaroon, "login", user_id)
except Exception:
raise AuthError(403, "Invalid token", errcode=Codes.FORBIDDEN)
self.ratelimit_login_per_account(user_id)
Expand Down
2 changes: 1 addition & 1 deletion tests/handlers/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def make_homeserver(self, reactor, clock):
hs_config["max_mau_value"] = 50
hs_config["limit_usage_by_mau"] = True

hs = self.setup_test_homeserver(config=hs_config, expire_access_token=True)
hs = self.setup_test_homeserver(config=hs_config)
return hs

def prepare(self, reactor, clock, hs):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def make_homeserver(self, reactor, clock):
"room_name": "Server Notices",
}

hs = self.setup_test_homeserver(config=hs_config, expire_access_token=True)
hs = self.setup_test_homeserver(config=hs_config)
return hs

def prepare(self, reactor, clock, hs):
Expand Down
1 change: 0 additions & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ def default_config(name, parse=False):
"enable_registration": True,
"enable_registration_captcha": False,
"macaroon_secret_key": "not even a little secret",
"expire_access_token": False,
"trusted_third_party_id_servers": [],
"room_invite_state_types": [],
"password_providers": [],
Expand Down

0 comments on commit 8c97f64

Please sign in to comment.