ECDSA TLS cert support, default for new certs

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)

[cmars, 2014-05-12 16:22:13]

Please take a look.

[cmars, 2014-05-12 16:31:16]

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.

[cmars, 2014-05-12 22:19:25]

Please take a look.

[fwereade, 2014-05-13 07:21:19]

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#new...
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.

[cmars, 2014-05-15 18:50:18]

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#new...
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.

[cmars, 2014-05-15 18:57:02]

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?

[fwereade, 2014-05-16 09:23:55]

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.

[jameinel, 2014-05-27 13:20:09]

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#newc...
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 the table tests are actually
isolated, etc).

https://codereview.appspot.com/100400043/diff/30001/cert/cert_test.go#newcode101
cert/cert_test.go:101: //c.Assert(caCert.MaxPathLen, Equals, 0)	TODO it ends up
as -1 - check that this is ok.
We can use a c.ExpectFailure() here to not have this commented out, though I
think it will miss the fact that if anything actually breaks then the test will
still not fail.

Also, did you do the "TODO it ends up as -1" ?

https://codereview.appspot.com/100400043/diff/30001/cert/cert_test.go#newcode106
cert/cert_test.go:106: for _, caKeyOpts := range testKeyOpts {
I guess this test means we want to have the testKeyOpts slice anyway, since we
want to do many-to-many permutation testing.

https://codereview.appspot.com/100400043/diff/30001/cert/cert_test.go#newcode199
cert/cert_test.go:199: }
FWIW, I don't really care about the per-connection overhead, as anything on the
order of MS is unobservable when your ping is already 200ms for Juju clients.

I realize you probably had some different requirements, so I'm interested in the
performance, but I don't think it matters for Juju clients.
I suppose it matters a bit more when we have 10,000 agents all trying to connect
to the api server roughly concurrently. Though I have the feeling Login, et al,
are still going to dominate the time.

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

https://codereview.appspot.com/100400043/diff/30001/environs/config/config.go...
environs/config/config.go:963: func (cfg *Config)
GenerateStateServerCertAndKey(keyOpts *cert.KeyOptions) (string, string, error)
{
If we're just going to use the default, is there a reason to actually expose
keyOpts here? I can understand at the lower level, because it is more of a
library function, but it feels like at the environs/config level unless we are
going to actually let users configure this (which seems unnecessary ATM), then
it is just adding complexity to our internals.

If you have a concrete use case, I don't have a problem with it, just wondering
why.

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

https://codereview.appspot.com/100400043/diff/30001/testing/cert.go#newcode38
testing/cert.go:38: })
I almost wish we could just disable SSL entirely for the test suite. I can say
we should have tests of the SSL code paths, but all of the tests of our general
logic really don't need to be going via SSL to a localhost server.