Merge lp:~jtv/gwacl/storage-ssl into lp:gwacl

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 211
Merged at revision: 211
Proposed branch: lp:~jtv/gwacl/storage-ssl
Merge into: lp:gwacl
Diff against target: 148 lines (+22/-13)
4 files modified
HACKING.txt (+5/-0)
storage_base.go (+5/-1)
storage_base_test.go (+10/-10)
storage_test.go (+2/-2)
To merge this branch: bzr merge lp:~jtv/gwacl/storage-ssl
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Review via email: mp+178513@code.launchpad.net

Commit message

Access storage API through https, instead of http.

Description of the change

I was getting spurious authentication failures, but only on GetContainerProperties, and most of the time. My best guess as to why this was is a transparent http proxy between my internet connection and Azure. A proxy could write its own Date header; it might change capitalization on header names; it might alter content types; and it might chunk large requests. Any of those could break Azure's custom authentication scheme for the storage protocol. (I don't think it was chunking though; that's known to trigger an authentication bug but the errors I was getting happened on body-less requests.)

There was another way to fix just the hazard for the Data header: use the x-ms-date header instead. But the signing logic is fairly intricate and exceedingly hard to debug. When my first attempts here failed, I tried https instead.

We didn't expect https to be this easy. It was very difficult on the management-API side, what with Go's standard library not supporting renegotiation and Azure requiring it. And then there was the matter of certificates. But on the storage side, just changing the scheme in the base URL from http to https did the trick. No client-side certificates, no overrides, no renegotiation, and crucially: no more spurious authentication bug!

Jeroen

To post a comment you must log in.
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Long term we need to make this configurable but this is good for now! Please do file a bug about this.

One thing before you land - please write in the comments and perhaps README/HACKING that https is default for now because of your specific problem.

Cheers.

review: Approve
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Funnily enough, the example URL in HACKING.txt already used https! I added a paragraph about this perhaps becoming configurable in the future, and filed bug 1208390 but to be honest I have no idea what point there would be. The storage API does not require any custom certificates, or patched standard library, or anything like that. Just four-to-the-bar https.

The comments in this branch already described exactly what you say, so hopefully that means we're done!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'HACKING.txt'
--- HACKING.txt 2013-08-01 07:50:26 +0000
+++ HACKING.txt 2013-08-05 11:00:41 +0000
@@ -100,6 +100,11 @@
100100
101 https://<accountname>.blob.core.windows.net/<containername>/<blobname>101 https://<accountname>.blob.core.windows.net/<containername>/<blobname>
102102
103The http version of the same URL would work too, but is prone to spurious
104authentication failures when accessed through a proxy. Therefore gwacl accesses
105the storage API through https; this may become configurable later if there is
106demand.
107
103108
104RESTful API access109RESTful API access
105------------------110------------------
106111
=== modified file 'storage_base.go'
--- storage_base.go 2013-07-31 13:30:08 +0000
+++ storage_base.go 2013-08-05 11:00:41 +0000
@@ -322,7 +322,11 @@
322// (The result ends in a slash.)322// (The result ends in a slash.)
323func (context *StorageContext) getAccountURL() string {323func (context *StorageContext) getAccountURL() string {
324 escapedAccount := url.QueryEscape(context.Account)324 escapedAccount := url.QueryEscape(context.Account)
325 return fmt.Sprintf("http://%s.blob.core.windows.net/", escapedAccount)325 // Use https. This does not suffer from the "no renegotiation" bug in
326 // Go's implementation. It's optional, but it gets around spurious
327 // authentication failures when working through a proxy that messes with
328 // our requests instead of passing them on verbatim.
329 return fmt.Sprintf("https://%s.blob.core.windows.net/", escapedAccount)
326}330}
327331
328// getContainerURL returns the URL for a given storage container.332// getContainerURL returns the URL for a given storage container.
329333
=== modified file 'storage_base_test.go'
--- storage_base_test.go 2013-08-01 07:00:09 +0000
+++ storage_base_test.go 2013-08-05 11:00:41 +0000
@@ -292,7 +292,7 @@
292 c.Check(292 c.Check(
293 context.getAccountURL(),293 context.getAccountURL(),
294 Equals,294 Equals,
295 "http://"+url.QueryEscape(account)+".blob.core.windows.net/")295 "https://"+url.QueryEscape(account)+".blob.core.windows.net/")
296}296}
297297
298func (suite *TestStorageContext) TestGetContainerURL(c *C) {298func (suite *TestStorageContext) TestGetContainerURL(c *C) {
@@ -302,7 +302,7 @@
302 c.Check(302 c.Check(
303 context.getContainerURL(container),303 context.getContainerURL(container),
304 Equals,304 Equals,
305 "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))305 "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
306}306}
307307
308func (suite *TestStorageContext) TestGetFileURL(c *C) {308func (suite *TestStorageContext) TestGetFileURL(c *C) {
@@ -313,7 +313,7 @@
313 c.Check(313 c.Check(
314 context.GetFileURL(container, file),314 context.GetFileURL(container, file),
315 Equals,315 Equals,
316 "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))316 "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
317}317}
318318
319func (suite *TestStorageContext) TestGetSignedFileURL(c *C) {319func (suite *TestStorageContext) TestGetSignedFileURL(c *C) {
@@ -361,7 +361,7 @@
361func (suite *TestListContainers) Test(c *C) {361func (suite *TestListContainers) Test(c *C) {
362 responseBody := `362 responseBody := `
363 <?xml version="1.0" encoding="utf-8"?>363 <?xml version="1.0" encoding="utf-8"?>
364 <EnumerationResults AccountName="http://myaccount.blob.core.windows.net">364 <EnumerationResults AccountName="https://myaccount.blob.core.windows.net">
365 <Prefix>prefix-value</Prefix>365 <Prefix>prefix-value</Prefix>
366 <Marker>marker-value</Marker>366 <Marker>marker-value</Marker>
367 <MaxResults>max-results-value</MaxResults>367 <MaxResults>max-results-value</MaxResults>
@@ -390,7 +390,7 @@
390 results, err := context.ListContainers(request)390 results, err := context.ListContainers(request)
391 c.Assert(err, IsNil)391 c.Assert(err, IsNil)
392 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(392 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
393 "http://%s.blob.core.windows.net/?comp=list", context.Account))393 "https://%s.blob.core.windows.net/?comp=list", context.Account))
394 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")394 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
395 c.Assert(results, NotNil)395 c.Assert(results, NotNil)
396 c.Assert(results.Containers[0].Name, Equals, "name-value")396 c.Assert(results.Containers[0].Name, Equals, "name-value")
@@ -648,7 +648,7 @@
648 err := context.CreateContainer(containerName)648 err := context.CreateContainer(containerName)
649 c.Assert(err, IsNil)649 c.Assert(err, IsNil)
650 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(650 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
651 "http://%s.blob.core.windows.net/%s?restype=container",651 "https://%s.blob.core.windows.net/%s?restype=container",
652 context.Account, containerName))652 context.Account, containerName))
653 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")653 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
654}654}
@@ -702,7 +702,7 @@
702 err := context.DeleteContainer(containerName)702 err := context.DeleteContainer(containerName)
703 c.Assert(err, IsNil)703 c.Assert(err, IsNil)
704 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(704 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
705 "http://%s.blob.core.windows.net/%s?restype=container",705 "https://%s.blob.core.windows.net/%s?restype=container",
706 context.Account, containerName))706 context.Account, containerName))
707 c.Check(transport.Request.Method, Equals, "DELETE")707 c.Check(transport.Request.Method, Equals, "DELETE")
708 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")708 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
@@ -768,7 +768,7 @@
768 props, err := context.GetContainerProperties(containerName)768 props, err := context.GetContainerProperties(containerName)
769 c.Assert(err, IsNil)769 c.Assert(err, IsNil)
770 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(770 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
771 "http://%s.blob.core.windows.net/%s?restype=container",771 "https://%s.blob.core.windows.net/%s?restype=container",
772 context.Account, containerName))772 context.Account, containerName))
773 c.Check(transport.Request.Method, Equals, "GET")773 c.Check(transport.Request.Method, Equals, "GET")
774 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")774 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
@@ -1085,7 +1085,7 @@
10851085
1086 c.Check(transport.Request.Method, Equals, "PUT")1086 c.Check(transport.Request.Method, Equals, "PUT")
1087 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(1087 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
1088 "http://%s.blob.core.windows.net/container/blobname?comp=blocklist",1088 "https://%s.blob.core.windows.net/container/blobname?comp=blocklist",
1089 context.Account))1089 context.Account))
1090 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")1090 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
10911091
@@ -1325,7 +1325,7 @@
1325 c.Check(transport.Request.Method, Equals, "PUT")1325 c.Check(transport.Request.Method, Equals, "PUT")
1326 c.Check(transport.Request.URL.String(), Matches,1326 c.Check(transport.Request.URL.String(), Matches,
1327 fmt.Sprintf(1327 fmt.Sprintf(
1328 "http://%s.blob.core.windows.net/mycontainer?.*", context.Account))1328 "https://%s.blob.core.windows.net/mycontainer?.*", context.Account))
1329 c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{1329 c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
1330 "comp": {"acl"},1330 "comp": {"acl"},
1331 "restype": {"container"},1331 "restype": {"container"},
13321332
=== modified file 'storage_test.go'
--- storage_test.go 2013-08-05 09:29:40 +0000
+++ storage_test.go 2013-08-05 11:00:41 +0000
@@ -87,7 +87,7 @@
87func assertBlockListSent(87func assertBlockListSent(
88 c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange) {88 c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange) {
89 c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(89 c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(
90 "http://%s.blob.core.windows.net/MyContainer/MyFile"+90 "https://%s.blob.core.windows.net/MyContainer/MyFile"+
91 "?comp=blocklist", context.Account))91 "?comp=blocklist", context.Account))
92 body, err := ioutil.ReadAll(exchange.Request.Body)92 body, err := ioutil.ReadAll(exchange.Request.Body)
93 c.Check(err, IsNil)93 c.Check(err, IsNil)
@@ -275,7 +275,7 @@
275 results, err := context.ListAllContainers()275 results, err := context.ListAllContainers()
276 c.Assert(err, IsNil)276 c.Assert(err, IsNil)
277 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(277 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
278 "http://%s.blob.core.windows.net/?comp=list", context.Account))278 "https://%s.blob.core.windows.net/?comp=list", context.Account))
279 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")279 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
280 c.Assert(results, NotNil)280 c.Assert(results, NotNil)
281 c.Assert(results.Containers[0].Name, Equals, "name-value")281 c.Assert(results.Containers[0].Name, Equals, "name-value")

Subscribers

People subscribed via source and target branches