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

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