Add latest changes from gitlab-org/gitlab@master

5 files changed, 91 insertions, 25 deletions

diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go
index 8954923ad75..a00cff34b89 100644
--- a/workhorse/internal/api/api.go
+++ b/workhorse/internal/api/api.go
@@ -3,6 +3,7 @@ package api
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -263,26 +264,22 @@ func (api *API) newRequest(r *http.Request, suffix string) (*http.Request, error
 
 // PreAuthorize performs a pre-authorization check against the API for the given HTTP request
 //
-// If `outErr` is set, the other fields will be nil and it should be treated as
-// a 500 error.
+// If the returned *http.Response is not nil, the caller is responsible for closing its body
 //
-// If httpResponse is present, the caller is responsible for closing its body
-//
-// authResponse will only be present if the authorization check was successful
-func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http.Response, authResponse *Response, outErr error) {
+// Only upon successful authorization do we return a non-nil *Response
+func (api *API) PreAuthorize(suffix string, r *http.Request) (_ *http.Response, _ *Response, err error) {
 	authReq, err := api.newRequest(r, suffix)
 	if err != nil {
 		return nil, nil, fmt.Errorf("preAuthorizeHandler newUpstreamRequest: %v", err)
 	}
 
-	httpResponse, err = api.doRequestWithoutRedirects(authReq)
+	httpResponse, err := api.doRequestWithoutRedirects(authReq)
 	if err != nil {
 		return nil, nil, fmt.Errorf("preAuthorizeHandler: do request: %v", err)
 	}
 	defer func() {
-		if outErr != nil {
+		if err != nil {
 			httpResponse.Body.Close()
-			httpResponse = nil
 		}
 	}()
 	requestsCounter.WithLabelValues(strconv.Itoa(httpResponse.StatusCode), authReq.Method).Inc()
@@ -293,17 +290,43 @@ func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http
 		return httpResponse, nil, nil
 	}
 
-	authResponse = &Response{}
+	authResponse := &Response{}
 	// The auth backend validated the client request and told us additional
 	// request metadata. We must extract this information from the auth
 	// response body.
 	if err := json.NewDecoder(httpResponse.Body).Decode(authResponse); err != nil {
-		return httpResponse, nil, fmt.Errorf("preAuthorizeHandler: decode authorization response: %v", err)
+		return nil, nil, fmt.Errorf("preAuthorizeHandler: decode authorization response: %v", err)
 	}
 
 	return httpResponse, authResponse, nil
 }
 
+// PreAuthorizeFixedPath makes an internal Workhorse API call to a fixed
+// path, using the HTTP headers of r.
+func (api *API) PreAuthorizeFixedPath(r *http.Request, method string, path string) (*Response, error) {
+	authReq, err := http.NewRequestWithContext(r.Context(), method, api.URL.String(), nil)
+	if err != nil {
+		return nil, fmt.Errorf("construct auth request: %w", err)
+	}
+	authReq.Header = helper.HeaderClone(r.Header)
+
+	ignoredResponse, apiResponse, err := api.PreAuthorize(path, authReq)
+	if err != nil {
+		return nil, fmt.Errorf("PreAuthorize: %w", err)
+	}
+
+	// We don't need the contents of ignoredResponse but we are responsible
+	// for closing it. Part of the reason PreAuthorizeFixedPath exists is to
+	// hide this awkwardness.
+	ignoredResponse.Body.Close()
+
+	if apiResponse == nil {
+		return nil, errors.New("no api response on fixed path")
+	}
+
+	return apiResponse, nil
+}
+
 func (api *API) PreAuthorizeHandler(next HandleFunc, suffix string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		httpResponse, authResponse, err := api.PreAuthorize(suffix, r)
diff --git a/workhorse/internal/upload/multipart_uploader.go b/workhorse/internal/upload/multipart_uploader.go
index c029b484955..2456a2c8626 100644
--- a/workhorse/internal/upload/multipart_uploader.go
+++ b/workhorse/internal/upload/multipart_uploader.go
@@ -24,10 +24,18 @@ func Multipart(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler {
 // where we cannot pre-authorize both because we don't know which Rails
 // endpoint to call, and because eagerly pre-authorizing would add too
 // much overhead.
-func SkipRailsPreAuthMultipart(tempPath string, h http.Handler, p Preparer) http.Handler {
+func SkipRailsPreAuthMultipart(tempPath string, myAPI *api.API, h http.Handler, p Preparer) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		s := &SavedFileTracker{Request: r}
-		fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
+
+		// We use testAuthorizer as a temporary measure. When
+		// https://gitlab.com/groups/gitlab-com/gl-infra/-/epics/742 is done, we
+		// should only be using apiAuthorizer.
+		fa := &testAuthorizer{
+			test:   &apiAuthorizer{myAPI},
+			actual: &eagerAuthorizer{&api.Response{TempPath: tempPath}},
+		}
+
 		interceptMultipartFiles(w, r, h, s, fa, p)
 	})
 }
diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go
index 91b5f7c01d5..200b3efc554 100644
--- a/workhorse/internal/upload/rewrite.go
+++ b/workhorse/internal/upload/rewrite.go
@@ -16,10 +16,10 @@ import (
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promauto"
-	"gitlab.com/gitlab-org/labkit/log"
-
 	"golang.org/x/image/tiff"
 
+	"gitlab.com/gitlab-org/gitlab/workhorse/internal/log"
+
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/lsif_transformer/parser"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/upload/destination"
@@ -233,14 +233,14 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy
 		log.WithContextFields(ctx, log.Fields{
 			"filename":  filename,
 			"imageType": imageType,
-		}).Print("invalid content type, not running exiftool")
+		}).Info("invalid content type, not running exiftool")
 
 		return tmpfile, nil
 	}
 
 	log.WithContextFields(ctx, log.Fields{
 		"filename": filename,
-	}).Print("running exiftool to remove any metadata")
+	}).Info("running exiftool to remove any metadata")
 
 	cleaner, err := exif.NewCleaner(ctx, tmpfile)
 	if err != nil {
@@ -309,3 +309,35 @@ func (ea *eagerAuthorizer) AuthorizeFile(r *http.Request) (*api.Response, error)
 }
 
 var _ fileAuthorizer = &eagerAuthorizer{}
+
+type apiAuthorizer struct {
+	api *api.API
+}
+
+func (aa *apiAuthorizer) AuthorizeFile(r *http.Request) (*api.Response, error) {
+	return aa.api.PreAuthorizeFixedPath(
+		r,
+		"POST",
+		"/api/v4/internal/workhorse/authorize_upload",
+	)
+}
+
+var _ fileAuthorizer = &apiAuthorizer{}
+
+type testAuthorizer struct {
+	test   fileAuthorizer
+	actual fileAuthorizer
+}
+
+func (ta *testAuthorizer) AuthorizeFile(r *http.Request) (*api.Response, error) {
+	logger := log.WithRequest(r)
+	if response, err := ta.test.AuthorizeFile(r); err != nil {
+		logger.WithError(err).Error("test api preauthorize request failed")
+	} else {
+		logger.WithFields(log.Fields{
+			"temp_path": response.TempPath,
+		}).Info("test api preauthorize request")
+	}
+
+	return ta.actual.AuthorizeFile(r)
+}
diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go
index 1a514bbc225..95c9b99b833 100644
--- a/workhorse/internal/upstream/routes.go
+++ b/workhorse/internal/upstream/routes.go
@@ -223,7 +223,7 @@ func configureRoutes(u *upstream) {
 	mimeMultipartUploader := upload.Multipart(api, signingProxy, preparer)
 
 	uploadPath := path.Join(u.DocumentRoot, "uploads/tmp")
-	tempfileMultipartProxy := upload.SkipRailsPreAuthMultipart(uploadPath, proxy, preparer)
+	tempfileMultipartProxy := upload.SkipRailsPreAuthMultipart(uploadPath, api, proxy, preparer)
 	ciAPIProxyQueue := queueing.QueueRequests("ci_api_job_requests", tempfileMultipartProxy, u.APILimit, u.APIQueueLimit, u.APIQueueTimeout)
 	ciAPILongPolling := builds.RegisterHandler(ciAPIProxyQueue, redis.WatchKey, u.APICILongPollingDuration)
 
diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go
index 180598ab260..5a08e80798a 100644
--- a/workhorse/upload_test.go
+++ b/workhorse/upload_test.go
@@ -68,7 +68,7 @@ func expectSignedRequest(t *testing.T, r *http.Request) {
 
 func uploadTestServer(t *testing.T, authorizeTests func(r *http.Request), extraTests func(r *http.Request)) *httptest.Server {
 	return testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		if strings.HasSuffix(r.URL.Path, "/authorize") {
+		if strings.HasSuffix(r.URL.Path, "/authorize") || r.URL.Path == "/api/v4/internal/workhorse/authorize_upload" {
 			expectSignedRequest(t, r)
 
 			w.Header().Set("Content-Type", api.ResponseContentType)
@@ -154,6 +154,10 @@ func TestAcceleratedUpload(t *testing.T) {
 		t.Run(tt.resource, func(t *testing.T) {
 			ts := uploadTestServer(t,
 				func(r *http.Request) {
+					if r.URL.Path == "/api/v4/internal/workhorse/authorize_upload" {
+						// Nothing to validate: this is a hard coded URL
+						return
+					}
 					resource := strings.TrimRight(tt.resource, "/")
 					// Validate %2F characters haven't been unescaped
 					require.Equal(t, resource+"/authorize", r.URL.String())
@@ -270,24 +274,23 @@ func TestUnacceleratedUploads(t *testing.T) {
 
 func TestBlockingRewrittenFieldsHeader(t *testing.T) {
 	canary := "untrusted header passed by user"
+	multiPartBody, multiPartContentType, err := multipartBodyWithFile()
+	require.NoError(t, err)
+
 	testCases := []struct {
 		desc        string
 		contentType string
 		body        io.Reader
 		present     bool
 	}{
-		{"multipart with file", "", nil, true}, // placeholder
+		{"multipart with file", multiPartContentType, multiPartBody, true},
 		{"no multipart", "text/plain", nil, false},
 	}
 
-	var err error
-	testCases[0].body, testCases[0].contentType, err = multipartBodyWithFile()
-	require.NoError(t, err)
-
 	for _, tc := range testCases {
 		ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
 			key := upload.RewrittenFieldsHeader
-			if tc.present {
+			if tc.present && r.URL.Path != "/api/v4/internal/workhorse/authorize_upload" {
 				require.Contains(t, r.Header, key)
 			} else {
 				require.NotContains(t, r.Header, key)