-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
This just replaces random bytes with macaroons. The macaroons are not inspected by the client or server. In particular, they claim to have an expiry time, but nothing verifies that they have not expired. Follow-up commits will actually enforce the expiration, and allow for token refresh. See https://bit.ly/matrix-auth for more information
macaroon.add_first_party_caveat("type = access") | ||
now = self.hs.get_clock().time() | ||
expiry = now + 60 * 60 | ||
macaroon.add_first_party_caveat("time < %s" % expiry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We generally always use milliseconds in our APIs. What are the benefits of using seconds?
- Generally I try and use
%d
for numbers instead of%s
to make it clear what we expect and to avoid fun surprises ifexpiry
happens to magically be something other than a number. expiry
should be in a tuple, i.e."time < %s" % (expiry,)
(note the trailing,
). Not putting it in a tuple usually works, but it can lead to surprised e.g. if printing a list the result changes depending on if you have 1 or more items in the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If we're concerned about the number of characters in a caveat, do we want to be adding the spacing?
- Are these constraints a standard format? How are they interpreted by clients and servers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. All of the examples I've seen use spaces as padding, but there's no good reason to.
I've documented the constraints in https://bit.ly/matrix-auth - we should push it into the spec at some point, when it's a little more stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we doc the draft spec of how it works (as opposed to the rationale doc) in matrix-org/matrix-doc - either as a branch or in drafts/ such that it converges on something that can be easily merged into the main spec?
On 18 Aug 2015, at 18:39, Daniel Wagner-Hall [email protected] wrote:
In synapse/handlers/register.py:
@@ -274,11 +274,18 @@ def check_user_id_is_valid(self, user_id):
)def generate_token(self, user_id):
# urlsafe variant uses _ and - so use . as the separator and replace
# all =s with .s so http clients don't quote =s when it is used as
# query params.
return (base64.urlsafe_b64encode(user_id).replace('=', '.') + '.' +
stringutils.random_string(18))
macaroon = pymacaroons.Macaroon(
location = self.hs.config.server_name,
identifier = "key",
key = self.hs.config.macaroon_secret_key)
macaroon.add_first_party_caveat("gen = 1")
macaroon.add_first_party_caveat("user_id = %s" % user_id)
macaroon.add_first_party_caveat("type = access")
now = self.hs.get_clock().time()
expiry = now + 60 \* 60
Done. All of the examples I've seen use spaces as padding, but there's no good reason to.macaroon.add_first_party_caveat("time < %s" % expiry)
I've documented the constraints in https://bit.ly/matrix-auth - we should push it into the spec at some point, when it's a little more stable.
—
Reply to this email directly or view it on GitHub.
I made the non-test seconds instead of ms, but not the test
LGTM |
Issue macaroons as opaque auth tokens
This just replaces random bytes with macaroons. The macaroons are not
inspected by the client or server.
In particular, they claim to have an expiry time, but nothing verifies
that they have not expired.
Follow-up commits will actually enforce the expiration, and allow for
token refresh.
See https://bit.ly/matrix-auth for more information