environs: change Bootstrap cert generation

environs: change Bootstrap cert generation

Bootstrap now gets its certificates from the environment,
and stores them by calling a function.

https://code.launchpad.net/~rogpeppe/juju-core/164-bootstrap-generate/+merge/136222

(do not edit description out of merge proposal)

[rog, 2012-11-26 17:00:06]

Please take a look.

[dave_cheney.net, 2012-11-27 00:51:02]

LGTM

https://codereview.appspot.com/6782119/diff/2001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6782119/diff/2001/juju/conn_test.go#newcode192
juju/conn_test.go:192: func panicWrite(name string, data []byte) error {
This guy is duplicated a few times, maybe move it to juju/testing, or
export_test ?

[niemeyer, 2012-11-27 16:21:28]

A few initial comments:

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode36
environs/bootstrap.go:36: log.Printf("got cert and key: %v, %v", hasCACert,
hasCAKey)
Log messages target actual users. This should be either removed  or tailored.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode39
environs/bootstrap.go:39: return fmt.Errorf("environment config has private key
without CA certificate")
"environment configuration with CA private key but no certificate"

The private key is also for the CA.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode51
environs/bootstrap.go:51: return fmt.Errorf("cannot create config with added CA
certificate: %v", err)
"cannot create environment configuration with new CA: %v"

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode54
environs/bootstrap.go:54: return fmt.Errorf("cannot add CA certificate to
environ: %v", err)
"cannot set environment configuration with CA: %v"

I'd prefer to avoid shortening words when talking to the user.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode56
environs/bootstrap.go:56: if err := writeCertFile(environ.Name()+"-cert.pem",
caCertPEM); err != nil {
The name should be the environment name, not the file name. It's up to the
function to decide how to name the file, or whether to use a file at all in
fact.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode57
environs/bootstrap.go:57: return fmt.Errorf("cannot save CA certificate: %v",
err)
s/save/write/

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode60
environs/bootstrap.go:60: return fmt.Errorf("cannot save CA key: %v", err)
s/save/write/

[rog, 2012-11-27 16:50:21]

Please take a look.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode36
environs/bootstrap.go:36: log.Printf("got cert and key: %v, %v", hasCACert,
hasCAKey)
On 2012/11/27 16:21:28, niemeyer wrote:
> Log messages target actual users. This should be either removed  or tailored.

debugging relic that slipped through the net. removed.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode39
environs/bootstrap.go:39: return fmt.Errorf("environment config has private key
without CA certificate")
On 2012/11/27 16:21:28, niemeyer wrote:
> "environment configuration with CA private key but no certificate"
> 
> The private key is also for the CA.

Done.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode51
environs/bootstrap.go:51: return fmt.Errorf("cannot create config with added CA
certificate: %v", err)
On 2012/11/27 16:21:28, niemeyer wrote:
> "cannot create environment configuration with new CA: %v"

Done.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode54
environs/bootstrap.go:54: return fmt.Errorf("cannot add CA certificate to
environ: %v", err)
On 2012/11/27 16:21:28, niemeyer wrote:
> "cannot set environment configuration with CA: %v"
> 
> I'd prefer to avoid shortening words when talking to the user.

Done.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode56
environs/bootstrap.go:56: if err := writeCertFile(environ.Name()+"-cert.pem",
caCertPEM); err != nil {
On 2012/11/27 16:21:28, niemeyer wrote:
> The name should be the environment name, not the file name. It's up to the
> function to decide how to name the file, or whether to use a file at all in
> fact.

Done.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode57
environs/bootstrap.go:57: return fmt.Errorf("cannot save CA certificate: %v",
err)
On 2012/11/27 16:21:28, niemeyer wrote:
> s/save/write/

Done.

https://codereview.appspot.com/6782119/diff/2001/environs/bootstrap.go#newcode60
environs/bootstrap.go:60: return fmt.Errorf("cannot save CA key: %v", err)
On 2012/11/27 16:21:28, niemeyer wrote:
> s/save/write/

Done.

[niemeyer, 2012-11-27 17:01:23]

This is great. LGTM with one trivial suggestion:

https://codereview.appspot.com/6782119/diff/2002/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/6782119/diff/2002/environs/bootstrap.go#newcod...
environs/bootstrap.go:126: return nil, nil, fmt.Errorf("more than one
certificate for CA")
"generated CA key pair must have 1 certificate, not %d"

(zero would also blow that up)

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

*** Submitted:

environs: change Bootstrap cert generation

Bootstrap now gets its certificates from the environment,
and stores them by calling a function.

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6782119

https://codereview.appspot.com/6782119/diff/2002/environs/bootstrap.go
File environs/bootstrap.go (right):

https://codereview.appspot.com/6782119/diff/2002/environs/bootstrap.go#newcod...
environs/bootstrap.go:126: return nil, nil, fmt.Errorf("more than one
certificate for CA")
On 2012/11/27 17:01:23, niemeyer wrote:
> "generated CA key pair must have 1 certificate, not %d"
> 
> (zero would also blow that up)

done, although zero can never actually happen in fact.