Commit 2434072e authored by Kirill Smelkov's avatar Kirill Smelkov Committed by Alain Takoudjou

fixup! NXD blob/auth: Teach it to handle HTTP Basic Auth too

@rafael approached me and asked why URLs like

	https://gitlab-ci-token:XXX@hostname/group/project/raw/master/file

work in CURL, but not in Chrome under AJAX requests.

After investigation it turned out they neither work in WGET and give 302
redirect to http://localhost:8080/users/sign_in:

	kirr@deco:~$ wget https://gitlab-ci-token:XXX@lab.nexedi.com/kirr/test/raw/master/hello.txt
	--2018-06-04 13:14:04--  https://gitlab-ci-token:*password*@lab.nexedi.com/kirr/test/raw/master/hello.txt
	Resolving lab.nexedi.com (lab.nexedi.com)... 176.31.129.213, 85.118.38.162
	Connecting to lab.nexedi.com (lab.nexedi.com)|176.31.129.213|:443... connected.
	HTTP request sent, awaiting response... 302 Found
	Location: http://localhost:8080/users/sign_in [following]
	--2018-06-04 13:14:04--  http://localhost:8080/users/sign_in
	Resolving localhost (localhost)... 127.0.0.1, ::1
	Connecting to localhost (localhost)|127.0.0.1|:8080... failed: Connection refused.
	Connecting to localhost (localhost)|::1|:8080... failed: Connection refused.

This turned out to be due to most clients (in fine accordance with RFC2617 /
RFC7617) first send request without Authorization header set and retry it with
that header only if server challenges it to(*), and our authorization code was
only trying to handle HTTP basic auth if Authorization header was provided
without issuing any challenge on server side.

Fix it by checking Rails backend reply for 302, which it gives for
unauthorized non-raw requests, and on our side convert it HTTP Basic
auth challenge if raw request does not contain any token. This way it
now works with user:password in URLs for both WGET and Chrome.

If any tokens were provided we leave Rails auth response as is because
we handle user/password only for that "no token provided at all" case.

(*) see https://en.wikipedia.org/wiki/Basic_access_authentication for overview.
/cc @alain.takoudjou, @jerome

/reviewed-on !2
parent 2f1b3ddd
...@@ -276,8 +276,8 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str ...@@ -276,8 +276,8 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str
// Request to auth backend to verify whether download is possible. // Request to auth backend to verify whether download is possible.
// - first option is via asking as `git fetch` would do, but on Rails // - first option is via asking as `git fetch` would do, but on Rails
// side this supports only basic auth, not private token. // side this supports only basic auth, not private token.
// - that's why we auth backend to authenticate as if it was request to // - that's why we first ask auth backend to authenticate as if it was
// get repo archive and propagate request query and header. // request to get repo archive and propagate request query and header.
url := project + "/repository/archive.zip" url := project + "/repository/archive.zip"
if query != "" { if query != "" {
url += "?" + query url += "?" + query
...@@ -308,13 +308,29 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str ...@@ -308,13 +308,29 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str
) )
authProxy.ServeHTTP(authReply.RawReply, reqDownloadAccess) authProxy.ServeHTTP(authReply.RawReply, reqDownloadAccess)
// If not successful and userinfo is provided without query, retry // If not successful and any token was not provided neither in
// authenticating as `git fetch` would do. // query nor in header, retry authenticating as `git fetch` would do.
// //
// The reason we want to do this second try is that HTTP auth is // The reason we want to do this second try is that HTTP auth is
// handled by upstream auth backend for git requests only, and we might // handled by upstream auth backend for git requests only, and we might
// want to use e.g. https://gitlab-ci-token:token@/.../raw/... // want to use e.g. https://gitlab-ci-token:token@/.../raw/...
if authReply.RepoPath != "" || user == nil || query != "" { if authReply.RepoPath != "" || query != "" || len(header) != 0 {
return authReply
}
if user == nil {
// backend gives "302 location: .../users/sign_in" when rejecting access.
// transform this to HTTP Basic Auth challenge if we don't have user/password already.
//
// reason: many clients - e.g. wget, chrome, even when given user/password,
// first send request without auth set and expect server to send them auth challenge.
// and only after the challenge they retry the request with Authorization header set.
if authReply.RawReply.Code == http.StatusFound &&
strings.HasSuffix(authReply.RawReply.HeaderMap.Get("location"), "/users/sign_in") {
r := httptest.NewRecorder()
r.Header().Set("WWW-Authenticate", "Basic realm=\"\"")
r.WriteHeader(http.StatusUnauthorized)
authReply.RawReply = r
}
return authReply return authReply
} }
......
...@@ -782,7 +782,9 @@ func download(t *testing.T, url, username, password string, h http.Header) (*htt ...@@ -782,7 +782,9 @@ func download(t *testing.T, url, username, password string, h http.Header) (*htt
for k, v := range h { for k, v := range h {
req.Header[k] = v req.Header[k] = v
} }
client := &http.Client{} client := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error {
return http.ErrUseLastResponse // don't follow redirects
}}
resp, err := client.Do(req) resp, err := client.Do(req)
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
...@@ -838,6 +840,14 @@ func (dl DownloadContext) ExpectCode(path string, code int) { ...@@ -838,6 +840,14 @@ func (dl DownloadContext) ExpectCode(path string, code int) {
} }
} }
// download `path` and expect HTTP reply header to be set to `value`.
func (dl DownloadContext) ExpectHeader(path, header, value string) {
resp, _ := dl.download(path)
if h := resp.Header.Get(header); h != value {
dl.t.Fatalf("Header %q: expected %q; got %q", header, value, h)
}
}
func TestBlobDownload(t *testing.T) { func TestBlobDownload(t *testing.T) {
// Prepare test server and "all-ok" auth backend // Prepare test server and "all-ok" auth backend
ts := archiveOKServer(t, "") ts := archiveOKServer(t, "")
...@@ -873,16 +883,26 @@ func TestPrivateBlobDownload(t *testing.T) { ...@@ -873,16 +883,26 @@ func TestPrivateBlobDownload(t *testing.T) {
ts := testhelper.TestServerWithHandler(nil, func(w http.ResponseWriter, r *http.Request) { ts := testhelper.TestServerWithHandler(nil, func(w http.ResponseWriter, r *http.Request) {
log.Println("UPSTREAM", r.Method, r.URL) log.Println("UPSTREAM", r.Method, r.URL)
gitfetch := (strings.HasSuffix(r.URL.Path, "/info/refs") && r.URL.RawQuery == "service=git-upload-pack") gitfetch := (strings.HasSuffix(r.URL.Path, "/info/refs") && r.URL.RawQuery == "service=git-upload-pack")
token_ok1 := r.URL.Query().Get("aaa_token") == "TOKEN-4AAA" token1 := r.URL.Query().Get("aaa_token")
token_ok2 := r.Header.Get("BBB-TOKEN") == "TOKEN-4BBB" token2 := r.Header.Get("BBB-TOKEN")
cookie, _ := r.Cookie("_gitlab_session") cookie := ""
cookie_ok3 := (cookie != nil && cookie.Value == "COOKIE-CCC") if c, err := r.Cookie("_gitlab_session"); err == nil {
username, password, user_ok4 := r.BasicAuth() cookie = c.Value
if user_ok4 { }
username, password, user_ok := r.BasicAuth()
if user_ok {
// user:password only accepted for `git fetch` requests // user:password only accepted for `git fetch` requests
user_ok4 = (gitfetch && username == "user-ddd" && password == "password-eee") user_ok = (gitfetch && username == "user-ddd" && password == "password-eee")
} }
if !(token_ok1 || token_ok2 || cookie_ok3 || user_ok4) {
// simulate rails which gives "302 location: .../users/sign_in" when no access by token
if !gitfetch && (token1 == "" && token2 == "" && cookie == "") {
w.Header().Set("location", ".../users/sign_in")
w.WriteHeader(302)
return
}
if !(token1 == "TOKEN-4AAA" || token2 == "TOKEN-4BBB" || cookie == "COOKIE-CCC" || user_ok) {
w.WriteHeader(403) w.WriteHeader(403)
fmt.Fprintf(w, "Access denied") fmt.Fprintf(w, "Access denied")
return return
...@@ -915,35 +935,41 @@ func TestPrivateBlobDownload(t *testing.T) { ...@@ -915,35 +935,41 @@ func TestPrivateBlobDownload(t *testing.T) {
defer ws.Close() defer ws.Close()
dl := NewDownloadContext(t, fmt.Sprintf("%s/%s/raw", ws.URL, testProject)) dl := NewDownloadContext(t, fmt.Sprintf("%s/%s/raw", ws.URL, testProject))
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 401)
dl.ExpectCode("/5f923865/README.md?bbb_token=TOKEN-4BBB", 403) dl.ExpectCode("/5f923865/README.md?bbb_token=TOKEN-4BBB", 302)
dl.ExpectCode("/5f923865/README.md?aaa_token=TOKEN-XXXX", 403)
dl.ExpectCode("/5f923865/README.md?aaa_token=TOKEN-4AAA", 200) dl.ExpectCode("/5f923865/README.md?aaa_token=TOKEN-4AAA", 200)
dl.ExpectSha1("/5f923865/README.md?aaa_token=TOKEN-4AAA", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c") dl.ExpectSha1("/5f923865/README.md?aaa_token=TOKEN-4AAA", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
dl.Header.Add("AAA-TOKEN", "TOKEN-4AAA") dl.Header.Set("AAA-TOKEN", "TOKEN-4AAA")
dl.ExpectCode("/5f923865/README.md", 302)
dl.Header.Set("BBB-TOKEN", "TOKEN-XXX")
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 403)
dl.Header.Add("BBB-TOKEN", "TOKEN-4BBB") dl.Header.Set("BBB-TOKEN", "TOKEN-4BBB")
dl.ExpectCode("/5f923865/README.md", 200) dl.ExpectCode("/5f923865/README.md", 200)
dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c") dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
dl.Header = make(http.Header) // clear dl.Header = make(http.Header) // clear
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 401)
dl.Header.Set("Cookie", "alpha=1") dl.Header.Set("Cookie", "alpha=1")
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 401)
dl.Header.Set("Cookie", "alpha=1; beta=2") dl.Header.Set("Cookie", "alpha=1; beta=2")
dl.ExpectCode("/5f923865/README.md", 401)
dl.Header.Set("Cookie", "alpha=1; _gitlab_session=COOKIE-XXX; beta=2")
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 403)
dl.Header.Set("Cookie", "alpha=1; _gitlab_session=COOKIE-CCC; beta=2") dl.Header.Set("Cookie", "alpha=1; _gitlab_session=COOKIE-CCC; beta=2")
dl.ExpectCode("/5f923865/README.md", 200) dl.ExpectCode("/5f923865/README.md", 200)
dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c") dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
dl.Header = make(http.Header) // clear dl.Header = make(http.Header) // clear
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 401)
dl.ExpectHeader("/5f923865/README.md", "www-authenticate", "Basic realm=\"\"")
dl.username = "user-aaa" dl.username = "user-aaa"
dl.password = "password-bbb" dl.password = "password-bbb"
dl.ExpectCode("/5f923865/README.md", 403) dl.ExpectCode("/5f923865/README.md", 403)
dl.username = "user-ddd" dl.username = "user-ddd"
dl.password = "password-eee" dl.password = "password-eee"
dl.ExpectCode("/5f923865/README.md?qqq_token=1", 403) dl.ExpectCode("/5f923865/README.md?qqq_token=1", 302)
dl.ExpectCode("/5f923865/README.md", 200) dl.ExpectCode("/5f923865/README.md", 200)
dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c") dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
} }
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment