Merge lp:~gz/juju-core/1.16_ssl_verification_bootstrap_state_1268913 into lp:juju-core/1.16

[Martin Packman]

Obey ssl-hostname-verification for provider-state

When reading the provider-state file from cloud storage on
bootstrap, skip validating the https cert if the config
ssl-hostname-verification is set to false.

https://codereview.appspot.com/56560043/

R=axwalk

Obey ssl-hostname-verification for provider-state

When reading the provider-state file from cloud storage on
bootstrap, skip validating the https cert if the config
ssl-hostname-verification is set to false.

Change simplified to not test the valid cert case, avoiding
the need for a usable testing https server.

https://codereview.appspot.com/56560043/

[Martin Packman (gz)]

Reviewers: mp+203037_code.launchpad.net,

Message:
Please take a look.

Description:
Obey ssl-hostname-verification for provider-state

When reading the provider-state file from cloud storage on
bootstrap, skip validating the https cert if the config
ssl-hostname-verification is set to false.

In order to reuse the existing testing infrastructure for
localStorage, I had to add a new PromotableStorage
interface which does the (overly magic) lookup of the
https endpoint via http head and uses it for *all*
requests, not just the modification ones.

On trunk, I don't see why ClientTLS shouldn't be switched
to just use https all the time.

https://code.launchpad.net/~gz/juju-core/1.16_ssl_verification_bootstrap_state_1268913/+merge/203037

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/56560043/

Affected files (+169, -53 lines):
   A [revision details]
   M cmd/jujud/bootstrap.go
   M environs/httpstorage/backend_test.go
   M environs/httpstorage/storage.go
   M environs/testing/storage.go
   M provider/common/bootstrap_test.go
   M provider/common/state.go
   M provider/common/state_test.go

[Andrew Wilkins (axwalk)]

On 2014/01/27 18:11:55, gz wrote:
> Please take a look.

"On trunk, I don't see why ClientTLS shouldn't be switched
to just use https all the time."

I suppose we could just use HTTPS, but disable host key verification.
The certificate isn't known to clients other than management nodes/CLI.

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/56560043/diff/20001/environs/httpstorage/storage.go
File environs/httpstorage/storage.go (right):

https://codereview.appspot.com/56560043/diff/20001/environs/httpstorage/storage.go#newcode26
environs/httpstorage/storage.go:26: type PromotableStorage interface {
I'm not keen on this. I think a better way to test, rather than further
exposing the guts of this package, would be to use a
net/http/httputil.ReverseProxy.

https://codereview.appspot.com/56560043/diff/20001/provider/common/state.go
File provider/common/state.go (right):

https://codereview.appspot.com/56560043/diff/20001/provider/common/state.go#newcode71
provider/common/state.go:71: func LoadStateFromURL(url string,
disableSSLHostnameVerification bool) (*BootstrapState, error) {
This is fine for now, but I wonder if we shouldn't have a method of
obtaining an http.Client for an environment. We now have HTTP proxy
settings and SSL hostname verification as options. We could centralise
all that and pass an http.Client in here.

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

On 2014/01/29 07:53:19, axw wrote:
> On 2014/01/27 18:11:55, gz wrote:
> > Please take a look.

> "On trunk, I don't see why ClientTLS shouldn't be switched
> to just use https all the time."

> I suppose we could just use HTTPS, but disable host key verification.
The
> certificate isn't known to clients other than management nodes/CLI.

William pointed out to me that actually, yes, we do have the (public) CA
cert. So we could indeed use HTTPS all the time. It would require us to
somehow get the cert into cloud-init so wget can use it when fetching
tools, and to use it when fetching from storage generally.

https://codereview.appspot.com/56560043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/56560043/diff/20001/environs/httpstorage/storage.go
File environs/httpstorage/storage.go (right):

https://codereview.appspot.com/56560043/diff/20001/environs/httpstorage/storage.go#newcode26
environs/httpstorage/storage.go:26: type PromotableStorage interface {
On 2014/01/29 08:12:59, axw wrote:
> I'm not keen on this. I think a better way to test, rather than
further exposing
> the guts of this package, would be to use a
net/http/httputil.ReverseProxy.

Okay, I'll revert most of this and write something independant.

https://codereview.appspot.com/56560043/diff/20001/provider/common/state.go
File provider/common/state.go (right):

https://codereview.appspot.com/56560043/diff/20001/provider/common/state.go#newcode71
provider/common/state.go:71: func LoadStateFromURL(url string,
disableSSLHostnameVerification bool) (*BootstrapState, error) {
On 2014/01/29 08:12:59, axw wrote:
> This is fine for now, but I wonder if we shouldn't have a method of
obtaining an
> http.Client for an environment. We now have HTTP proxy settings and
SSL hostname
> verification as options. We could centralise all that and pass an
http.Client in
> here.

Yes, on trunk there should be some refactoring like that. There are a
couple of places that want a http.Client obeying thig setting and we may
want more.

https://codereview.appspot.com/56560043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

On 2014/01/30 13:57:41, gz wrote:
> Please take a look.

Thanks, much nicer I think. LGTM.

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

Oops, forgot to publish this.

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go
File provider/common/state_test.go (right):

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go#newcode46
provider/common/state_test.go:46: // testingHTTPServer creates a tempdir
backed https server with invalid certs
s/invalid/self-signed/?

https://codereview.appspot.com/56560043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go
File provider/common/state_test.go (right):

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go#newcode46
provider/common/state_test.go:46: // testingHTTPServer creates a tempdir
backed https server with invalid certs
On 2014/01/31 01:57:27, axw wrote:
> s/invalid/self-signed/?

Good call. And really, it's just without certs right?

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

On 2014/01/31 12:33:34, gz wrote:
> Please take a look.

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go
> File provider/common/state_test.go (right):

https://codereview.appspot.com/56560043/diff/60001/provider/common/state_test.go#newcode46
> provider/common/state_test.go:46: // testingHTTPServer creates a
tempdir backed
> https server with invalid certs
> On 2014/01/31 01:57:27, axw wrote:
> > s/invalid/self-signed/?

> Good call. And really, it's just without certs right?

No, there's a certificate (there has to be), it's just self-signed. The
SSL certificate verification fails because the CA is unknown.

https://codereview.appspot.com/56560043/

[Martin Packman (gz)]

On 2014/01/31 12:52:59, axw wrote:

> No, there's a certificate (there has to be), it's just self-signed.
The SSL
> certificate verification fails because the CA is unknown.

Hm, but we're not supplying it, so presumably the httptest server stuff
has its own one?

https://codereview.appspot.com/56560043/

[Andrew Wilkins (axwalk)]

On 2014/02/01 14:54:54, gz wrote:
> On 2014/01/31 12:52:59, axw wrote:
> >
> > No, there's a certificate (there has to be), it's just self-signed.
The SSL
> > certificate verification fails because the CA is unknown.

> Hm, but we're not supplying it, so presumably the httptest server
stuff has its
> own one?

Yes:
http://golang.org/src/pkg/net/http/httptest/server.go?s=3153:3180#L107

https://codereview.appspot.com/56560043/

=== modified file 'cmd/jujud/bootstrap.go'
--- cmd/jujud/bootstrap.go	2013-09-30 19:40:06 +0000
+++ cmd/jujud/bootstrap.go	2014-01-31 12:32:29 +0000
@@ -96,7 +96,7 @@
 		return fmt.Errorf("cannot read provider-state-url file: %v", err)
 	}
 	stateInfoURL := strings.Split(string(data), "\n")[0]
-	bsState, err := common.LoadStateFromURL(stateInfoURL)
+	bsState, err := common.LoadStateFromURL(stateInfoURL, !cfg.SSLHostnameVerification())
 	if err != nil {
 		return fmt.Errorf("cannot load state from URL %q (read from %q): %v", stateInfoURL, providerStateURLFile, err)
 	}

=== modified file 'environs/httpstorage/storage.go'
--- environs/httpstorage/storage.go	2013-10-02 05:46:56 +0000
+++ environs/httpstorage/storage.go	2014-01-31 12:32:29 +0000
@@ -64,12 +64,15 @@
 }
 
 func (s *localStorage) getHTTPSBaseURL() (string, error) {
-	url, _ := s.URL("/") // never fails
+	url, _ := s.URL("") // never fails
 	resp, err := s.client.Head(url)
 	if err != nil {
 		return "", err
 	}
 	resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		return "", fmt.Errorf("Could not access file storage: %v %s", url, resp.Status)
+	}
 	httpsURL, err := resp.Location()
 	if err != nil {
 		return "", err

=== modified file 'provider/common/bootstrap_test.go'
--- provider/common/bootstrap_test.go	2013-10-02 00:29:29 +0000
+++ provider/common/bootstrap_test.go	2014-01-31 12:32:29 +0000
@@ -151,7 +151,7 @@
 	err := common.Bootstrap(env, constraints.Value{}, nil)
 	c.Assert(err, gc.IsNil)
 
-	savedState, err := common.LoadStateFromURL(checkURL)
+	savedState, err := common.LoadStateFromURL(checkURL, false)
 	c.Assert(err, gc.IsNil)
 	c.Assert(savedState, gc.DeepEquals, &common.BootstrapState{
 		StateInstances:  []instance.Id{instance.Id(checkInstanceId)},

=== modified file 'provider/common/state.go'
--- provider/common/state.go	2013-10-01 12:03:33 +0000
+++ provider/common/state.go	2014-01-31 12:32:29 +0000
@@ -21,6 +21,7 @@
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
+	"launchpad.net/juju-core/utils"
 )
 
 // StateFile is the name of the file where the provider's state is stored.
@@ -67,11 +68,19 @@
 }
 
 // LoadStateFromURL reads state from the given URL.
-func LoadStateFromURL(url string) (*BootstrapState, error) {
-	resp, err := http.Get(url)
+func LoadStateFromURL(url string, disableSSLHostnameVerification bool) (*BootstrapState, error) {
+	client := http.DefaultClient
+	if disableSSLHostnameVerification {
+		logger.Infof("hostname SSL verification disabled")
+		client = utils.GetNonValidatingHTTPClient()
+	}
+	resp, err := client.Get(url)
 	if err != nil {
 		return nil, err
 	}
+	if resp.StatusCode != http.StatusOK {
+		return nil, fmt.Errorf("could not load state from url: %v %s", url, resp.Status)
+	}
 	return loadState(resp.Body)
 }
 

=== modified file 'provider/common/state_test.go'
--- provider/common/state_test.go	2013-10-02 10:39:12 +0000
+++ provider/common/state_test.go	2014-01-31 12:32:29 +0000
@@ -4,8 +4,10 @@
 package common_test
 
 import (
-	"bytes"
 	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
 
 	gc "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
@@ -30,10 +32,26 @@
 	AddCleanup(testbase.CleanupFunc)
 }
 
-func newStorage(suite cleaner, c *gc.C) storage.Storage {
-	closer, stor, _ := envtesting.CreateLocalTestStorage(c)
+func newStorageWithDataDir(suite cleaner, c *gc.C) (stor storage.Storage, dataDir string) {
+	closer, stor, dataDir := envtesting.CreateLocalTestStorage(c)
 	suite.AddCleanup(func(*gc.C) { closer.Close() })
-	return stor
+	return
+}
+
+func newStorage(suite cleaner, c *gc.C) (stor storage.Storage) {
+	stor, _ = newStorageWithDataDir(suite, c)
+	return
+}
+
+// testingHTTPServer creates a tempdir backed https server without certs
+func testingHTTPSServer(suite cleaner, c *gc.C) (baseURL string, dataDir string) {
+	dataDir = c.MkDir()
+	mux := http.NewServeMux()
+	mux.Handle("/", http.FileServer(http.Dir(dataDir)))
+	server := httptest.NewTLSServer(mux)
+	suite.AddCleanup(func(*gc.C) { server.Close() })
+	baseURL = server.URL
+	return
 }
 
 func (suite *StateSuite) TestCreateStateFileWritesEmptyStateFile(c *gc.C) {
@@ -72,32 +90,49 @@
 	c.Check(content, gc.DeepEquals, marshaledState)
 }
 
-func (suite *StateSuite) setUpSavedState(c *gc.C, stor storage.Storage) common.BootstrapState {
+func (suite *StateSuite) setUpSavedState(c *gc.C, dataDir string) common.BootstrapState {
 	arch := "amd64"
 	state := common.BootstrapState{
 		StateInstances:  []instance.Id{instance.Id("an-instance-id")},
 		Characteristics: []instance.HardwareCharacteristics{{Arch: &arch}}}
 	content, err := goyaml.Marshal(state)
 	c.Assert(err, gc.IsNil)
-	err = stor.Put(common.StateFile, ioutil.NopCloser(bytes.NewReader(content)), int64(len(content)))
+	err = ioutil.WriteFile(filepath.Join(dataDir, common.StateFile), []byte(content), 0644)
 	c.Assert(err, gc.IsNil)
 	return state
 }
 
 func (suite *StateSuite) TestLoadStateReadsStateFile(c *gc.C) {
-	storage := newStorage(suite, c)
-	state := suite.setUpSavedState(c, storage)
+	storage, dataDir := newStorageWithDataDir(suite, c)
+	state := suite.setUpSavedState(c, dataDir)
 	storedState, err := common.LoadState(storage)
 	c.Assert(err, gc.IsNil)
 	c.Check(*storedState, gc.DeepEquals, state)
 }
 
 func (suite *StateSuite) TestLoadStateFromURLReadsStateFile(c *gc.C) {
-	stor := newStorage(suite, c)
-	state := suite.setUpSavedState(c, stor)
+	stor, dataDir := newStorageWithDataDir(suite, c)
+	state := suite.setUpSavedState(c, dataDir)
 	url, err := stor.URL(common.StateFile)
 	c.Assert(err, gc.IsNil)
-	storedState, err := common.LoadStateFromURL(url)
+	storedState, err := common.LoadStateFromURL(url, false)
+	c.Assert(err, gc.IsNil)
+	c.Check(*storedState, gc.DeepEquals, state)
+}
+
+func (suite *StateSuite) TestLoadStateFromURLBadCert(c *gc.C) {
+	baseURL, _ := testingHTTPSServer(suite, c)
+	url := baseURL + "/" + common.StateFile
+	storedState, err := common.LoadStateFromURL(url, false)
+	c.Assert(err, gc.ErrorMatches, ".*/provider-state:.* certificate signed by unknown authority")
+	c.Assert(storedState, gc.IsNil)
+}
+
+func (suite *StateSuite) TestLoadStateFromURLBadCertPermitted(c *gc.C) {
+	baseURL, dataDir := testingHTTPSServer(suite, c)
+	state := suite.setUpSavedState(c, dataDir)
+	url := baseURL + "/" + common.StateFile
+	storedState, err := common.LoadStateFromURL(url, true)
 	c.Assert(err, gc.IsNil)
 	c.Check(*storedState, gc.DeepEquals, state)
 }