Code review comment for lp:~chipaca/snappy/auth

Revision history for this message
Tyler Hicks (tyhicks) wrote :

I have a few inline comments and then some higher level questions below.

1) Why implement scrypt, bcrypt, and sha512crypt all right out of the gate? The design should definitely incorporate algorithm agility so that we can move from scrypt to something else down the road if scrypt is found to be flawed. However, I found it odd that we're bringing the complexity of scrypt plus two "lesser" algorithms in from the start.
 - I've already reviewed the bcrypt implementation and it looks good
 - I still need to review the scrypt implementation
 - I'm ignoring sha512crypt until we've determined that it is needed.

2) This change creates a single username/password combo, which is different than the username/password combo for PAM logins, to be used for all sensitive requests made to snapd. How will that work in the future in a multi-user scenario? All users of the system will need to share the same snapd login information? Would it make sense for snapd to talk to PAM and authenticate these requests in that way?

Note that I haven't yet had a chance to review the tests.

« Back to merge proposal