environs/config: make CA cert optional

environs/config: make CA cert optional

As discussed online.
Also make cmd/juju not use juju.Bootstrap, because
it's generating a PEM file we can't use.

https://code.launchpad.net/~rogpeppe/juju-core/160-config-root-cert-fixed/+merge/135479

Requires: https://code.launchpad.net/~rogpeppe/juju-core/162-config-root-cert-reapply/+merge/135468

(do not edit description out of merge proposal)

[rog, 2012-11-21 17:40:33]

Please take a look.

[rog, 2012-11-21 17:42:28]

Please take a look.

[rog, 2012-11-21 18:23:37]

Please take a look.

[TheMue, 2012-11-22 11:02:47]

My understanding of certs in keys isn't very good, so only small formal remarks.

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap.go#newcode61
cmd/juju/bootstrap.go:61: `)
The other CL has a [1:] here to suppress the leading newline. Is it missing
here?

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap_test.go#n...
cmd/juju/bootstrap_test.go:54: // TODO(rog) reenable
??? Sorry, don't know it.

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go#ne...
environs/config/config.go:143: //
One // too much. ;)

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

https://codereview.appspot.com/6850087/diff/3003/environs/config/config_test....
environs/config/config_test.go:365: func (*ConfigSuite) TestConfigNoCertFiles(c
*C) {
Isn't that covered above in "No CA cert or key"?

[rog, 2012-11-22 11:08:05]

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap.go#newcode61
cmd/juju/bootstrap.go:61: `)
On 2012/11/22 11:02:47, TheMue wrote:
> The other CL has a [1:] here to suppress the leading newline. Is it missing
> here?

It doesn't matter here, as we're not using it for tests that rely on the exact
content of it.

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/6850087/diff/3003/cmd/juju/bootstrap_test.go#n...
cmd/juju/bootstrap_test.go:54: // TODO(rog) reenable
On 2012/11/22 11:02:47, TheMue wrote:
> ??? Sorry, don't know it.

in a short while, we'll re-enable this check.

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

https://codereview.appspot.com/6850087/diff/3003/environs/config/config_test....
environs/config/config_test.go:365: func (*ConfigSuite) TestConfigNoCertFiles(c
*C) {
On 2012/11/22 11:02:47, TheMue wrote:
> Isn't that covered above in "No CA cert or key"?

No, because the "No CA cert or key" doesn't cover the logic that tries to read
from a file and ignores the file-not-found error.

[niemeyer, 2012-11-22 16:28:29]

Feels like the changes to config need a bit more care.

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

https://codereview.appspot.com/6850087/diff/3003/environs/config/authkeys.go#...
environs/config/authkeys.go:72: if len(certPEM) == 0 {
Documentation doesn't say this is optional too. Also, what happens if len(key) >
0 but len(cert) == 0? And how do we handle a file that is empty? It's certainly
not valid.

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go#ne...
environs/config/config.go:143: //
On 2012/11/22 11:02:47, TheMue wrote:
> One // too much. ;)

+1

Also please reindent the comment. It's a staircase ATM.

And what is m? It's undocumented, and it even changes it.

The result values need names.

What about that path? We pass a default path in, and get a path out? Who figured
what the path was?

This function needs some love.

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go#ne...
environs/config/config.go:153: m[attr] = string(data)
It loads the data out, converts it to []byte, converts it back to string, and
puts it back in, exactly as it was?

We can do this here instead, and remove the former if block:

if v, ok := m[attr]; ok {
    return []byte(v), "", nil
}

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go#ne...
environs/config/config.go:168: return data, path, nil
If a file doesn't exist we pretend it exists, and was empty? That sounds error
prone. We should handle that situation differently than if there's an actual
empty file.

[rog, 2012-11-22 19:07:19]

Please take a look.

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

https://codereview.appspot.com/6850087/diff/3003/environs/config/authkeys.go#...
environs/config/authkeys.go:72: if len(certPEM) == 0 {
On 2012/11/22 16:28:30, niemeyer wrote:
> Documentation doesn't say this is optional too.


yeah, maybe we should only call this function when
one of the two is non-empty.

> Also, what happens if len(key) >
> 0 but len(cert) == 0?

this case is handled fine - the X59KeyPair call
above will fail and return an appropriate error.

> And how do we handle a file that is empty? It's certainly
> not valid.

changed so that nil unambiguously signifies no data.

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6850087/diff/3003/environs/config/config.go#ne...
environs/config/config.go:143: //
On 2012/11/22 16:28:30, niemeyer wrote:
> This function needs some love.

very true. enough love provided now, i hope.

[rog, 2012-11-22 21:03:23]

Please take a look.

[rog, 2012-11-23 13:11:43]

Please take a look.

[niemeyer, 2012-11-23 14:16:21]

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:52: // provided as nil values.
// .. nil values to avoid having them read from the standard paths.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:140: // if the attribute is not set.
This is way too vague considering the complexity going on here.

// maybeReadFile sets m[attr] to:
//
// 1) Preserves m[attr] as nil if it was already nil
// 2) The content of the file m[attr+"-path"], if that's set
// 3) The content of defaultPath if it exists and m[attr] is unset ("")
// 4) Preserves the content of m[attr], otherwise
//
// The m[attr+"-path"] key is always deleted.

The complexity makes it a bit error prone. See below for example.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:145: if path == "" {
What if the path is != "" and m[attr] is nil, shouldn't it stay nil?

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:164: if os.IsNotExist(err) {
It should be an error to have a path specified that does not exist.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:195: // certifying authority, in PEM format.
// CACert returns the certificate of the CA that signed the state server
// certificate, and whether the setting is available.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:197: func (c *Config) CACertPEM() ([]byte, bool) {
This should actually be CACert, as mentioned in a previous review. That's how
every other method is named in Config.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:204: // It returns false if the key is not present.
// CAPrivateKey returns the private key of the CA that signed the state
// server certificate, and whether the setting is available.

[rog, 2012-11-23 14:43:58]

Please take a look.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:52: // provided as nil values.
On 2012/11/23 14:16:21, niemeyer wrote:
> // .. nil values to avoid having them read from the standard paths. 

Done.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:140: // if the attribute is not set.
On 2012/11/23 14:16:21, niemeyer wrote:
> This is way too vague considering the complexity going on here.
> 
> // maybeReadFile sets m[attr] to:
> //
> // 1) Preserves m[attr] as nil if it was already nil
> // 2) The content of the file m[attr+"-path"], if that's set
> // 3) The content of defaultPath if it exists and m[attr] is unset ("")
> // 4) Preserves the content of m[attr], otherwise
> //
> // The m[attr+"-path"] key is always deleted.
> 
> The complexity makes it a bit error prone. See below for example.

that's much better, thanks.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:145: if path == "" {
On 2012/11/23 14:16:21, niemeyer wrote:
> What if the path is != "" and m[attr] is nil, shouldn't it stay nil?

i don't think so, as discussed on line.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:164: if os.IsNotExist(err) {
On 2012/11/23 14:16:21, niemeyer wrote:
> It should be an error to have a path specified that does not exist.

good point. done.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:195: // certifying authority, in PEM format.
On 2012/11/23 14:16:21, niemeyer wrote:
> // CACert returns the certificate of the CA that signed the state server
> // certificate, and whether the setting is available.

Done.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:197: func (c *Config) CACertPEM() ([]byte, bool) {
On 2012/11/23 14:16:21, niemeyer wrote:
> This should actually be CACert, as mentioned in a previous review. That's how
> every other method is named in Config.

Done.

https://codereview.appspot.com/6850087/diff/13017/environs/config/config.go#n...
environs/config/config.go:204: // It returns false if the key is not present.
On 2012/11/23 14:16:21, niemeyer wrote:
> // CAPrivateKey returns the private key of the CA that signed the state
> // server certificate, and whether the setting is available.

Done.

[niemeyer, 2012-11-23 15:29:16]

LGTM

[rog, 2012-11-23 16:10:27]

*** Submitted:

environs/config: make CA cert optional

As discussed online.
Also make cmd/juju not use juju.Bootstrap, because
it's generating a PEM file we can't use.

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6850087