trivial: add PasswordHash and RandomBytes

trivial: add PasswordHash and RandomBytes



https://code.launchpad.net/~rogpeppe/juju-core/110-environs-password-hash/+merge/128263

(do not edit description out of merge proposal)

[rog, 2012-10-05 14:32:43]

Please take a look.

[rog, 2012-10-05 15:32:00]

Please take a look.

[niemeyer, 2012-10-05 15:57:25]

Looking good. A couple of comments:

https://codereview.appspot.com/6615047/diff/4001/environs/password.go
File environs/password.go (right):

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode1
environs/password.go:1: package environs
Both of those functions are good fits for the trivial package.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode17
environs/password.go:17: _, err := io.ReadFull(rand.Reader, buf)
Please use a pseudo-random generator in a local variable that's seeded on
startup with time. Cloud machines are well known for having poor real
randomness, and we don't want to block a provider forever when firing 100
machines because we have to randomness.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode29
environs/password.go:29: return base64.StdEncoding.EncodeToString(h)
What's the length of the password? Note that there's little point in having a
password that is larger in size than the hash size that MonogDB uses on its
passwords internally (MD5, so 16 bytes, IIRC). Also, should make sure that the
size is not triggering the padding of base64, since otherwise the last
characters will be well-defined.

[rog, 2012-10-05 16:33:45]

Please take a look.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go
File environs/password.go (right):

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode1
environs/password.go:1: package environs
On 2012/10/05 15:57:25, niemeyer wrote:
> Both of those functions are good fits for the trivial package.

Done.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode17
environs/password.go:17: _, err := io.ReadFull(rand.Reader, buf)
On 2012/10/05 15:57:25, niemeyer wrote:
> Please use a pseudo-random generator in a local variable that's seeded on
> startup with time. Cloud machines are well known for having poor real
> randomness, and we don't want to block a provider forever when firing 100
> machines because we have to randomness.

This is pretty much what crypto/rand does anyway by reading from /dev/urandom,
which a) doesn't block and b) will surely contain at least the current time as
an entropy source, along with whatever else it can get its hands on.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode29
environs/password.go:29: return base64.StdEncoding.EncodeToString(h)
On 2012/10/05 15:57:25, niemeyer wrote:
> What's the length of the password? Note that there's little point in having a
> password that is larger in size than the hash size that MonogDB uses on its
> passwords internally (MD5, so 16 bytes, IIRC). Also, should make sure that the
> size is not triggering the padding of base64, since otherwise the last
> characters will be well-defined.

it's 76 bytes. i can reduce it down arbitrarily. if you think 16 bytes is good,
i'll use that. i don't think the base64 padding will make any difference - i'm
pretty sure that the security of a password is based on its entropy, and that's
not decreased by adding well-known bytes.

[niemeyer, 2012-10-05 17:07:05]

https://codereview.appspot.com/6615047/diff/4001/environs/password.go
File environs/password.go (right):

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode17
environs/password.go:17: _, err := io.ReadFull(rand.Reader, buf)
On 2012/10/05 16:33:45, rog wrote:
> On 2012/10/05 15:57:25, niemeyer wrote:
> > Please use a pseudo-random generator in a local variable that's seeded on
> > startup with time. Cloud machines are well known for having poor real
> > randomness, and we don't want to block a provider forever when firing 100
> > machines because we have to randomness.
> 
> This is pretty much what crypto/rand does anyway by reading from /dev/urandom,
> which a) doesn't block and b) will surely contain at least the current time as
> an entropy source, along with whatever else it can get its hands on.

Sounds good.

https://codereview.appspot.com/6615047/diff/4001/environs/password.go#newcode29
environs/password.go:29: return base64.StdEncoding.EncodeToString(h)
On 2012/10/05 16:33:45, rog wrote:
> On 2012/10/05 15:57:25, niemeyer wrote:
> > What's the length of the password? Note that there's little point in having
a
> > password that is larger in size than the hash size that MonogDB uses on its
> > passwords internally (MD5, so 16 bytes, IIRC). Also, should make sure that
the
> > size is not triggering the padding of base64, since otherwise the last
> > characters will be well-defined.
> 
> it's 76 bytes. i can reduce it down arbitrarily. if you think 16 bytes is
good,

Yes, 16 bytes pre-encoding sounds enough.

> i'll use that. i don't think the base64 padding will make any difference - i'm
> pretty sure that the security of a password is based on its entropy, and
that's
> not decreased by adding well-known bytes.

It's just silly to have "==" behind every parssword. Let's avoid that, please.

[rog, 2012-10-05 17:48:25]

Please take a look.

[rog, 2012-10-05 17:55:08]

*** Submitted:

trivial: add PasswordHash and RandomBytes

R=niemeyer
CC=
https://codereview.appspot.com/6615047