Merge lp:~chipaca/snappy/auth into lp:~snappy-dev/snappy/snappy-moved-to-github
| Status: | Rejected |
|---|---|
| Rejected by: | John Lenton on 2015-10-11 |
| Proposed branch: | lp:~chipaca/snappy/auth |
| Merge into: | lp:~snappy-dev/snappy/snappy-moved-to-github |
| Prerequisite: | lp:~chipaca/snappy/merged-list |
| Diff against target: |
962 lines (+784/-41) 11 files modified
daemon/api.go (+24/-17) daemon/basicauth_1.3.go (+54/-0) daemon/basicauth_1.4.go (+30/-0) daemon/crypt/bcrypt.go (+38/-0) daemon/crypt/crypt.go (+120/-0) daemon/crypt/crypt_test.go (+248/-0) daemon/crypt/scrypt.go (+78/-0) daemon/crypt/sha512crypt.go (+120/-0) daemon/daemon.go (+23/-1) daemon/daemon_test.go (+48/-23) daemon/response.go (+1/-0) |
| To merge this branch: | bzr merge lp:~chipaca/snappy/auth |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Tyler Hicks | 2015-10-07 | Needs Fixing on 2015-10-09 | |
| Sergio Schvezov | 2015-10-07 | Approve on 2015-10-09 | |
|
Review via email:
|
|||
This proposal supersedes a proposal from 2015-10-07.
Commit Message
Require authentication for non-guest methods.
Description of the Change
Only GET can be used by non-authenticated, and then only for commands flagged with GuestOK. This means that we need some way to bootsrap, which'll come in a followup branch. We also want to change the credentials via the same REST API, which is yet another followup.
I'll repeat it in bigger letters in case you missed it: THIS WILL LOCK YOU OUT OF THE REST API.
This'll probably be breaking integration tests; need to check with the guys.
| Sergio Schvezov (sergiusens) wrote : | # |
| John Lenton (chipaca) wrote : | # |
re gc: probably not possible without major work, as the password will be clear(ish) in the http request headers.
If that's a concern, we should switch to digest auth.
- 752. By John Lenton on 2015-10-09
-
annotated 1.3's basicAuth with return variable names
| 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.
| Tyler Hicks (tyhicks) wrote : | # |
The username handling doesn't look correct to me. AuthOK() calls crypt.Check(
| Tyler Hicks (tyhicks) wrote : | # |
Have you benchmarked scrypt on the BBB or Raspberry Pi 2? I'm curious if there are significant delays when checking the password. I think scrypt is generally the best choice from a security standpoint but it may take considerable time to authenticate these REST API calls.
| John Lenton (chipaca) wrote : | # |
Wrt colons in username, “a user-id containing a colon character is invalid”, says the rfc about basic auth. Also I don't really care, because
Wrt password vs user+pass, i concatenate user+password from basic auth to create the "password" that crypt needs. As there aren't users, this seemed ok (the alternative being to ask for a username and discard it).
The multiple choice of algorithms are precisely because I benchmarked them on the bbb. sha512crypt is the only one that gives sub-second performance; scrypt takes about 30 seconds.
| John Lenton (chipaca) wrote : | # |
http://
Note it says N but it's actually log2(N). That is, 14 means an N of 16k.
That run was stapping at over 5s; I did a run with N of 1<<14 to check, and it was 23 seconds. Not 30, sorry -- even on that i7 14 is rather slow for a REST API (and on my slightly older laptop i7 it's about 2× that).
| Tyler Hicks (tyhicks) wrote : | # |
> The multiple choice of algorithms are precisely because I benchmarked them on
> the bbb. sha512crypt is the only one that gives sub-second performance; scrypt
> takes about 30 seconds.
Then is scrypt even an option? This MP has 'var DefaultAlg = SCRYPT' and crypt.Write() unconditionally uses SCRYPT.
| John Lenton (chipaca) wrote : | # |
>
> Then is scrypt even an option? This MP has 'var DefaultAlg = SCRYPT' and
> crypt.Write() unconditionally uses SCRYPT.
in a later branch I intend to let an oem snap override the default (so on arm you'd tweak it), and it's an scrypt with N=1<<10 so it's ok on even not-so-zippy intels.
| Tyler Hicks (tyhicks) wrote : | # |
> Wrt colons in username, “a user-id containing a colon character is invalid”,
> says the rfc about basic auth. Also I don't really care, because
>
> Wrt password vs user+pass, i concatenate user+password from basic auth to
> create the "password" that crypt needs. As there aren't users, this seemed ok
> (the alternative being to ask for a username and discard it).
Ok, so that again locks us into a single username and password combo for authenticating to snapd. Maybe that's fine but it would be good for one of the Snappy architects to comment here regarding whether or not multi-user support will be important in the near to medium term for snapd and Snappy, in general.
| Tyler Hicks (tyhicks) wrote : | # |
Can you add a comment to the code where the socket is created to make it clear that if this API is ever exposed over the network, HTTPS *must* used?
| Tyler Hicks (tyhicks) wrote : | # |
I need to start my weekend but I still need to finish looking at the scrypt implementation and research the N, R, and P values chosen. In the meantime, can you reach out to the architect team and get them to comment on how important multi-user support is?
| Sergio Schvezov (sergiusens) wrote : | # |
Maybe something closer to this https:/
| John Lenton (chipaca) wrote : | # |
We're doing the other thing, switching to named unix sockets. Thank you and sorry to waste your time, Tyler.
| Tyler Hicks (tyhicks) wrote : | # |
It wasn't a waste of time, at all! I'm glad we got the right design in place for local auth and this will allow me to start thinking about what will be needed for remote auth down the line. Thanks again, John!
Unmerged revisions
- 752. By John Lenton on 2015-10-09
-
annotated 1.3's basicAuth with return variable names
- 751. By John Lenton on 2015-10-09
-
fix for 1.3 not having basicAuth methods on http.Request. Also caught a nasty bug wrt null-termination
- 750. By John Lenton on 2015-10-09
-
restrict REST API calls w/auth
- 749. By John Lenton on 2015-10-08
-
Merged merged-list into auth.
- 748. By John Lenton on 2015-10-08
-
Merged merged-list into auth.
- 747. By John Lenton on 2015-10-08
-
Merged merged-list into auth.
- 746. By John Lenton on 2015-10-07
-
Merged merged-list into auth.
- 745. By John Lenton on 2015-10-07
-
first pass at auth


This code looks really neat and tidy, kudos!
I added a few irrelevant comments here and there (tl;rl; they are all irrelevant).
The only thing that I might consider trying is forcing garbage collection and getting rid of the 'password' from memory as soon as we have a hash, unless I missed that and is already taken care of.