Sign LFS upload requests that have been handled by workhorse

This is a necessary change to solve a security issue. For more details,
see this issue: https://gitlab.com/gitlab-org/gitlab-workhorse/issues/197

authorization_test.go

@@ -95,7 +95,7 @@ func TestPreAuthorizeRedirect(t *testing.T) {
 
 func TestPreAuthorizeJWT(t *testing.T) {
 	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		token, err := jwt.Parse(r.Header.Get(api.RequestHeader), func(token *jwt.Token) (interface{}, error) {
+		token, err := jwt.Parse(r.Header.Get(secret.RequestHeader), func(token *jwt.Token) (interface{}, error) {
 			// Don't forget to validate the alg is what you expect:
 			if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
 				return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"])

internal/api/api.go

@@ -23,9 +23,6 @@ const (
 	// Custom content type for API responses, to catch routing / programming mistakes
 	ResponseContentType = "application/vnd.gitlab-workhorse+json"
 
-	// This header carries the JWT token for gitlab-rails
-	RequestHeader = "Gitlab-Workhorse-Api-Request"
-
 	failureResponseLimit = 32768
 )
 
@@ -194,17 +191,6 @@ func (api *API) newRequest(r *http.Request, suffix string) (*http.Request, error
 	// This allows the Host header received by the backend to be consistent with other
 	// requests not going through gitlab-workhorse.
 	authReq.Host = r.Host
-	// Set a custom header for the request. This can be used in some
-	// configurations (Passenger) to solve auth request routing problems.
-	authReq.Header.Set("Gitlab-Workhorse", api.Version)
-
-	helper.SetForwardedFor(&authReq.Header, r)
-
-	tokenString, err := secret.JWTTokenString(secret.DefaultClaims)
-	if err != nil {
-		return nil, fmt.Errorf("newRequest: sign JWT: %v", err)
-	}
-	authReq.Header.Set(RequestHeader, tokenString)
 
 	return authReq, nil
 }
@@ -280,7 +266,9 @@ func (api *API) PreAuthorizeHandler(next HandleFunc, suffix string) http.Handler
 }
 
 func (api *API) doRequestWithoutRedirects(authReq *http.Request) (*http.Response, error) {
-	return api.Client.Transport.RoundTrip(authReq)
+	signingTripper := secret.NewRoundTripper(api.Client.Transport, api.Version)
+
+	return signingTripper.RoundTrip(authReq)
 }
 
 func copyAuthHeader(httpResponse *http.Response, w http.ResponseWriter) {

internal/secret/roundtripper.go

+package secret
+
+import (
+	"net/http"
+)
+
+const (
+	// This header carries the JWT token for gitlab-rails
+	RequestHeader = "Gitlab-Workhorse-Api-Request"
+)
+
+type roundTripper struct {
+	next    http.RoundTripper
+	version string
+}
+
+// NewRoundTripper creates a RoundTripper that adds the JWT token header to a
+// request. This is used to verify that a request came from workhorse
+func NewRoundTripper(next http.RoundTripper, version string) http.RoundTripper {
+	return &roundTripper{next: next, version: version}
+}
+
+func (r *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	tokenString, err := JWTTokenString(DefaultClaims)
+	if err != nil {
+		return nil, err
+	}
+
+	// Set a custom header for the request. This can be used in some
+	// configurations (Passenger) to solve auth request routing problems.
+	req.Header.Set("Gitlab-Workhorse", r.version)
+	req.Header.Set(RequestHeader, tokenString)
+
+	return r.next.RoundTrip(req)
+}

internal/upstream/routes.go

@@ -2,6 +2,7 @@ package upstream
 
 import (
 	"net/http"
+	"net/url"
 	"path"
 	"regexp"
 
@@ -19,6 +20,7 @@ import (
 	proxypkg "gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/queueing"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/redis"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/sendfile"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/sendurl"
@@ -129,6 +131,21 @@ func (ro *routeEntry) isMatch(cleanedPath string, req *http.Request) bool {
 	return ok
 }
 
+func buildProxy(backend *url.URL, version string, rt http.RoundTripper) http.Handler {
+	proxier := proxypkg.NewProxy(backend, version, rt)
+
+	return senddata.SendData(
+		sendfile.SendFile(apipkg.Block(proxier)),
+		git.SendArchive,
+		git.SendBlob,
+		git.SendDiff,
+		git.SendPatch,
+		git.SendSnapshot,
+		artifacts.SendEntry,
+		sendurl.SendURL,
+	)
+}
+
 // Routing table
 // We match against URI not containing the relativeUrlRoot:
 // see upstream.ServeHTTP
@@ -139,23 +156,12 @@ func (u *upstream) configureRoutes() {
 		u.Version,
 		u.RoundTripper,
 	)
+
 	static := &staticpages.Static{DocumentRoot: u.DocumentRoot}
-	proxy := senddata.SendData(
-		sendfile.SendFile(
-			apipkg.Block(
-				proxypkg.NewProxy(
-					u.Backend,
-					u.Version,
-					u.RoundTripper,
-				))),
-		git.SendArchive,
-		git.SendBlob,
-		git.SendDiff,
-		git.SendPatch,
-		git.SendSnapshot,
-		artifacts.SendEntry,
-		sendurl.SendURL,
-	)
+	proxy := buildProxy(u.Backend, u.Version, u.RoundTripper)
+
+	signingTripper := secret.NewRoundTripper(u.RoundTripper, u.Version)
+	signingProxy := buildProxy(u.Backend, u.Version, signingTripper)
 
 	uploadPath := path.Join(u.DocumentRoot, "uploads/tmp")
 	uploadAccelerateProxy := upload.Accelerate(&upload.SkipRailsAuthorizer{TempPath: uploadPath}, proxy)
@@ -167,7 +173,7 @@ func (u *upstream) configureRoutes() {
 		route("GET", gitProjectPattern+`info/refs\z`, git.GetInfoRefsHandler(api)),
 		route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.UploadPack(api)), withMatcher(isContentType("application/x-git-upload-pack-request"))),
 		route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.ReceivePack(api)), withMatcher(isContentType("application/x-git-receive-pack-request"))),
-		route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, proxy), withMatcher(isContentType("application/octet-stream"))),
+		route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, signingProxy), withMatcher(isContentType("application/octet-stream"))),
 
 		// CI Artifacts
 		route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(artifacts.UploadArtifacts(api, proxy))),

main_test.go

@@ -26,6 +26,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream"
 )
@@ -403,7 +404,7 @@ func TestApiContentTypeBlock(t *testing.T) {
 func TestAPIFalsePositivesAreProxied(t *testing.T) {
 	goodResponse := []byte(`<html></html>`)
 	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		if r.Header.Get(api.RequestHeader) != "" && r.Method != "GET" {
+		if r.Header.Get(secret.RequestHeader) != "" && r.Method != "GET" {
 			w.WriteHeader(500)
 			w.Write([]byte("non-GET request went through PreAuthorize handler"))
 		} else {

upload_test.go

@@ -4,10 +4,12 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
 	"regexp"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -84,17 +86,12 @@ func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.
 	})
 }
 
-func TestAcceleratedUpload(t *testing.T) {
-	reqBody, contentType, err := multipartBodyWithFile()
-	if err != nil {
-		t.Fatal(err)
-	}
-	ts := uploadTestServer(t, func(r *http.Request) {
-		jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), func(token *jwt.Token) (interface{}, error) {
+func parseJWT(token *jwt.Token) (interface{}, error) {
 	// Don't forget to validate the alg is what you expect:
 	if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
 		return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"])
 	}
+
 	testhelper.ConfigureSecret()
 	secretBytes, err := secret.Bytes()
 	if err != nil {
@@ -102,16 +99,21 @@ func TestAcceleratedUpload(t *testing.T) {
 	}
 
 	return secretBytes, nil
-		})
+}
+
+func TestAcceleratedUpload(t *testing.T) {
+	reqBody, contentType, err := multipartBodyWithFile()
 	if err != nil {
 		t.Fatal(err)
 	}
+	ts := uploadTestServer(t, func(r *http.Request) {
+		jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), parseJWT)
+		require.NoError(t, err)
 
 		rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{})
 		if len(rewrittenFields) != 1 || len(rewrittenFields["file"].(string)) == 0 {
 			t.Fatalf("Unexpected rewritten_fields value: %v", rewrittenFields)
 		}
-
 	})
 
 	defer ts.Close()
@@ -191,3 +193,69 @@ func TestBlockingRewrittenFieldsHeader(t *testing.T) {
 
 	}
 }
+
+func TestLfsUpload(t *testing.T) {
+	reqBody := "test data"
+	rspBody := "test success"
+	oid := "916f0027a575074ce72a331777c3478d6513f786a591bd892da1a577bf2335f9"
+	resource := fmt.Sprintf("/%s/gitlab-lfs/objects/%s/%d", testRepo, oid, len(reqBody))
+
+	lfsApiResponse := fmt.Sprintf(
+		`{"TempPath":%q, "LfsOid":%q, "LfsSize": %d}`,
+		scratchDir, oid, len(reqBody),
+	)
+
+	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, r.Method, "PUT")
+		switch r.RequestURI {
+		case resource + "/authorize":
+			// Expect the authorization call to be signed
+			_, err := jwt.Parse(r.Header.Get(secret.RequestHeader), parseJWT)
+			require.NoError(t, err)
+
+			// Instruct workhorse to accept the upload
+			w.Header().Set("Content-Type", api.ResponseContentType)
+			_, err = fmt.Fprint(w, lfsApiResponse)
+			require.NoError(t, err)
+
+		case resource:
+			// Expect the finalization call to be signed
+			_, err := jwt.Parse(r.Header.Get(secret.RequestHeader), parseJWT)
+			require.NoError(t, err)
+
+			// Expect the request to point to a file on disk containing the data
+			require.NoError(t, r.ParseForm())
+			require.Equal(t, oid, r.Form.Get("file.sha256"), "Invalid SHA256 populated")
+			require.Equal(t, strconv.Itoa(len(reqBody)), r.Form.Get("file.size"), "Invalid size populated")
+
+			tempfile, err := ioutil.ReadFile(r.Form.Get("file.path"))
+			require.NoError(t, err)
+			require.Equal(t, reqBody, string(tempfile), "Temporary file has the wrong body")
+
+			fmt.Fprint(w, rspBody)
+		default:
+			t.Fatalf("Unexpected request to upstream! %v %q", r.Method, r.RequestURI)
+		}
+	})
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	req, err := http.NewRequest("PUT", ws.URL+resource, strings.NewReader(reqBody))
+	require.NoError(t, err)
+
+	req.Header.Set("Content-Type", "application/octet-stream")
+	req.ContentLength = int64(len(reqBody))
+
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+
+	defer resp.Body.Close()
+	rspData, err := ioutil.ReadAll(resp.Body)
+	require.NoError(t, err)
+
+	// Expect the (eventual) response to be proxied through, untouched
+	require.Equal(t, 200, resp.StatusCode)
+	require.Equal(t, rspBody, string(rspData))
+}