Merge lp:~axwalk/juju-core/lp1235102-httpstorage-head into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/httpstorage: use req.Host in HEAD

The HEAD response was returning the listening IP
address, rather than the host presented in the
original HTTP Request. This is changed to now use
the request's Host field.

Fixes #1235102

https://codereview.appspot.com/14388043/

environs/httpstorage: use req.Host in HEAD

The HEAD response was returning the listening IP
address, rather than the host presented in the
original HTTP Request. This is changed to now use
the request's Host field.

Fixes #1235102

https://codereview.appspot.com/14388043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+189225_code.launchpad.net,

Message:
Please take a look.

Description:
environs/httpstorage: use req.Host in HEAD

The HEAD response was returning the listening IP
address, rather than the host presented in the
original HTTP Request. This is changed to now use
the request's Host field.

Fixes #1235102

https://code.launchpad.net/~axwalk/juju-core/lp1235102-httpstorage-head/+merge/189225

(do not edit description out of merge proposal)

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

Affected files (+19, -6 lines):
   A [revision details]
   M environs/httpstorage/backend.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131003170932-4tkr0tu40wb0nfgl
+New revision: <email address hidden>

Index: environs/httpstorage/backend.go
=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go 2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend.go 2013-10-04 07:38:49 +0000
@@ -22,9 +22,9 @@
  type storageBackend struct {
   backend storage.Storage

- // httpsBaseURL is the base URL to send to clients
+ // httpsPort is the port to send to clients
   // if they perform a HEAD request.
- httpsBaseURL string
+ httpsPort int

   // authkey is non-empty if modifying requests
   // require an auth key.
@@ -37,7 +37,7 @@
   case "PUT", "DELETE":
    // Don't allow modifying operations if there's an HTTPS backend
    // to handle that, and ensure the user is authorised/authenticated.
- if s.httpsBaseURL != "" || !s.authorised(req) {
+ if s.httpsPort != 0 || !s.authorised(req) {
     http.Error(w, "unauthorised access", http.StatusUnauthorized)
     return
    }
@@ -72,8 +72,19 @@
  // handleHead returns the HTTPS URL for the specified
  // path in the Location header.
  func (s *storageBackend) handleHead(w http.ResponseWriter, req
*http.Request) {
- if s.httpsBaseURL != "" {
- w.Header().Set("Location", s.httpsBaseURL+req.URL.Path)
+ if s.httpsPort != 0 {
+ hostonly := req.Host
+ if i := strings.LastIndex(hostonly, "]"); i != -1 {
+ // [ipv6]:port
+ hostonly = hostonly[:i+1]
+ } else {
+ // host:port
+ if i := strings.LastIndex(hostonly, ":"); i != -1 {
+ hostonly = hostonly[:i]
+ }
+ }
+ url := fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort,
req.URL.Path)
+ w.Header().Set("Location", url)
   } else {
    http.Error(w, "method HEAD is not supported",
http.StatusMethodNotAllowed)
    return
@@ -192,7 +203,7 @@
     listener.Close()
     return nil, fmt.Errorf("cannot start TLS listener: %v", err)
    }
- backend.httpsBaseURL = fmt.Sprintf("https://%s", tlsListener.Addr())
+ backend.httpsPort = tlsListener.Addr().(*net.TCPAddr).Port
    goServe(tlsListener, tlsBackend)
   }
   goServe(listener, backend)

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14388043/

[William Reade (fwereade)]

LGTM, thanks

https://codereview.appspot.com/14388043/

[Roger Peppe (rogpeppe)]

LGTM with some minor grumblings.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go
File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode65
environs/httpstorage/backend.go:65: func (s *storageBackend)
authorised(req *http.Request) bool {
s/authorised/authorized/

We standardise on US spelling in this code base,
to match Go's conventions.
(I just spent a while grepping for "authorized")

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode76
environs/httpstorage/backend.go:76: hostonly := req.Host
Can't you use net.SplitHostPort here?

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode86
environs/httpstorage/backend.go:86: url :=
fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort, req.URL.Path)
tbh this feel like abuse of the HEAD method.
It should at least check that req.URL.Path is /.
But if it's conventional, I *guess* it's ok.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go
File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go#newcode152
environs/httpstorage/backend_test.go:152: c.Assert(location.String(),
gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
We should continue with the test and check that we can
actually connect with the given URL.

https://codereview.appspot.com/14388043/

[Andrew Wilkins (axwalk)]

On 2013/10/04 12:08:36, rog wrote:
> LGTM with some minor grumblings.

Thanks, I'll address these in another CL since I've already landed this.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go
> File environs/httpstorage/backend.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode65
> environs/httpstorage/backend.go:65: func (s *storageBackend)
authorised(req
> *http.Request) bool {
> s/authorised/authorized/

> We standardise on US spelling in this code base,
> to match Go's conventions.
> (I just spent a while grepping for "authorized")

Thanks, will update.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode76
> environs/httpstorage/backend.go:76: hostonly := req.Host
> Can't you use net.SplitHostPort here?

Yes. I thought there was a function, just didn't find it in my
admittedly short search.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend.go#newcode86
> environs/httpstorage/backend.go:86: url :=
fmt.Sprintf("https://%s:%d%s",
> hostonly, s.httpsPort, req.URL.Path)
> tbh this feel like abuse of the HEAD method.

Indeed :)
I considered doing redirects, but not all client methods follow them,
and it'd cause two requests for each Put/Remove.

> It should at least check that req.URL.Path is /.
> But if it's conventional, I *guess* it's ok.

So, the only way Path might be empty is if the request line had URI with
no path. Right?
Never going to happen for this, so I'd rather not complicate it.

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go
> File environs/httpstorage/backend_test.go (right):

https://codereview.appspot.com/14388043/diff/4001/environs/httpstorage/backend_test.go#newcode152
> environs/httpstorage/backend_test.go:152: c.Assert(location.String(),
> gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
> We should continue with the test and check that we can
> actually connect with the given URL.

Okay.

https://codereview.appspot.com/14388043/

=== modified file 'environs/httpstorage/backend.go'
--- environs/httpstorage/backend.go	2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend.go	2013-10-04 08:07:30 +0000
@@ -22,9 +22,9 @@
 type storageBackend struct {
 	backend storage.Storage
 
-	// httpsBaseURL is the base URL to send to clients
+	// httpsPort is the port to send to clients
 	// if they perform a HEAD request.
-	httpsBaseURL string
+	httpsPort int
 
 	// authkey is non-empty if modifying requests
 	// require an auth key.
@@ -37,7 +37,7 @@
 	case "PUT", "DELETE":
 		// Don't allow modifying operations if there's an HTTPS backend
 		// to handle that, and ensure the user is authorised/authenticated.
-		if s.httpsBaseURL != "" || !s.authorised(req) {
+		if s.httpsPort != 0 || !s.authorised(req) {
 			http.Error(w, "unauthorised access", http.StatusUnauthorized)
 			return
 		}
@@ -72,8 +72,19 @@
 // handleHead returns the HTTPS URL for the specified
 // path in the Location header.
 func (s *storageBackend) handleHead(w http.ResponseWriter, req *http.Request) {
-	if s.httpsBaseURL != "" {
-		w.Header().Set("Location", s.httpsBaseURL+req.URL.Path)
+	if s.httpsPort != 0 {
+		hostonly := req.Host
+		if i := strings.LastIndex(hostonly, "]"); i != -1 {
+			// [ipv6]:port
+			hostonly = hostonly[:i+1]
+		} else {
+			// host:port
+			if i := strings.LastIndex(hostonly, ":"); i != -1 {
+				hostonly = hostonly[:i]
+			}
+		}
+		url := fmt.Sprintf("https://%s:%d%s", hostonly, s.httpsPort, req.URL.Path)
+		w.Header().Set("Location", url)
 	} else {
 		http.Error(w, "method HEAD is not supported", http.StatusMethodNotAllowed)
 		return
@@ -192,7 +203,7 @@
 			listener.Close()
 			return nil, fmt.Errorf("cannot start TLS listener: %v", err)
 		}
-		backend.httpsBaseURL = fmt.Sprintf("https://%s", tlsListener.Addr())
+		backend.httpsPort = tlsListener.Addr().(*net.TCPAddr).Port
 		goServe(tlsListener, tlsBackend)
 	}
 	goServe(listener, backend)

=== modified file 'environs/httpstorage/backend_test.go'
--- environs/httpstorage/backend_test.go	2013-10-02 05:48:00 +0000
+++ environs/httpstorage/backend_test.go	2013-10-04 08:07:30 +0000
@@ -61,7 +61,7 @@
 	caKeyPEM := []byte(coretesting.CAKey)
 	listener, err = httpstorage.ServeTLS("127.0.0.1:0", embedded, caCertPEM, caKeyPEM, hostnames, testAuthkey)
 	c.Assert(err, gc.IsNil)
-	return listener, fmt.Sprintf("http://%s/", listener.Addr()), dataDir
+	return listener, fmt.Sprintf("http://localhost:%d/", listener.Addr().(*net.TCPAddr).Port), dataDir
 }
 
 type testCase struct {
@@ -149,7 +149,7 @@
 	c.Assert(resp.StatusCode, gc.Equals, http.StatusOK)
 	location, err := resp.Location()
 	c.Assert(err, gc.IsNil)
-	c.Assert(location.String(), gc.Matches, "https://127.0.0.1:[0-9]{5}/arbitrary")
+	c.Assert(location.String(), gc.Matches, "https://localhost:[0-9]{5}/arbitrary")
 }
 
 func (s *backendSuite) TestGet(c *gc.C) {