-
Notifications
You must be signed in to change notification settings - Fork 308
Conversation
4a1097b
to
08b3490
Compare
Rebased on master, previous head was @4a1097b8a007bb2e7256aa3d57a9539e182454a9 |
Alright! This one's up—and it's the big one! @rohitpaulk @aandis et al. What do we think of our encryption procedure here? |
cc: @gratipay/security @TheHmadQureshi |
08b3490
to
01b4ec3
Compare
|
||
def test_ep_packs_encryptingly(self): | ||
packed = Participant.encrypting_packer.pack({"foo": "bar"}) | ||
assert urlsafe_b64decode(packed)[0] == b'\x80' # Frenet version |
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.
Frenet → Fernet
Do we trust Fernet? Do we trust |
Bottom of https://cryptography.io/en/latest/:
|
InvestigationsFernet
|
I noticed a |
Basically we need to decide if we're going to trust a) Fernet, and b) |
|
Fernet looks like a rinky-dink operation. It has 44 stars and 7 forks and approximately one contributor. |
The author doesn't mention fernet on his blog and barely on twitter. In fact:
https://twitter.com/dstufft/status/437363105228939264 Hmmm ... |
Other end of that Twitter convo suggests that Fernet comes from Heroku (where kr works):
https://twitter.com/craigkerstiens/status/437352536303874048 P.S. Should I/we be @mention'ing folks here? Would that be spam or transparency? |
I mean, PyCA advertises their interest in third-party audits. |
Searching |
"I don't have any issues with the code [on this PR], it seems simple enough. The only problem is this Fernet—how secure and reliable it is." —@kaguillera |
I've started a doc on credential management: gratipay/inside.gratipay.com#606. |
I don't see how we would do this apart from a key management application such as StrongAuth or Porticor, and I think that's something to reticket and tackle further down the line. |
Hmmm ... I was wondering about that: should we be using asymmetric encryption? Web app encrypts, other processes decrypt? |
I did some spelunking in the Patreon data dump. They do appear to have used RSA. While their implementation does have API for both encrypting and decrypting, I don't see that they do actually decrypt anywhere in the web application. This makes it possible for them to distribute a public key with the application, and keep the private key completely separate. This works for their model, where all creators have a direct relationship with Patreon. On Gratipay we're facilitating a relationship between a ~user and a Team, which is an organization other than Gratipay. We need to be able to communicate full identity information from the ~user to the Team (e.g., W-9 in the U.S.), and vice versa. In order to do that, we need to be able to decrypt within the application, so even if we used asymmetric encryption, we wouldn't be able to keep the private key entirely away from the web app. |
This is the one I actually reviewed.
Okay! The investigations and punchlist items are all checked off. I'm pretty sure that means that this is ready for final review and merge. 😳 Who is going to be brave enough? @rohitpaulk? @aandis? |
There's a bottle of fernet here at Stack, waiting to celebrate ... |
|
||
""" | ||
|
||
def __init__(self, key, *old_keys): |
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.
Can't this be simplified to just take *keys
?
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.
Hmm, maybe this was done to emphasize the answer to https://github.com/gratipay/gratipay.com/pull/3998/files#r62548400?
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.
Yeah, that's more or less what I was thinking. I wanted to differentiate between the first key, which is the one used for encryption (and decryption), and the old keys, which can only be used for decryption.
This looks fine to me overall - I've left a few comments regarding how multiple keys will be handled, would be great if we could have more tests for that |
Awesome, thanks! I've updated the punchlist in the description, and will get another commit or two together here ... |
@rohitpaulk Punchlist updated with responses, I'll let you check them off if you approve. :) |
Ooooooh wow. Oh wowohwowohwow. 😳 Here we go! |
Update dependency docs in light of #3998
@whit537 thanks for this nice search report about symmetric encryption. |
⬑ #3994
⬑ #3976
← #4004 — #3999 →
Punchlist
cryptography
—implement symmetric encryption #3998 (comment)