Merge lp://staging/~cmars/juju-core/ecdsa-tls into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+219230_code.launchpad.net,

Message:
Please take a look.

Description:
ECDSA TLS cert support, default for new certs

ECDSA P-256 provides an equivalent to 128-bits of security, an
improvement
over RSA-1024 (equivalent to 80-bits of security)[1].

Provided benchmarks indicate that a P-256 TLS handshake incurs a 3ms
increase
in CPU time over RSA-1024 (about 48%). However, RSA-2048 (still weaker
than P-256) incurs a 90% increase in CPU time for a TLS handshake.

PASS: cert_test.go:172: certSuite.BenchmarkEcdsa256Handshake 200
9425053 ns/op
PASS: cert_test.go:185: certSuite.BenchmarkEcdsa256Sha256Handshake
200 9396808 ns/op
PASS: cert_test.go:146: certSuite.BenchmarkRsa1024Handshake 500
6355974 ns/op
PASS: cert_test.go:159: certSuite.BenchmarkRsa2048Handshake 100
12092335 ns/op

[1]
http://csrc.nist.gov/publications/nistpubs/800-57/sp800-57_part1_rev3_general.pdf,
Table 2, p. 64

https://code.launchpad.net/~cmars/juju-core/ecdsa-tls/+merge/219230

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/100400043/

Affected files (+475, -164 lines):
   A [revision details]
   M agent/agent.go
   M agent/agent_test.go
   M agent/bootstrap_test.go
   M agent/format-1.18.go
   M agent/format_whitebox_test.go
   M agent/identity_test.go
   M agent/mongo/mongo.go
   M agent/mongo/mongo_test.go
   M cert/cert.go
   M cert/cert_test.go
   M cmd/jujud/agent_test.go
   M cmd/jujud/bootstrap_test.go
   M environs/cloudinit.go
   M environs/cloudinit/cloudinit_test.go
   M environs/config/config.go
   M environs/config/config_test.go
   M environs/httpstorage/backend.go
   M environs/open.go
   M state/api/params/params.go
   M testing/cert.go
   M testing/mgo.go
   M worker/rsyslog/worker.go

On 2014/05/12 16:22:13, cmars wrote:
> Please take a look.

Just realized I need to set the SubjectKeyId for the CA cert.. need to
figure out how to do this for the ECDSA public key.

https://codereview.appspot.com/100400043/

Please take a look.

https://codereview.appspot.com/100400043/

This looks broadly sane to me, but (1) I'm not clear on what happens
around upgrades/HA, and (2) I'm not sue what the macro-level impact is
here. Better performance when bouncing large systems? Ping me when
you're online and we can chat.

FWIW, I'd really like to see the uncertainties in the original code
addressed.

Marking WIP pending discussion.

https://codereview.appspot.com/100400043/diff/10001/cert/cert.go
File cert/cert.go (right):

https://codereview.appspot.com/100400043/diff/10001/cert/cert.go#newcode228
cert/cert.go:228: SerialNumber: new(big.Int),
If we're touching this code, can we please fix this as well?

https://codereview.appspot.com/100400043/diff/10001/cert/cert_test.go
File cert/cert_test.go (right):

https://codereview.appspot.com/100400043/diff/10001/cert/cert_test.go#newcode98
cert/cert_test.go:98: //c.Assert(caCert.MaxPathLen, Equals, 0) TODO it
ends up as -1 - check that this is ok.
I hadn't noticed this before -- would you please figure out what's the
deal here?

https://codereview.appspot.com/100400043/diff/10001/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/100400043/diff/10001/environs/cloudinit.go#newcode146
environs/cloudinit.go:146: return errgo.Annotate(err, "cannot generate
state server certificate")
We're using juju.errors now, I think? But I haven't checked whether
thumper's changes have landed yet, please verify.

https://codereview.appspot.com/100400043/

Please take a look.

https://codereview.appspot.com/100400043/diff/10001/cert/cert.go
File cert/cert.go (right):

https://codereview.appspot.com/100400043/diff/10001/cert/cert.go#newcode228
cert/cert.go:228: SerialNumber: new(big.Int),
On 2014/05/13 07:21:18, fwereade wrote:
> If we're touching this code, can we please fix this as well?

Done. Will use a random 160-bit value here (max allowed by the spec).

https://codereview.appspot.com/100400043/diff/10001/cert/cert_test.go
File cert/cert_test.go (right):

https://codereview.appspot.com/100400043/diff/10001/cert/cert_test.go#newcode98
cert/cert_test.go:98: //c.Assert(caCert.MaxPathLen, Equals, 0) TODO it
ends up as -1 - check that this is ok.
On 2014/05/13 07:21:19, fwereade wrote:
> I hadn't noticed this before -- would you please figure out what's the
deal
> here?

Known issue, should be fixed in Go 1.3. Commented the test.

https://codereview.appspot.com/100400043/diff/10001/environs/cloudinit.go
File environs/cloudinit.go (right):

https://codereview.appspot.com/100400043/diff/10001/environs/cloudinit.go#newcode146
environs/cloudinit.go:146: return errgo.Annotate(err, "cannot generate
state server certificate")
On 2014/05/13 07:21:19, fwereade wrote:
> We're using juju.errors now, I think? But I haven't checked whether
thumper's
> changes have landed yet, please verify.

Need to merge w/latest trunk.

https://codereview.appspot.com/100400043/

Merge w/latest trunk & addressed comments.

Adding Nate Finch -- please review from an HA perspective. A separate
server certificate is now issued for the state server API endpoint &
MongoDB, because:

1. It's generally a good idea not to share private keys & certificates
among services.

2. MongoDB (as it is packaged, at least) doesn't seem to support serving
TLS with an ECDSA private key & server certificate. It does seem to
support serving with an RSA key & cert issued by an ECDSA CA -- but will
this be a problem for HA interconnects when there is an ECDSA CA in the
chain?

https://codereview.appspot.com/100400043/

On 2014/05/15 18:57:02, cmars wrote:
> Merge w/latest trunk & addressed comments.

> Adding Nate Finch -- please review from an HA perspective. A separate
server
> certificate is now issued for the state server API endpoint & MongoDB,
because:

> 1. It's generally a good idea not to share private keys & certificates
among
> services.

> 2. MongoDB (as it is packaged, at least) doesn't seem to support
serving TLS
> with an ECDSA private key & server certificate. It does seem to
support serving
> with an RSA key & cert issued by an ECDSA CA -- but will this be a
problem for
> HA interconnects when there is an ECDSA CA in the chain?

We need to talk to michael/wayne re rsyslog ca-cert changes too.

https://codereview.appspot.com/100400043/

I think changing the type of certificate is fine. I'd like to know that
you've verified that various clients have no problems with the new certs
(the python juju wrappers specifically, but openssl/tls in general since
GUI connects via webbrowser's websocket implementations).

I did notice that there are a few places where we just pass nil in all
of our callers, which makes me wonder if we really want a parameter.

And we really want to do this as an upgrade step with a format version
number bump in agent.conf. We shouldn't be changing the file in place.

https://codereview.appspot.com/100400043/diff/30001/agent/format-1.18.go
File agent/format-1.18.go (right):

https://codereview.appspot.com/100400043/diff/30001/agent/format-1.18.go#newcode48
agent/format-1.18.go:48: StateDbKey string `yaml:",omitempty"`
So these changes shouldn't be done in the format-1.18, instead we should
have a format-1.20 that introduces them and then an upgrade step to
populate the new fields.

https://codereview.appspot.com/100400043/diff/30001/cert/cert.go
File cert/cert.go (left):

https://codereview.appspot.com/100400043/diff/30001/cert/cert.go#oldcode156
cert/cert.go:156: SerialNumber: new(big.Int),
For the changes here to the actual certificate content, I'm trusting
that you know what you're doing, because I don't actually know the spec.
I believe we had some problems in the past with some libraries not
liking our Certs (we were missing a field that said we could use the
certs in the way that we were, IIRC).

Have you run the generated certs through a few SSL libraries (openssl
and gnutls come to mind)?

It might be nice to have an automated construct that created a cert and
ran it against a battery of 3rd party implementations and made sure they
were happy. Not that you have to do that here, but maybe it would be
helpful?

https://codereview.appspot.com/100400043/diff/30001/cert/cert.go
File cert/cert.go (right):

https://codereview.appspot.com/100400043/diff/30001/cert/cert.go#newcode102
cert/cert.go:102: return x509.ECDSAWithSHA512, nil
This may be standard syntax, but odd that the RSA ones are SHA256 with
RSA, and the ECDSA ones are ECDSA with SHA.

https://codereview.appspot.com/100400043/diff/30001/cert/cert.go#newcode109
cert/cert.go:109: func PublicKey(privKey interface{}) (interface{},
error) {
It seems like rather than taking an interface{} and returning an
interface{}, would it be better to have rsa.PrivateKey be able to have a
PublicKey() method? And then you have a Key interface that is
PublicKey() and PublicKeyId() ?

I guess put another way, would it be better to have our own types
implementing an interface, rather than having functions that switch
based on type?

https://codereview.appspot.com/100400043/diff/30001/cert/cert_test.go
File cert/cert_test.go (right):

https://codereview.appspot.com/100400043/diff/30001/cert/cert_test.go#newcode75
cert/cert_test.go:75: for _, keyOpts := range testKeyOpts {
I always like to ask the question "is this actually better/clearer as a
table based test vs just having 3 test cases"?

Our tooling infrastructure doesn't make table based testing great (no
easy way to just one permutation, hard to ensure that...

Just marking this as WiP so that it will be clear when you're ready for it to be reviewed again.