juju: implement Bootstrap

juju: implement Bootstrap

This generates the initial TLS certificate
for the bootstrap instance.

https://code.launchpad.net/~rogpeppe/juju-core/145-juju-bootstrap/+merge/134339

(do not edit description out of merge proposal)

[rog, 2012-11-14 17:37:08]

Please take a look.

[niemeyer, 2012-11-15 00:39:49]

The structure looks good, but this is pretty dense. I'll have to look at it
again with a fresher mind.

[TheMue, 2012-11-15 13:04:22]

So far it looks good, I only have to dig deeper into the X509 stuff.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode80
juju/bootstrap.go:80: MaxPathLen: 1,
Interesting, go fmt problem due to long identifier?

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode84
juju/bootstrap.go:84: return nil, fmt.Errorf("canot create certificate: %v",
err)
Wording differs: certificate, root CA, bootstrap certificate, root certificate.

[niemeyer, 2012-11-16 17:53:23]

Looking good. A bunch of details, but nothing major:

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode23
juju/bootstrap.go:23: // authority certificate and private key in PEM format can
be given in
Seems a bit too wordy. I suggest simplifying to:

"The root certificate and private key in PEM format"

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode24
juju/bootstrap.go:24: // rootPEM; if this is nil, the root CA certificate and
key pair is read
"root certificate"

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode25
juju/bootstrap.go:25: // from $HOME/.juju/<environ-name>-cert.pem, or generated
and written
As per prior reviews, if we have both in a single file, it should be called
<environ name>.pem

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode53
juju/bootstrap.go:53: // TODO make sure that the environment name cannot
I don't think that's needed here. We're calling this from an environ.Environ.
Whoever built that should have validated.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode55
juju/bootstrap.go:55: path := filepath.Join(os.Getenv("HOME"), ".juju",
envName+"-cert.pem")
s/-cert//

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode56
juju/bootstrap.go:56: data, err := ioutil.ReadFile(path)
if err == nil {
    return data, nil
} else if !os.IsNotExist(err) {
    return nil, err
}

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode71
juju/bootstrap.go:71: CommonName:   "juju-generated root CA for environment " +
envName,
envName should be quoted.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode74
juju/bootstrap.go:74: NotBefore:             now.Add(-5 * time.Minute).UTC(),
now.UTC().Add, to reflect bottom line?

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode80
juju/bootstrap.go:80: MaxPathLen: 1,
As discussed, we need to understand what this really means.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode84
juju/bootstrap.go:84: return nil, fmt.Errorf("canot create certificate: %v",
err)
cannot

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode101
juju/bootstrap.go:101: func generateBootstrapCert(envName string, rootCert
*x509.Certificate, rootKey *rsa.PrivateKey) ([]byte, error) {
s/Bootstrap//

This function generates a certificate with the given data, no matter how we use
it.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode115
juju/bootstrap.go:115: CommonName:   "anyServer",
"*" + no ServerName on connectins, as discussed. The comment can also go away
with that, I think.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode118
juju/bootstrap.go:118: NotBefore: now.Add(-5 * time.Minute).UTC(),
now.UTC().Add as below

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode146
juju/bootstrap.go:146: func parseRootPEM(rootPEM []byte, requirePrivateKey bool)
(cert *x509.Certificate, priv *rsa.PrivateKey, err error) {
The result names don't look necessary here. They are actually potentially error
prone, since there's shadowing going on below.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode194
juju/bootstrap.go:194: return nil, nil, fmt.Errorf("certificate private key has
unexpected type %T", tlsCert.PrivateKey)
s/certificate private key/root CA private key/

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode197
juju/bootstrap.go:197: return nil, nil, fmt.Errorf("expected 1 certificate, got
%d", len(tlsCert.Certificate))
"root CA has %d certificates, want 1"

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode198
juju/bootstrap.go:198: }
The logic below is an exact copy of lines 175-183. We can easily avoid this with
an "else" clause on keyBlock == nil.

[rog, 2012-11-19 12:28:39]

Please take a look.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode23
juju/bootstrap.go:23: // authority certificate and private key in PEM format can
be given in
On 2012/11/16 17:53:23, niemeyer wrote:
> Seems a bit too wordy. I suggest simplifying to:
> 
> "The root certificate and private key in PEM format"

Looking at your commments in upstream CL, I've gone with "CA" here, and I'll try
to be consistent with that.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode25
juju/bootstrap.go:25: // from $HOME/.juju/<environ-name>-cert.pem, or generated
and written
On 2012/11/16 17:53:23, niemeyer wrote:
> As per prior reviews, if we have both in a single file, it should be called
> <environ name>.pem

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode53
juju/bootstrap.go:53: // TODO make sure that the environment name cannot
On 2012/11/16 17:53:23, niemeyer wrote:
> I don't think that's needed here. We're calling this from an environ.Environ.
> Whoever built that should have validated.

yeah. we should add the check to environs/config.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode55
juju/bootstrap.go:55: path := filepath.Join(os.Getenv("HOME"), ".juju",
envName+"-cert.pem")
On 2012/11/16 17:53:23, niemeyer wrote:
> s/-cert//

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode56
juju/bootstrap.go:56: data, err := ioutil.ReadFile(path)
On 2012/11/16 17:53:23, niemeyer wrote:
> if err == nil {
>     return data, nil
> } else if !os.IsNotExist(err) {
>     return nil, err
> }

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode71
juju/bootstrap.go:71: CommonName:   "juju-generated root CA for environment " +
envName,
On 2012/11/16 17:53:23, niemeyer wrote:
> envName should be quoted.

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode74
juju/bootstrap.go:74: NotBefore:             now.Add(-5 * time.Minute).UTC(),
On 2012/11/16 17:53:23, niemeyer wrote:
> now.UTC().Add, to reflect bottom line?

yeah, though it doesn't make any difference in this case.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode80
juju/bootstrap.go:80: MaxPathLen: 1,
On 2012/11/16 17:53:23, niemeyer wrote:
> As discussed, we need to understand what this really means.

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode101
juju/bootstrap.go:101: func generateBootstrapCert(envName string, rootCert
*x509.Certificate, rootKey *rsa.PrivateKey) ([]byte, error) {
On 2012/11/16 17:53:23, niemeyer wrote:
> s/Bootstrap//
> 
> This function generates a certificate with the given data, no matter how we
use
> it.

the metadata in the certificate is the information we want the bootstrap
certificate to have - that's what makes this a "bootstrap certificate". but done
anyway; i don't mind much.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode115
juju/bootstrap.go:115: CommonName:   "anyServer",
On 2012/11/16 17:53:23, niemeyer wrote:
> "*" + no ServerName on connectins, as discussed. The comment can also go away
> with that, I think.

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode118
juju/bootstrap.go:118: NotBefore: now.Add(-5 * time.Minute).UTC(),
On 2012/11/16 17:53:23, niemeyer wrote:
> now.UTC().Add as below

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode146
juju/bootstrap.go:146: func parseRootPEM(rootPEM []byte, requirePrivateKey bool)
(cert *x509.Certificate, priv *rsa.PrivateKey, err error) {
On 2012/11/16 17:53:23, niemeyer wrote:
> The result names don't look necessary here. They are actually potentially
error
> prone, since there's shadowing going on below.

Done.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode198
juju/bootstrap.go:198: }
On 2012/11/16 17:53:23, niemeyer wrote:
> The logic below is an exact copy of lines 175-183. We can easily avoid this
with
> an "else" clause on keyBlock == nil.

done, slightly differently.

[niemeyer, 2012-11-19 13:07:31]

Thanks. LGTM with trivials:

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode53
juju/bootstrap.go:53: // TODO make sure that the environment name cannot
On 2012/11/19 12:28:40, rog wrote:
> On 2012/11/16 17:53:23, niemeyer wrote:
> > I don't think that's needed here. We're calling this from an
environ.Environ.
> > Whoever built that should have validated.
> 
> yeah. we should add the check to environs/config.

Please drop the TODO then.

https://codereview.appspot.com/6843059/diff/2001/juju/bootstrap.go#newcode101
juju/bootstrap.go:101: func generateBootstrapCert(envName string, rootCert
*x509.Certificate, rootKey *rsa.PrivateKey) ([]byte, error) {
On 2012/11/19 12:28:40, rog wrote:
> the metadata in the certificate is the information we want the bootstrap
> certificate to have - that's what makes this a "bootstrap certificate". but
done

There's no such thing as a "bootstrap certificate" in our implementation. We
have a bunch of other terms, though, which we should keep reusing when we want
to refer to these respective things.

https://codereview.appspot.com/6843059/diff/12001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/12001/juju/bootstrap.go#newcode109
juju/bootstrap.go:109: // host names containing dots.
// This won't match host names with dots. The hostname
// is hardcoded when connecting to avoid the issue.

[rog, 2012-11-19 14:05:51]

*** Submitted:

juju: implement Bootstrap

This generates the initial TLS certificate
for the bootstrap instance.

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

https://codereview.appspot.com/6843059/diff/12001/juju/bootstrap.go
File juju/bootstrap.go (right):

https://codereview.appspot.com/6843059/diff/12001/juju/bootstrap.go#newcode109
juju/bootstrap.go:109: // host names containing dots.
On 2012/11/19 13:07:31, niemeyer wrote:
> // This won't match host names with dots. The hostname
> // is hardcoded when connecting to avoid the issue.

Done.