environs/config: add ca-cert and ca-private-key

environs/config: add ca-cert and ca-private-key

Unfortunately the knock-on changes are quite
widespread.

https://code.launchpad.net/~rogpeppe/juju-core/140-config-root-cert/+merge/134717

Requires: https://code.launchpad.net/~rogpeppe/juju-core/146-cmd-bootstrap-use-juju/+merge/134437

(do not edit description out of merge proposal)

[rog, 2012-11-16 18:02:59]

Please take a look.

[niemeyer, 2012-11-16 19:51:17]

Again, a few comments, but really great stuff.

https://codereview.appspot.com/6846066/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6846066/diff/2001/cmd/juju/cmd_test.go#newcode46
cmd/juju/cmd_test.go:46: []byte(envConfig), 0666)
That's a pretty short line for wrapping like this.

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go#...
environs/config/authkeys.go:65: func readCertFile(path string, defaultPath
string) ([]byte, error) {
s/Cert/Config/?  There's nothing about certs below.

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go#...
environs/config/authkeys.go:73: log.Printf("reading cert file %q", path)
Doesn't feel like an interesting event.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:33: // TODO(rog) update the doc comment below - it's
getting messy
This branch is the one making it hard to read. Let's please avoid this TODO by
not introducing the problem in the first place.

Part of the problem is also being created by the level of detail. "Authorized
SSH keys and the root CA details are read from ~/.juju by default." kills 10
lines from that explanation, for example.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:50: // "root-cert", all of type string.  Additional
keys recognised are:
Not entirely right (and was already bogus before). It's possible to have a valid
configuration without authorized-keys and root-cert being explicitly mentioned.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:85: 
Reading the code below, it occurs to me that we could improve names to reduce
the length, make it more consistent, and avoid some ambiguity ("root" has a
nearby meaning):

Attributes: ca-cert, ca-private-key
Method names: config.CACert(), config.CAPrivateKey()
Filenames: <env name>-cert.pem <envname>-private-key.pem

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:86: // Load root certificate into root-cert if
necessary.
// Load CA certificate into ca-cert if necessary.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:87: rootCert := []byte(c.m["root-cert"].(string))
s/rootCert/caCert/; s/root-cert/ca-cert/; etc

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:90: rootCert, err = readCertFile(rootCertPath,
name+"-root-cert.pem")
name+"-cert.pem"

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:98: // Load root key into root-private-cert if
necessary.
// Load CA private key into ca-private-key if necessary.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:107: rootKey, err = readCertFile(rootKeyPath,
name+"-root-key.pem")
name+"-private-key.pem"

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:176: func (c *Config) RootCertPEM() string {
CACert

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:183: func (c *Config) RootPrivateKeyPEM() string {
CAPrivateKey

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:273: "root-cert-path":        "",
The order here should be the same as the one used in the definition of "fields"
above.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/config_test....
environs/config/config_test.go:28: about string
s/about/summary/; (that's how we call it elsewhere)

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go
File testing/cert.go (right):

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode12
testing/cert.go:12: RootCertPEM = `
CACertPEM

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode26
testing/cert.go:26: RootCertPEMBytes = []byte(RootCertPEM)
That seems a bit over the top. []byte(foo) is okay.

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode28
testing/cert.go:28: RootCertX509 = mustParseCertPEM(RootCertPEM)
CACertX509

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode30
testing/cert.go:30: RootKeyPEM = `
CAPrivateKey

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode41
testing/cert.go:41: RootKeyPEMBytes = []byte(RootKeyPEM)
d

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode44
testing/cert.go:44: RootPEMBytes = []byte(RootCertPEM + RootKeyPEM)
d; we really don't need all trivial variations of the theme pre-defined.

https://codereview.appspot.com/6846066/diff/2001/testing/cert.go#newcode46
testing/cert.go:46: RootKeyRSA = mustParseKeyPEM(RootKeyPEM)
CAPrivateKeyRSA

[jameinel, 2012-11-18 10:10:02]

Some follow up comments from me. It seems we could collapse a few of the
definitions of 'rootCert' into using the testing versions.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:33: // TODO(rog) update the doc comment below - it's
getting messy
I would probably go with 'if not specified' vs 'by default'. And for *me* it is
useful to see the specific paths that it would be read from. Possibly just
listing the filenames.

If not specified, authorized SSH keys and root CA details will be read from:
 ~/.ssh/authorized-keys
 ~/.juju/<name>-root-cert.pem
 ~/.juju/<name>-root-key.pem

Also note that your simplified form makes it sound like authorized keys would be
read from ~/.juju, while they are actually read from ~/.ssh

https://codereview.appspot.com/6846066/diff/2001/juju/conn_test.go
File juju/conn_test.go (left):

https://codereview.appspot.com/6846066/diff/2001/juju/conn_test.go#oldcode41
juju/conn_test.go:41: }
I'm just noticing, it seems a little unmatched to call it 'root-cert' and
'root-private-key', vs root-key. (certainly in directories I've seen, you have
foo.cert and foo.key)
I doubt it matters in the long run, but something to consider.

https://codereview.appspot.com/6846066/diff/2001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6846066/diff/2001/juju/testing/conn.go#newcode186
juju/testing/conn.go:186: 
It seems odd to be repeating these in quite a few files, when is you've already
established testing credentials elsewhere.

This one also contains both the cert and the key. You also appear to be using
the testing versions above, but are adding ones here. Should this just be
removed?

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go#oldcode394
state/state_test.go:394: initial := map[string]interface{}{
I'm assuming this is go fmt that is re-indenting all of these lines causing
noise in the diff, right?

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go#newcode1187
state/state_test.go:1187: 
Another place where it seems better to use a common root cert value.

[rog, 2012-11-20 11:15:16]

Please take a look.

https://codereview.appspot.com/6846066/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6846066/diff/2001/cmd/juju/cmd_test.go#newcode46
cmd/juju/cmd_test.go:46: []byte(envConfig), 0666)
On 2012/11/16 19:51:17, niemeyer wrote:
> That's a pretty short line for wrapping like this.

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go#...
environs/config/authkeys.go:65: func readCertFile(path string, defaultPath
string) ([]byte, error) {
On 2012/11/16 19:51:17, niemeyer wrote:
> s/Cert/Config/?  There's nothing about certs below.

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/authkeys.go#...
environs/config/authkeys.go:73: log.Printf("reading cert file %q", path)
On 2012/11/16 19:51:17, niemeyer wrote:
> Doesn't feel like an interesting event.

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:33: // TODO(rog) update the doc comment below - it's
getting messy
On 2012/11/18 10:10:02, john.meinel wrote:
> I would probably go with 'if not specified' vs 'by default'. And for *me* it
is
> useful to see the specific paths that it would be read from. Possibly just
> listing the filenames.
> 
> If not specified, authorized SSH keys and root CA details will be read from:
>  ~/.ssh/authorized-keys
>  ~/.juju/<name>-root-cert.pem
>  ~/.juju/<name>-root-key.pem
> 
> Also note that your simplified form makes it sound like authorized keys would
be
> read from ~/.juju, while they are actually read from ~/.ssh

I've gone with something similar to this.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:86: // Load root certificate into root-cert if
necessary.
On 2012/11/16 19:51:17, niemeyer wrote:
> // Load CA certificate into ca-cert if necessary.

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:87: rootCert := []byte(c.m["root-cert"].(string))
On 2012/11/16 19:51:17, niemeyer wrote:
> s/rootCert/caCert/; s/root-cert/ca-cert/; etc

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:90: rootCert, err = readCertFile(rootCertPath,
name+"-root-cert.pem")
On 2012/11/16 19:51:17, niemeyer wrote:
> name+"-cert.pem"

Done.

https://codereview.appspot.com/6846066/diff/2001/environs/config/config.go#ne...
environs/config/config.go:98: // Load root key into root-private-cert if
necessary.
On 2012/11/16 19:51:17, niemeyer wrote:
> // Load CA private key into ca-private-key if necessary.

Done.

https://codereview.appspot.com/6846066/diff/2001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6846066/diff/2001/juju/testing/conn.go#newcode186
juju/testing/conn.go:186: 
On 2012/11/18 10:10:02, john.meinel wrote:
> It seems odd to be repeating these in quite a few files, when is you've
already
> established testing credentials elsewhere.
> 
> This one also contains both the cert and the key. You also appear to be using
> the testing versions above, but are adding ones here. Should this just be
> removed?

it should indeed; it's just a relic.

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go#oldcode394
state/state_test.go:394: initial := map[string]interface{}{
On 2012/11/18 10:10:02, john.meinel wrote:
> I'm assuming this is go fmt that is re-indenting all of these lines causing
> noise in the diff, right?

indeed it is. of course, a conscientious code editor might well do the same
thing to maintain code prettiness. :-)

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6846066/diff/2001/state/state_test.go#newcode1187
state/state_test.go:1187: 
On 2012/11/18 10:10:02, john.meinel wrote:
> Another place where it seems better to use a common root cert value.

Done.

[niemeyer, 2012-11-20 13:48:52]

LGTM!

[rog, 2012-11-20 14:29:02]

*** Submitted:

environs/config: add ca-cert and ca-private-key

Unfortunately the knock-on changes are quite
widespread.

R=niemeyer, john.meinel
CC=
https://codereview.appspot.com/6846066

[rog, 2012-11-21 15:53:22]

Please take a look.