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

Proposed by Jeroen T. Vermeulen on 2013-08-05
Status: Merged
Approved by: Jeroen T. Vermeulen on 2013-08-05
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) 2013-08-05 Approve on 2013-08-05
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.
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
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
1=== modified file 'HACKING.txt'
2--- HACKING.txt 2013-08-01 07:50:26 +0000
3+++ HACKING.txt 2013-08-05 11:00:41 +0000
4@@ -100,6 +100,11 @@
5
6 https://<accountname>.blob.core.windows.net/<containername>/<blobname>
7
8+The http version of the same URL would work too, but is prone to spurious
9+authentication failures when accessed through a proxy. Therefore gwacl accesses
10+the storage API through https; this may become configurable later if there is
11+demand.
12+
13
14 RESTful API access
15 ------------------
16
17=== modified file 'storage_base.go'
18--- storage_base.go 2013-07-31 13:30:08 +0000
19+++ storage_base.go 2013-08-05 11:00:41 +0000
20@@ -322,7 +322,11 @@
21 // (The result ends in a slash.)
22 func (context *StorageContext) getAccountURL() string {
23 escapedAccount := url.QueryEscape(context.Account)
24- return fmt.Sprintf("http://%s.blob.core.windows.net/", escapedAccount)
25+ // Use https. This does not suffer from the "no renegotiation" bug in
26+ // Go's implementation. It's optional, but it gets around spurious
27+ // authentication failures when working through a proxy that messes with
28+ // our requests instead of passing them on verbatim.
29+ return fmt.Sprintf("https://%s.blob.core.windows.net/", escapedAccount)
30 }
31
32 // getContainerURL returns the URL for a given storage container.
33
34=== modified file 'storage_base_test.go'
35--- storage_base_test.go 2013-08-01 07:00:09 +0000
36+++ storage_base_test.go 2013-08-05 11:00:41 +0000
37@@ -292,7 +292,7 @@
38 c.Check(
39 context.getAccountURL(),
40 Equals,
41- "http://"+url.QueryEscape(account)+".blob.core.windows.net/")
42+ "https://"+url.QueryEscape(account)+".blob.core.windows.net/")
43 }
44
45 func (suite *TestStorageContext) TestGetContainerURL(c *C) {
46@@ -302,7 +302,7 @@
47 c.Check(
48 context.getContainerURL(container),
49 Equals,
50- "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
51+ "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
52 }
53
54 func (suite *TestStorageContext) TestGetFileURL(c *C) {
55@@ -313,7 +313,7 @@
56 c.Check(
57 context.GetFileURL(container, file),
58 Equals,
59- "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
60+ "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
61 }
62
63 func (suite *TestStorageContext) TestGetSignedFileURL(c *C) {
64@@ -361,7 +361,7 @@
65 func (suite *TestListContainers) Test(c *C) {
66 responseBody := `
67 <?xml version="1.0" encoding="utf-8"?>
68- <EnumerationResults AccountName="http://myaccount.blob.core.windows.net">
69+ <EnumerationResults AccountName="https://myaccount.blob.core.windows.net">
70 <Prefix>prefix-value</Prefix>
71 <Marker>marker-value</Marker>
72 <MaxResults>max-results-value</MaxResults>
73@@ -390,7 +390,7 @@
74 results, err := context.ListContainers(request)
75 c.Assert(err, IsNil)
76 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
77- "http://%s.blob.core.windows.net/?comp=list", context.Account))
78+ "https://%s.blob.core.windows.net/?comp=list", context.Account))
79 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
80 c.Assert(results, NotNil)
81 c.Assert(results.Containers[0].Name, Equals, "name-value")
82@@ -648,7 +648,7 @@
83 err := context.CreateContainer(containerName)
84 c.Assert(err, IsNil)
85 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
86- "http://%s.blob.core.windows.net/%s?restype=container",
87+ "https://%s.blob.core.windows.net/%s?restype=container",
88 context.Account, containerName))
89 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
90 }
91@@ -702,7 +702,7 @@
92 err := context.DeleteContainer(containerName)
93 c.Assert(err, IsNil)
94 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
95- "http://%s.blob.core.windows.net/%s?restype=container",
96+ "https://%s.blob.core.windows.net/%s?restype=container",
97 context.Account, containerName))
98 c.Check(transport.Request.Method, Equals, "DELETE")
99 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
100@@ -768,7 +768,7 @@
101 props, err := context.GetContainerProperties(containerName)
102 c.Assert(err, IsNil)
103 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
104- "http://%s.blob.core.windows.net/%s?restype=container",
105+ "https://%s.blob.core.windows.net/%s?restype=container",
106 context.Account, containerName))
107 c.Check(transport.Request.Method, Equals, "GET")
108 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
109@@ -1085,7 +1085,7 @@
110
111 c.Check(transport.Request.Method, Equals, "PUT")
112 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
113- "http://%s.blob.core.windows.net/container/blobname?comp=blocklist",
114+ "https://%s.blob.core.windows.net/container/blobname?comp=blocklist",
115 context.Account))
116 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
117
118@@ -1325,7 +1325,7 @@
119 c.Check(transport.Request.Method, Equals, "PUT")
120 c.Check(transport.Request.URL.String(), Matches,
121 fmt.Sprintf(
122- "http://%s.blob.core.windows.net/mycontainer?.*", context.Account))
123+ "https://%s.blob.core.windows.net/mycontainer?.*", context.Account))
124 c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
125 "comp": {"acl"},
126 "restype": {"container"},
127
128=== modified file 'storage_test.go'
129--- storage_test.go 2013-08-05 09:29:40 +0000
130+++ storage_test.go 2013-08-05 11:00:41 +0000
131@@ -87,7 +87,7 @@
132 func assertBlockListSent(
133 c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange) {
134 c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(
135- "http://%s.blob.core.windows.net/MyContainer/MyFile"+
136+ "https://%s.blob.core.windows.net/MyContainer/MyFile"+
137 "?comp=blocklist", context.Account))
138 body, err := ioutil.ReadAll(exchange.Request.Body)
139 c.Check(err, IsNil)
140@@ -275,7 +275,7 @@
141 results, err := context.ListAllContainers()
142 c.Assert(err, IsNil)
143 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
144- "http://%s.blob.core.windows.net/?comp=list", context.Account))
145+ "https://%s.blob.core.windows.net/?comp=list", context.Account))
146 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
147 c.Assert(results, NotNil)
148 c.Assert(results.Containers[0].Name, Equals, "name-value")

Subscribers

People subscribed via source and target branches