Merge lp:~chipaca/snappy/auth into lp:~snappy-dev/snappy/snappy-moved-to-github
- auth
- Merge into snappy-moved-to-github
Status: | Rejected |
---|---|
Rejected by: | John Lenton |
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 (community) | Needs Fixing | ||
Sergio Schvezov | Approve | ||
Review via email: mp+273684@code.launchpad.net |
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.
Sergio Schvezov (sergiusens) : | # |
- 752. By John Lenton
-
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
-
annotated 1.3's basicAuth with return variable names
- 751. By John Lenton
-
fix for 1.3 not having basicAuth methods on http.Request. Also caught a nasty bug wrt null-termination
- 750. By John Lenton
-
restrict REST API calls w/auth
- 749. By John Lenton
-
Merged merged-list into auth.
- 748. By John Lenton
-
Merged merged-list into auth.
- 747. By John Lenton
-
Merged merged-list into auth.
- 746. By John Lenton
-
Merged merged-list into auth.
- 745. By John Lenton
-
first pass at auth
Preview Diff
1 | === modified file 'daemon/api.go' |
2 | --- daemon/api.go 2015-10-08 12:50:11 +0000 |
3 | +++ daemon/api.go 2015-10-09 13:33:12 +0000 |
4 | @@ -57,35 +57,41 @@ |
5 | |
6 | var ( |
7 | rootCmd = &Command{ |
8 | - Path: "/", |
9 | - GET: SyncResponse([]string{"/1.0"}).Self, |
10 | + GuestOK: true, |
11 | + Path: "/", |
12 | + GET: SyncResponse([]string{"/1.0"}).Self, |
13 | } |
14 | |
15 | v1Cmd = &Command{ |
16 | - Path: "/1.0", |
17 | - GET: v1Get, |
18 | + GuestOK: true, |
19 | + Path: "/1.0", |
20 | + GET: v1Get, |
21 | } |
22 | |
23 | metaIconCmd = &Command{ |
24 | - Path: "/1.0/icons/{icon}", |
25 | - GET: metaIconGet, |
26 | + GuestOK: true, |
27 | + Path: "/1.0/icons/{icon}", |
28 | + GET: metaIconGet, |
29 | } |
30 | |
31 | appIconCmd = &Command{ |
32 | - Path: "/1.0/icons/{name}.{origin}/icon", |
33 | - GET: appIconGet, |
34 | + GuestOK: true, |
35 | + Path: "/1.0/icons/{name}.{origin}/icon", |
36 | + GET: appIconGet, |
37 | } |
38 | |
39 | packagesCmd = &Command{ |
40 | - Path: "/1.0/packages", |
41 | - GET: getPackagesInfo, |
42 | - POST: sideloadPackage, |
43 | + GuestOK: true, |
44 | + Path: "/1.0/packages", |
45 | + GET: getPackagesInfo, |
46 | + POST: sideloadPackage, |
47 | } |
48 | |
49 | packageCmd = &Command{ |
50 | - Path: "/1.0/packages/{name}.{origin}", |
51 | - GET: getPackageInfo, |
52 | - POST: postPackage, |
53 | + GuestOK: true, |
54 | + Path: "/1.0/packages/{name}.{origin}", |
55 | + GET: getPackageInfo, |
56 | + POST: postPackage, |
57 | } |
58 | |
59 | packageConfigCmd = &Command{ |
60 | @@ -95,9 +101,10 @@ |
61 | } |
62 | |
63 | packageSvcsCmd = &Command{ |
64 | - Path: "/1.0/packages/{name}.{origin}/services", |
65 | - GET: packageService, |
66 | - PUT: packageService, |
67 | + GuestOK: true, |
68 | + Path: "/1.0/packages/{name}.{origin}/services", |
69 | + GET: packageService, |
70 | + PUT: packageService, |
71 | } |
72 | |
73 | packageSvcCmd = &Command{ |
74 | |
75 | === added file 'daemon/basicauth_1.3.go' |
76 | --- daemon/basicauth_1.3.go 1970-01-01 00:00:00 +0000 |
77 | +++ daemon/basicauth_1.3.go 2015-10-09 13:33:12 +0000 |
78 | @@ -0,0 +1,54 @@ |
79 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
80 | +// +build !go1.4 |
81 | + |
82 | +/* |
83 | + * Copyright (C) 2014-2015 Canonical Ltd |
84 | + * |
85 | + * This program is free software: you can redistribute it and/or modify |
86 | + * it under the terms of the GNU General Public License version 3 as |
87 | + * published by the Free Software Foundation. |
88 | + * |
89 | + * This program is distributed in the hope that it will be useful, |
90 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
91 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
92 | + * GNU General Public License for more details. |
93 | + * |
94 | + * You should have received a copy of the GNU General Public License |
95 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
96 | + * |
97 | + */ |
98 | + |
99 | +package daemon |
100 | + |
101 | +import ( |
102 | + "bytes" |
103 | + "encoding/base64" |
104 | + "net/http" |
105 | + "strings" |
106 | +) |
107 | + |
108 | +const authScheme = "Basic " // extra space because cheating |
109 | + |
110 | +func basicAuth(req *http.Request) (username string, password string, ok bool) { |
111 | + auth := req.Header.Get("Authorization") |
112 | + if !strings.HasPrefix(auth, authScheme) { |
113 | + return "", "", false |
114 | + } |
115 | + |
116 | + up, err := base64.StdEncoding.DecodeString(auth[len(authScheme):]) |
117 | + if err != nil { |
118 | + return "", "", false |
119 | + } |
120 | + |
121 | + idx := bytes.IndexByte(up, ':') |
122 | + if idx < 0 { |
123 | + return "", "", false |
124 | + } |
125 | + |
126 | + return string(up[:idx]), string(up[idx+1:]), true |
127 | +} |
128 | + |
129 | +func setBasicAuth(req *http.Request, username, password string) { |
130 | + up := base64.StdEncoding.EncodeToString([]byte(username + ":" + password)) |
131 | + req.Header.Set("Authorization", authScheme+up) |
132 | +} |
133 | |
134 | === added file 'daemon/basicauth_1.4.go' |
135 | --- daemon/basicauth_1.4.go 1970-01-01 00:00:00 +0000 |
136 | +++ daemon/basicauth_1.4.go 2015-10-09 13:33:12 +0000 |
137 | @@ -0,0 +1,30 @@ |
138 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
139 | +// +build go1.4 |
140 | + |
141 | +/* |
142 | + * Copyright (C) 2014-2015 Canonical Ltd |
143 | + * |
144 | + * This program is free software: you can redistribute it and/or modify |
145 | + * it under the terms of the GNU General Public License version 3 as |
146 | + * published by the Free Software Foundation. |
147 | + * |
148 | + * This program is distributed in the hope that it will be useful, |
149 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
150 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
151 | + * GNU General Public License for more details. |
152 | + * |
153 | + * You should have received a copy of the GNU General Public License |
154 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
155 | + * |
156 | + */ |
157 | + |
158 | +package daemon |
159 | + |
160 | +import ( |
161 | + "net/http" |
162 | +) |
163 | + |
164 | +var ( |
165 | + basicAuth = (*http.Request).BasicAuth |
166 | + setBasicAuth = (*http.Request).SetBasicAuth |
167 | +) |
168 | |
169 | === added directory 'daemon/crypt' |
170 | === added file 'daemon/crypt/bcrypt.go' |
171 | --- daemon/crypt/bcrypt.go 1970-01-01 00:00:00 +0000 |
172 | +++ daemon/crypt/bcrypt.go 2015-10-09 13:33:12 +0000 |
173 | @@ -0,0 +1,38 @@ |
174 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
175 | + |
176 | +/* |
177 | + * Copyright (C) 2014-2015 Canonical Ltd |
178 | + * |
179 | + * This program is free software: you can redistribute it and/or modify |
180 | + * it under the terms of the GNU General Public License version 3 as |
181 | + * published by the Free Software Foundation. |
182 | + * |
183 | + * This program is distributed in the hope that it will be useful, |
184 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
185 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
186 | + * GNU General Public License for more details. |
187 | + * |
188 | + * You should have received a copy of the GNU General Public License |
189 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
190 | + * |
191 | + */ |
192 | + |
193 | +package crypt |
194 | + |
195 | +import ( |
196 | + "golang.org/x/crypto/bcrypt" |
197 | +) |
198 | + |
199 | +var bcryptAlg = cryptAlg{ |
200 | + gen: func(password []byte) ([]byte, error) { |
201 | + hash, err := bcrypt.GenerateFromPassword(password, -1) |
202 | + if err != nil { |
203 | + return nil, err |
204 | + } |
205 | + |
206 | + return hash, nil |
207 | + }, |
208 | + cmp: func(hashedPassword, password []byte) error { |
209 | + return bcrypt.CompareHashAndPassword(hashedPassword, password) |
210 | + }, |
211 | +} |
212 | |
213 | === added file 'daemon/crypt/crypt.go' |
214 | --- daemon/crypt/crypt.go 1970-01-01 00:00:00 +0000 |
215 | +++ daemon/crypt/crypt.go 2015-10-09 13:33:12 +0000 |
216 | @@ -0,0 +1,120 @@ |
217 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
218 | + |
219 | +/* |
220 | + * Copyright (C) 2014-2015 Canonical Ltd |
221 | + * |
222 | + * This program is free software: you can redistribute it and/or modify |
223 | + * it under the terms of the GNU General Public License version 3 as |
224 | + * published by the Free Software Foundation. |
225 | + * |
226 | + * This program is distributed in the hope that it will be useful, |
227 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
228 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
229 | + * GNU General Public License for more details. |
230 | + * |
231 | + * You should have received a copy of the GNU General Public License |
232 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
233 | + * |
234 | + */ |
235 | + |
236 | +package crypt |
237 | + |
238 | +import ( |
239 | + "bytes" |
240 | + "errors" |
241 | + "io" |
242 | + "os" |
243 | + "path/filepath" |
244 | + |
245 | + "launchpad.net/snappy/dirs" |
246 | + "launchpad.net/snappy/helpers" |
247 | +) |
248 | + |
249 | +type cryptAlg struct { |
250 | + gen func(password []byte) ([]byte, error) |
251 | + cmp func(hashed, password []byte) error |
252 | +} |
253 | + |
254 | +// the known crypt algorithms |
255 | +// |
256 | +// xxx is algorithm the right name even |
257 | +const ( |
258 | + SHA512 = "sha512" |
259 | + SCRYPT = "scrypt" |
260 | + BCRYPT = "bcrypt" |
261 | +) |
262 | + |
263 | +// the default algorithm |
264 | +var DefaultAlg = SCRYPT |
265 | + |
266 | +var cryptAlgs = map[string]cryptAlg{ |
267 | + SCRYPT: scryptAlg, |
268 | + BCRYPT: bcryptAlg, |
269 | + SHA512: sha512cryptAlg, |
270 | +} |
271 | + |
272 | +var ( |
273 | + // ErrBadCreds is a generic "bad password" error |
274 | + ErrBadCreds = errors.New("bad credentials") |
275 | +) |
276 | + |
277 | +const ( |
278 | + maxSecretLen = 1024 // upper bound of the length of a secret |
279 | + maxPrefixLen = 8 // upper bound of the prefix length (e.g. "scrypt:") |
280 | + |
281 | + minpwlen = 8 |
282 | +) |
283 | + |
284 | +func pwFile() string { |
285 | + return filepath.Join(dirs.GlobalRootDir, dirs.SnappyDir, ".snapd_creds") |
286 | +} |
287 | + |
288 | +// Write the given password out to the file, crypting it first |
289 | +func Write(password []byte) error { |
290 | + // TODO: cracklib |
291 | + if len(password) < minpwlen { |
292 | + return ErrBadCreds |
293 | + } |
294 | + |
295 | + hash, err := cryptAlgs[DefaultAlg].gen(password) |
296 | + if err != nil { |
297 | + return err |
298 | + } |
299 | + |
300 | + buf := append([]byte(DefaultAlg+":"), hash...) |
301 | + if err := helpers.AtomicWriteFile(pwFile(), buf, 0600); err != nil { |
302 | + return err |
303 | + } |
304 | + |
305 | + return nil |
306 | +} |
307 | + |
308 | +// Check whether this password and the one on file match. |
309 | +func Check(password []byte) bool { |
310 | + f, err := os.Open(pwFile()) |
311 | + if err != nil { |
312 | + return false |
313 | + } |
314 | + |
315 | + buf := make([]byte, maxPrefixLen+maxSecretLen) |
316 | + n, err := f.ReadAt(buf, 0) |
317 | + if err != io.EOF { |
318 | + return false |
319 | + } |
320 | + |
321 | + if n < maxPrefixLen { |
322 | + return false |
323 | + } |
324 | + |
325 | + idx := bytes.IndexByte(buf[:maxPrefixLen], ':') |
326 | + if idx < 1 { |
327 | + return false |
328 | + } |
329 | + |
330 | + alg, ok := cryptAlgs[string(buf[:idx])] |
331 | + if !ok { |
332 | + return false |
333 | + } |
334 | + |
335 | + return alg.cmp(buf[idx+1:n], password) == nil |
336 | +} |
337 | |
338 | === added file 'daemon/crypt/crypt_test.go' |
339 | --- daemon/crypt/crypt_test.go 1970-01-01 00:00:00 +0000 |
340 | +++ daemon/crypt/crypt_test.go 2015-10-09 13:33:12 +0000 |
341 | @@ -0,0 +1,248 @@ |
342 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
343 | + |
344 | +/* |
345 | + * Copyright (C) 2014-2015 Canonical Ltd |
346 | + * |
347 | + * This program is free software: you can redistribute it and/or modify |
348 | + * it under the terms of the GNU General Public License version 3 as |
349 | + * published by the Free Software Foundation. |
350 | + * |
351 | + * This program is distributed in the hope that it will be useful, |
352 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
353 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
354 | + * GNU General Public License for more details. |
355 | + * |
356 | + * You should have received a copy of the GNU General Public License |
357 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
358 | + * |
359 | + */ |
360 | + |
361 | +package crypt |
362 | + |
363 | +import ( |
364 | + "bytes" |
365 | + "crypto/rand" |
366 | + "errors" |
367 | + "io" |
368 | + "os" |
369 | + "path/filepath" |
370 | + "testing" |
371 | + |
372 | + "gopkg.in/check.v1" |
373 | + |
374 | + "launchpad.net/snappy/dirs" |
375 | +) |
376 | + |
377 | +// Hook up check.v1 into the "go test" runner |
378 | +func Test(t *testing.T) { check.TestingT(t) } |
379 | + |
380 | +type cryptSuite struct { |
381 | + d string |
382 | + oldReader io.Reader |
383 | + oldN int |
384 | + oldR int |
385 | + oldP int |
386 | +} |
387 | + |
388 | +var _ = check.Suite(&cryptSuite{}) |
389 | + |
390 | +var errBadStuff = errors.New("fail") |
391 | + |
392 | +type totallyRandom struct { |
393 | + err error |
394 | +} |
395 | + |
396 | +func (t *totallyRandom) Read(p []byte) (int, error) { |
397 | + if t.err != nil { |
398 | + return 0, t.err |
399 | + } |
400 | + if len(p) < 1 { |
401 | + return 0, nil |
402 | + } |
403 | + p[0] = 9 // http://dilbert.com/strip/2001-10-25 -- wait, 2001? Ow. |
404 | + return 1, nil |
405 | +} |
406 | + |
407 | +func (s *cryptSuite) SetUpTest(c *check.C) { |
408 | + s.d = c.MkDir() |
409 | + dirs.SetRootDir(s.d) |
410 | + c.Check(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, dirs.SnappyDir), 0755), check.IsNil) |
411 | + |
412 | + s.oldN = scryptN |
413 | + s.oldR = scryptR |
414 | + s.oldP = scryptP |
415 | + s.oldReader = rand.Reader |
416 | + rand.Reader = &totallyRandom{} |
417 | + scryptN = 2 |
418 | + scryptR = 1 |
419 | + scryptP = 1 |
420 | +} |
421 | + |
422 | +func (s *cryptSuite) TearDownTest(c *check.C) { |
423 | + rand.Reader = s.oldReader |
424 | + scryptN = s.oldN |
425 | + scryptR = s.oldR |
426 | + scryptP = s.oldP |
427 | +} |
428 | + |
429 | +func (s *cryptSuite) TestCryptAlgs(c *check.C) { |
430 | + |
431 | + password := []byte("password") |
432 | + // know any interesting password to try? call 0800-CHIPACA |
433 | + for k, v := range cryptAlgs { |
434 | + hash, err := v.gen(password) |
435 | + c.Assert(err, check.IsNil, check.Commentf(k)) |
436 | + c.Assert(hash, check.NotNil, check.Commentf(k)) |
437 | + |
438 | + c.Check(v.cmp(hash, password), check.IsNil, check.Commentf(k)) |
439 | + c.Check(v.cmp(hash, []byte("what")), check.NotNil, check.Commentf(k)) |
440 | + } |
441 | +} |
442 | + |
443 | +func (s *cryptSuite) TestCryptAlgsFailure(c *check.C) { |
444 | + defer func() { |
445 | + rand.Reader.(*totallyRandom).err = nil |
446 | + }() |
447 | + |
448 | + password := []byte("password") |
449 | + for k, v := range cryptAlgs { |
450 | + rand.Reader.(*totallyRandom).err = nil |
451 | + |
452 | + goodHash, err := v.gen(password) |
453 | + c.Check(err, check.IsNil, check.Commentf(k)) |
454 | + c.Check(goodHash, check.NotNil, check.Commentf(k)) |
455 | + c.Check(v.cmp(goodHash, password), check.IsNil, check.Commentf(k)) |
456 | + |
457 | + rand.Reader.(*totallyRandom).err = errBadStuff |
458 | + |
459 | + hash, err := v.gen(password) |
460 | + c.Check(err, check.NotNil, check.Commentf(k)) |
461 | + c.Check(hash, check.IsNil, check.Commentf(k)) |
462 | + } |
463 | +} |
464 | + |
465 | +func (s *cryptSuite) TestScryptFailures(c *check.C) { |
466 | + pass := []byte("password") |
467 | + c.Check(scryptAlg.cmp([]byte("w"), pass), check.ErrorMatches, "illegal ascii85 data .*") |
468 | + |
469 | + hash, err := scryptAlg.gen(pass) |
470 | + c.Assert(err, check.IsNil) |
471 | + c.Check(scryptAlg.cmp(hash, pass), check.IsNil) |
472 | + |
473 | + scryptN = 3 |
474 | + defer func() { |
475 | + scryptN = 2 |
476 | + }() |
477 | + |
478 | + c.Check(scryptAlg.cmp(hash, pass), check.ErrorMatches, "scrypt: N .*") |
479 | + _, err = scryptAlg.gen(pass) |
480 | + c.Check(err, check.ErrorMatches, "scrypt: N .*") |
481 | +} |
482 | + |
483 | +func (s *cryptSuite) TestSHA512CryptFailrues(c *check.C) { |
484 | + c.Check(sha512cryptAlg.cmp([]byte("$$$$$$$$$$"), []byte("x")), check.ErrorMatches, "invalid argument") |
485 | +} |
486 | + |
487 | +func (s *cryptSuite) TestRoundtrip(c *check.C) { |
488 | + c.Check(Write([]byte("password")), check.IsNil) |
489 | + c.Check(Check([]byte("password")), check.Equals, true) |
490 | + c.Check(Check([]byte("fishfish")), check.Equals, false) |
491 | +} |
492 | + |
493 | +func (s *cryptSuite) TestRoundtripChangedDefaults(c *check.C) { |
494 | + DefaultAlg = "bcrypt" |
495 | + c.Check(Write([]byte("password")), check.IsNil) |
496 | + DefaultAlg = "scrypt" |
497 | + c.Check(Check([]byte("password")), check.Equals, true) |
498 | + c.Check(Check([]byte("fishfish")), check.Equals, false) |
499 | +} |
500 | + |
501 | +var failAlg = cryptAlg{ |
502 | + gen: func(password []byte) ([]byte, error) { |
503 | + return nil, errBadStuff |
504 | + }, |
505 | + cmp: func([]byte, []byte) error { |
506 | + return errBadStuff |
507 | + }, |
508 | +} |
509 | + |
510 | +func (s *cryptSuite) TestCryptFailures(c *check.C) { |
511 | + alg := cryptAlgs[DefaultAlg] |
512 | + defer func() { cryptAlgs[DefaultAlg] = alg }() |
513 | + pass := []byte("password") |
514 | + dirs.SetRootDir("/does/not/exist") |
515 | + |
516 | + // first, try to read it without having written it first: |
517 | + c.Check(Check(pass), check.Equals, false) |
518 | + |
519 | + // next, write it |
520 | + c.Check(Write(pass), check.ErrorMatches, ".*no such file or directory.*") |
521 | + // oops, my bad |
522 | + dirs.SetRootDir(s.d) |
523 | + // *now* write it |
524 | + c.Check(Write(pass[:3]), check.ErrorMatches, ".*bad credentials.*") |
525 | + // jk, jk |
526 | + c.Check(Write(pass), check.IsNil) |
527 | + // pass, oh joy, etc |
528 | + c.Check(Check(pass), check.Equals, true) |
529 | + // write it again |
530 | + c.Check(Write(pass), check.IsNil) |
531 | + // yep, still passes |
532 | + c.Check(Check(pass), check.Equals, true) |
533 | + |
534 | + // now the algorithm starts to fail. darned algorithms, they |
535 | + // don't make 'em like they used. |
536 | + cryptAlgs[DefaultAlg] = failAlg |
537 | + |
538 | + c.Check(Write(pass), check.NotNil) |
539 | + c.Check(Check(pass), check.Equals, false) |
540 | + |
541 | + // back to normal |
542 | + cryptAlgs[DefaultAlg] = alg |
543 | + c.Check(Check(pass), check.Equals, true) |
544 | + |
545 | + f, err := os.OpenFile(pwFile(), os.O_WRONLY, 0) |
546 | + c.Check(err, check.IsNil) |
547 | + |
548 | + // but what's this? filesystem corruption! |
549 | + _, err = f.WriteAt([]byte{0}, 0) |
550 | + c.Check(err, check.IsNil) |
551 | + c.Check(Check(pass), check.Equals, false) |
552 | + |
553 | + // mental |
554 | + _, err = f.WriteAt(bytes.Repeat([]byte{'x'}, maxPrefixLen*2), 0) |
555 | + c.Check(err, check.IsNil) |
556 | + c.Check(Check(pass), check.Equals, false) |
557 | + |
558 | + // all better |
559 | + c.Check(Write(pass), check.IsNil) |
560 | + c.Check(Check(pass), check.Equals, true) |
561 | + |
562 | + // stahp! |
563 | + os.Truncate(pwFile(), maxPrefixLen-1) |
564 | + c.Check(Check(pass), check.Equals, false) |
565 | + |
566 | + // no, really! |
567 | + os.Truncate(pwFile(), 0) |
568 | + c.Check(Check(pass), check.Equals, false) |
569 | + |
570 | + // ok, enough |
571 | + c.Check(os.Remove(pwFile()), check.IsNil) |
572 | + c.Check(os.Mkdir(pwFile(), 0755), check.IsNil) |
573 | + c.Check(Check(pass), check.Equals, false) |
574 | + c.Check(os.Remove(pwFile()), check.IsNil) |
575 | + c.Check(os.Symlink("does-not-exist", pwFile()), check.IsNil) |
576 | + c.Check(Check(pass), check.Equals, false) |
577 | +} |
578 | + |
579 | +func (s *cryptSuite) TestCryptNil(c *check.C) { |
580 | + alg := DefaultAlg |
581 | + defer func() { DefaultAlg = alg }() |
582 | + |
583 | + for k := range cryptAlgs { |
584 | + DefaultAlg = k |
585 | + |
586 | + c.Check(Write(nil), check.NotNil, check.Commentf(k)) |
587 | + c.Check(Check(nil), check.NotNil, check.Commentf(k)) |
588 | + } |
589 | +} |
590 | |
591 | === added file 'daemon/crypt/scrypt.go' |
592 | --- daemon/crypt/scrypt.go 1970-01-01 00:00:00 +0000 |
593 | +++ daemon/crypt/scrypt.go 2015-10-09 13:33:12 +0000 |
594 | @@ -0,0 +1,78 @@ |
595 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
596 | + |
597 | +/* |
598 | + * Copyright (C) 2014-2015 Canonical Ltd |
599 | + * |
600 | + * This program is free software: you can redistribute it and/or modify |
601 | + * it under the terms of the GNU General Public License version 3 as |
602 | + * published by the Free Software Foundation. |
603 | + * |
604 | + * This program is distributed in the hope that it will be useful, |
605 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
606 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
607 | + * GNU General Public License for more details. |
608 | + * |
609 | + * You should have received a copy of the GNU General Public License |
610 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
611 | + * |
612 | + */ |
613 | + |
614 | +package crypt |
615 | + |
616 | +import ( |
617 | + "bytes" |
618 | + "crypto/rand" |
619 | + "encoding/ascii85" |
620 | + "io" |
621 | + |
622 | + "golang.org/x/crypto/scrypt" |
623 | +) |
624 | + |
625 | +const ( |
626 | + scryptSaltLen = 32 |
627 | + scryptHashLen = 64 |
628 | +) |
629 | + |
630 | +var ( |
631 | + scryptN = 1 << 10 |
632 | + scryptR = 8 |
633 | + scryptP = 16 |
634 | +) |
635 | + |
636 | +var scryptAlg = cryptAlg{ |
637 | + cmp: func(hashedPassword, password []byte) error { |
638 | + saltedHash := make([]byte, scryptSaltLen+scryptHashLen) |
639 | + _, _, err := ascii85.Decode(saltedHash, hashedPassword, true) |
640 | + if err != nil { |
641 | + return err |
642 | + } |
643 | + |
644 | + salt := saltedHash[:scryptSaltLen] |
645 | + hash, err := scrypt.Key([]byte(password), salt, scryptN, scryptR, scryptP, scryptHashLen) |
646 | + if err != nil { |
647 | + return err |
648 | + } |
649 | + |
650 | + if !bytes.Equal(hash, saltedHash[scryptSaltLen:]) { |
651 | + return ErrBadCreds |
652 | + } |
653 | + |
654 | + return nil |
655 | + }, |
656 | + gen: func(password []byte) ([]byte, error) { |
657 | + salt := make([]byte, scryptSaltLen) |
658 | + if _, err := io.ReadFull(rand.Reader, salt); err != nil { |
659 | + return nil, err |
660 | + } |
661 | + |
662 | + hash, err := scrypt.Key([]byte(password), salt, scryptN, scryptR, scryptP, scryptHashLen) |
663 | + if err != nil { |
664 | + return nil, err |
665 | + } |
666 | + |
667 | + coded := make([]byte, ascii85.MaxEncodedLen(scryptSaltLen+scryptHashLen)) |
668 | + n := ascii85.Encode(coded, append(salt, hash...)) |
669 | + |
670 | + return coded[:n], nil |
671 | + }, |
672 | +} |
673 | |
674 | === added file 'daemon/crypt/sha512crypt.go' |
675 | --- daemon/crypt/sha512crypt.go 1970-01-01 00:00:00 +0000 |
676 | +++ daemon/crypt/sha512crypt.go 2015-10-09 13:33:12 +0000 |
677 | @@ -0,0 +1,120 @@ |
678 | +// -*- Mode: Go; indent-tabs-mode: t -*- |
679 | + |
680 | +/* |
681 | + * Copyright (C) 2014-2015 Canonical Ltd |
682 | + * |
683 | + * This program is free software: you can redistribute it and/or modify |
684 | + * it under the terms of the GNU General Public License version 3 as |
685 | + * published by the Free Software Foundation. |
686 | + * |
687 | + * This program is distributed in the hope that it will be useful, |
688 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
689 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
690 | + * GNU General Public License for more details. |
691 | + * |
692 | + * You should have received a copy of the GNU General Public License |
693 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
694 | + * |
695 | + */ |
696 | + |
697 | +package crypt |
698 | + |
699 | +/* |
700 | +#cgo CFLAGS: -D_GNU_SOURCE -O3 -Wextra -Werror -pedantic |
701 | +#cgo LDFLAGS: -lcrypt |
702 | +#include <crypt.h> |
703 | +#include <string.h> |
704 | +*/ |
705 | +import "C" |
706 | + |
707 | +import ( |
708 | + "bytes" |
709 | + "crypto/rand" |
710 | + "encoding/binary" |
711 | + "unsafe" |
712 | +) |
713 | + |
714 | +const ( |
715 | + sha512saltLetters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789./" |
716 | + sha512saltNChars = 16 |
717 | +) |
718 | + |
719 | +func mksalt() ([]byte, error) { |
720 | + raw := make([]uint32, 3) |
721 | + if err := binary.Read(rand.Reader, binary.LittleEndian, &raw); err != nil { |
722 | + return nil, err |
723 | + } |
724 | + |
725 | + full := make([]byte, sha512saltNChars+4) |
726 | + full[0] = byte('$') |
727 | + full[1] = byte('6') |
728 | + full[2] = byte('$') |
729 | + full[3+sha512saltNChars] = byte('$') |
730 | + salt := full[3 : 3+sha512saltNChars] |
731 | + |
732 | + m := uint32(0) |
733 | + for i, n := range raw { |
734 | + for j := 1; j < 6; j++ { |
735 | + salt[i*5+j] = sha512saltLetters[n&63] |
736 | + n >>= 6 |
737 | + } |
738 | + m += n << (2 * uint(i)) |
739 | + } |
740 | + salt[0] = sha512saltLetters[m] |
741 | + |
742 | + return full, nil |
743 | +} |
744 | + |
745 | +func crypt(password, saltOrHashedPassword []byte, checkOnly bool) ([]byte, error) { |
746 | + s := C.struct_crypt_data{} |
747 | + s.initialized = 0 |
748 | + |
749 | + if password[len(password)-1] != 0 { |
750 | + password = append(password, 0) |
751 | + } |
752 | + if saltOrHashedPassword[len(saltOrHashedPassword)-1] != 0 { |
753 | + saltOrHashedPassword = append(saltOrHashedPassword, 0) |
754 | + } |
755 | + |
756 | + p, err := C.crypt_r( |
757 | + (*C.char)(unsafe.Pointer(&password[0])), |
758 | + (*C.char)(unsafe.Pointer(&saltOrHashedPassword[0])), |
759 | + &s, |
760 | + ) |
761 | + if err != nil { |
762 | + return nil, err |
763 | + } |
764 | + |
765 | + n := C.strnlen(p, maxSecretLen) |
766 | + if n == maxSecretLen { |
767 | + // just in case |
768 | + return nil, ErrBadCreds |
769 | + } |
770 | + |
771 | + buf := (*[maxSecretLen]byte)(unsafe.Pointer(p))[:n+1] // null-terminated |
772 | + |
773 | + if checkOnly { |
774 | + if bytes.Equal(buf, saltOrHashedPassword) { |
775 | + return nil, nil |
776 | + } |
777 | + return nil, ErrBadCreds |
778 | + } |
779 | + |
780 | + return append([]byte(nil), buf...), nil |
781 | +} |
782 | + |
783 | +var sha512cryptAlg = cryptAlg{ |
784 | + gen: func(password []byte) ([]byte, error) { |
785 | + salt, err := mksalt() |
786 | + if err != nil { |
787 | + return nil, err |
788 | + } |
789 | + |
790 | + return crypt(password, salt, false) |
791 | + }, |
792 | + cmp: func(hashedPassword, password []byte) error { |
793 | + _, err := crypt(password, hashedPassword, true) |
794 | + |
795 | + return err |
796 | + }, |
797 | +} |
798 | |
799 | === modified file 'daemon/daemon.go' |
800 | --- daemon/daemon.go 2015-10-05 23:10:59 +0000 |
801 | +++ daemon/daemon.go 2015-10-09 13:33:12 +0000 |
802 | @@ -30,6 +30,7 @@ |
803 | "github.com/gorilla/mux" |
804 | "gopkg.in/tomb.v2" |
805 | |
806 | + "launchpad.net/snappy/daemon/crypt" |
807 | "launchpad.net/snappy/logger" |
808 | ) |
809 | |
810 | @@ -47,7 +48,9 @@ |
811 | |
812 | // A Command routes a request to an individual per-verb ResponseFUnc |
813 | type Command struct { |
814 | - Path string |
815 | + // GuestOK: whether GET requests can proceed without auth |
816 | + GuestOK bool |
817 | + Path string |
818 | // |
819 | GET ResponseFunc |
820 | PUT ResponseFunc |
821 | @@ -57,7 +60,26 @@ |
822 | d *Daemon |
823 | } |
824 | |
825 | +// AuthOK checks whether the request has enough authorization for the command |
826 | +func (c *Command) AuthOK(r *http.Request) bool { |
827 | + if c.GuestOK && r.Method == "GET" { |
828 | + return true |
829 | + } |
830 | + |
831 | + user, pass, ok := basicAuth(r) |
832 | + if !ok { |
833 | + return false |
834 | + } |
835 | + |
836 | + return crypt.Check([]byte(user + pass)) |
837 | +} |
838 | + |
839 | func (c *Command) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
840 | + if !c.AuthOK(r) { |
841 | + Unauthorized.ServeHTTP(w, r) |
842 | + return |
843 | + } |
844 | + |
845 | var rspf ResponseFunc |
846 | var rsp Response = BadMethod |
847 | |
848 | |
849 | === modified file 'daemon/daemon_test.go' |
850 | --- daemon/daemon_test.go 2015-10-05 23:10:59 +0000 |
851 | +++ daemon/daemon_test.go 2015-10-09 13:33:12 +0000 |
852 | @@ -23,52 +23,77 @@ |
853 | "fmt" |
854 | "net/http" |
855 | "net/http/httptest" |
856 | + "os" |
857 | + "path/filepath" |
858 | |
859 | "github.com/gorilla/mux" |
860 | "gopkg.in/check.v1" |
861 | + |
862 | + "launchpad.net/snappy/daemon/crypt" |
863 | + "launchpad.net/snappy/dirs" |
864 | ) |
865 | |
866 | type daemonSuite struct{} |
867 | |
868 | var _ = check.Suite(&daemonSuite{}) |
869 | |
870 | -type mockHandler struct { |
871 | - cmd *Command |
872 | - lastMethod string |
873 | -} |
874 | - |
875 | -func (mck *mockHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
876 | - mck.lastMethod = r.Method |
877 | -} |
878 | -func (mck *mockHandler) Self(*Command, *http.Request) Response { |
879 | - return mck |
880 | -} |
881 | - |
882 | -func mkRF(c *check.C, cmd *Command, mck *mockHandler) ResponseFunc { |
883 | +type nullHandler struct{} |
884 | + |
885 | +func (nu *nullHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
886 | +} |
887 | +func (nu *nullHandler) Self(*Command, *http.Request) Response { |
888 | + return nu |
889 | +} |
890 | + |
891 | +func mkRF(c *check.C, cmd *Command, method string) ResponseFunc { |
892 | return func(innerCmd *Command, req *http.Request) Response { |
893 | c.Assert(cmd, check.Equals, innerCmd) |
894 | - return mck |
895 | + c.Assert(method, check.Equals, req.Method) |
896 | + return &nullHandler{} |
897 | } |
898 | } |
899 | |
900 | func (s *daemonSuite) TestCommandMethodDispatch(c *check.C) { |
901 | + dirs.SetRootDir(c.MkDir()) |
902 | + c.Check(os.MkdirAll(filepath.Join(dirs.GlobalRootDir, dirs.SnappyDir), 0755), check.IsNil) |
903 | + |
904 | cmd := &Command{} |
905 | - mck := &mockHandler{cmd: cmd} |
906 | - rf := mkRF(c, cmd, mck) |
907 | - cmd.GET = rf |
908 | - cmd.PUT = rf |
909 | - cmd.POST = rf |
910 | - cmd.DELETE = rf |
911 | + cmd.GET = mkRF(c, cmd, "GET") |
912 | + cmd.PUT = mkRF(c, cmd, "PUT") |
913 | + cmd.POST = mkRF(c, cmd, "POST") |
914 | + cmd.DELETE = mkRF(c, cmd, "DELETE") |
915 | + |
916 | + crypt.DefaultAlg = crypt.SHA512 |
917 | + c.Check(crypt.Write([]byte("password")), check.IsNil) |
918 | |
919 | for _, method := range []string{"GET", "POST", "PUT", "DELETE"} { |
920 | req, err := http.NewRequest(method, "", nil) |
921 | - c.Assert(err, check.IsNil) |
922 | - cmd.ServeHTTP(nil, req) |
923 | - c.Check(mck.lastMethod, check.Equals, method) |
924 | + c.Assert(err, check.IsNil, check.Commentf(method)) |
925 | + |
926 | + cmd.GuestOK = false |
927 | + rec := httptest.NewRecorder() |
928 | + cmd.ServeHTTP(rec, req) |
929 | + c.Check(rec.Code, check.Equals, http.StatusUnauthorized, check.Commentf(method)) |
930 | + |
931 | + cmd.GuestOK = true |
932 | + rec = httptest.NewRecorder() |
933 | + cmd.ServeHTTP(rec, req) |
934 | + if method == "GET" { |
935 | + c.Check(rec.Code, check.Equals, http.StatusOK, check.Commentf(method)) |
936 | + } else { |
937 | + c.Check(rec.Code, check.Equals, http.StatusUnauthorized, check.Commentf(method)) |
938 | + } |
939 | + cmd.GuestOK = false |
940 | + |
941 | + rec = httptest.NewRecorder() |
942 | + setBasicAuth(req, "pass", "word") |
943 | + cmd.ServeHTTP(rec, req) |
944 | + c.Check(rec.Code, check.Equals, http.StatusOK, check.Commentf(method)) |
945 | } |
946 | |
947 | req, err := http.NewRequest("POTATO", "", nil) |
948 | c.Assert(err, check.IsNil) |
949 | + setBasicAuth(req, "pass", "word") |
950 | rec := httptest.NewRecorder() |
951 | cmd.ServeHTTP(rec, req) |
952 | c.Check(rec.Code, check.Equals, http.StatusMethodNotAllowed) |
953 | |
954 | === modified file 'daemon/response.go' |
955 | --- daemon/response.go 2015-10-08 12:52:40 +0000 |
956 | +++ daemon/response.go 2015-10-09 13:33:12 +0000 |
957 | @@ -177,4 +177,5 @@ |
958 | BadMethod = ErrorResponse(http.StatusMethodNotAllowed) |
959 | InternalError = ErrorResponse(http.StatusInternalServerError) |
960 | NotImplemented = ErrorResponse(http.StatusNotImplemented) |
961 | + Unauthorized = ErrorResponse(http.StatusUnauthorized) |
962 | ) |
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.