Prepare for lazy upload preauth

These are Workhorse refactors to prepare for having lazy
pre-authentication of multipart MIME file uploads.

Changelog: other

workhorse/internal/upload/artifacts_uploader.go

@@ -43,16 +43,12 @@ type artifactsUploadProcessor struct {
 // Artifacts is like a Multipart but specific for artifacts upload.
 func Artifacts(myAPI *api.API, h http.Handler, p Preparer) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		opts, err := p.Prepare(a)
-		if err != nil {
-			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: error preparing file storage options"))
-			return
-		}
-
 		format := r.URL.Query().Get(ArtifactFormatKey)
-
-		mg := &artifactsUploadProcessor{format: format, SavedFileTracker: SavedFileTracker{Request: r}}
-		interceptMultipartFiles(w, r, h, a, mg, opts)
+		mg := &artifactsUploadProcessor{
+			format:           format,
+			SavedFileTracker: SavedFileTracker{Request: r},
+		}
+		interceptMultipartFiles(w, r, h, mg, &eagerAuthorizer{a}, p)
 	}, "/authorize")
 }

workhorse/internal/upload/multipart_uploader.go

 package upload
 import (
-	"fmt"
 	"net/http"
 	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/helper"
 )
 // Multipart is a request middleware. If the request has a MIME multipart
@@ -17,12 +15,19 @@ func Multipart(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler {
 	return rails.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		s := &SavedFileTracker{Request: r}
-		opts, err := p.Prepare(a)
-		if err != nil {
-			helper.Fail500(w, r, fmt.Errorf("Multipart: error preparing file storage options"))
-			return
-		}
-
-		interceptMultipartFiles(w, r, h, a, s, opts)
+		interceptMultipartFiles(w, r, h, s, &eagerAuthorizer{a}, p)
 	}, "/authorize")
 }
+
+// SkipRailsPreAuthMultipart behaves like Multipart except it does not
+// pre-authorize with Rails. It is intended for use on catch-all routes
+// 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 {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		s := &SavedFileTracker{Request: r}
+		fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
+		interceptMultipartFiles(w, r, h, s, fa, p)
+	})
+}

workhorse/internal/upload/rewrite.go

@@ -62,13 +62,14 @@ var (
 )
 type rewriter struct {
-	writer          *multipart.Writer
-	preauth         *api.Response
+	writer *multipart.Writer
+	fileAuthorizer
+	Preparer
 	filter          MultipartFormProcessor
 	finalizedFields map[string]bool
 }
-func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, preauth *api.Response, filter MultipartFormProcessor, opts *destination.UploadOpts) error {
+func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, filter MultipartFormProcessor, fa fileAuthorizer, preparer Preparer) error {
 	// Create multipart reader
 	reader, err := r.MultipartReader()
 	if err != nil {
@@ -83,7 +84,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 	rew := &rewriter{
 		writer:          writer,
-		preauth:         preauth,
+		fileAuthorizer:  fa,
+		Preparer:        preparer,
 		filter:          filter,
 		finalizedFields: make(map[string]bool),
 	}
@@ -108,7 +110,7 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr
 		}
 		if filename != "" {
-			err = rew.handleFilePart(r.Context(), name, p, opts)
+			err = rew.handleFilePart(r, name, p)
 		} else {
 			err = rew.copyPart(r.Context(), name, p)
 		}
@@ -128,7 +130,7 @@ func parseAndNormalizeContentDisposition(header textproto.MIMEHeader) (string, s
 	return params["name"], params["filename"]
 }
-func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *destination.UploadOpts) error {
+func (rew *rewriter) handleFilePart(r *http.Request, name string, p *multipart.Part) error {
 	if rew.filter.Count() >= maxFilesAllowed {
 		return ErrTooManyFilesUploaded
 	}
@@ -141,22 +143,28 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 		return fmt.Errorf("illegal filename: %q", filename)
 	}
-	var inputReader io.ReadCloser
-	var err error
-	imageType := exif.FileTypeFromSuffix(filename)
-	switch {
-	case imageType != exif.TypeUnknown:
+	apiResponse, err := rew.AuthorizeFile(r)
+	if err != nil {
+		return err
+	}
+	opts, err := rew.Prepare(apiResponse)
+	if err != nil {
+		return err
+	}
+	var inputReader io.ReadCloser
+	ctx := r.Context()
+	if imageType := exif.FileTypeFromSuffix(filename); imageType != exif.TypeUnknown {
 		inputReader, err = handleExifUpload(ctx, p, filename, imageType)
 		if err != nil {
 			return err
 		}
-	case rew.preauth.ProcessLsif:
-		inputReader, err = handleLsifUpload(ctx, p, opts.LocalTempPath, filename, rew.preauth)
+	} else if apiResponse.ProcessLsif {
+		inputReader, err = handleLsifUpload(ctx, p, opts.LocalTempPath, filename)
 		if err != nil {
 			return err
 		}
-	default:
+	} else {
 		inputReader = ioutil.NopCloser(p)
 	}
@@ -265,7 +273,7 @@ func isJPEG(r io.Reader) bool {
 	return http.DetectContentType(buf) == "image/jpeg"
 }
-func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) {
+func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string) (io.ReadCloser, error) {
 	parserConfig := parser.Config{
 		TempPath: tempPath,
 	}
@@ -289,3 +297,15 @@ func (rew *rewriter) copyPart(ctx context.Context, name string, p *multipart.Par
 	return nil
 }
+
+type fileAuthorizer interface {
+	AuthorizeFile(*http.Request) (*api.Response, error)
+}
+
+type eagerAuthorizer struct{ response *api.Response }
+
+func (ea *eagerAuthorizer) AuthorizeFile(r *http.Request) (*api.Response, error) {
+	return ea.response, nil
+}
+
+var _ fileAuthorizer = &eagerAuthorizer{}

workhorse/internal/upload/skip_rails_authorizer.go

-package upload
-
-import (
-	"net/http"
-
-	"gitlab.com/gitlab-org/gitlab/workhorse/internal/api"
-)
-
-// SkipRailsAuthorizer implements a fake PreAuthorizer that does not call
-// the gitlab-rails API. It must be fast because it gets called on each
-// request proxied to Rails.
-type SkipRailsAuthorizer struct {
-	// TempPath is a directory where workhorse can store files that can later
-	// be accessed by gitlab-rails.
-	TempPath string
-}
-
-func (l *SkipRailsAuthorizer) PreAuthorizeHandler(next api.HandleFunc, _ string) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		next(w, r, &api.Response{TempPath: l.TempPath})
-	})
-}

workhorse/internal/upload/uploads.go

@@ -40,13 +40,13 @@ type MultipartFormProcessor interface {
 // interceptMultipartFiles is the core of the implementation of
 // Multipart.
-func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *destination.UploadOpts) {
+func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, filter MultipartFormProcessor, fa fileAuthorizer, p Preparer) {
 	var body bytes.Buffer
 	writer := multipart.NewWriter(&body)
 	defer writer.Close()
 	// Rewrite multipart form data
-	err := rewriteFormFilesFromMultipart(r, writer, preauth, filter, opts)
+	err := rewriteFormFilesFromMultipart(r, writer, filter, fa, p)
 	if err != nil {
 		switch err {
 		case ErrInjectedClientParam:

workhorse/internal/upload/uploads_test.go

@@ -71,12 +71,10 @@ func TestUploadHandlerForwardingRawData(t *testing.T) {
 	response := httptest.NewRecorder()
 	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
+	fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
 	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, handler, apiResponse, nil, opts)
+	interceptMultipartFiles(response, httpRequest, handler, nil, fa, preparer)
 	require.Equal(t, 202, response.Code)
 	require.Equal(t, "RESPONSE", response.Body.String(), "response body")
@@ -139,12 +137,10 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) {
 	handler := newProxy(ts.URL)
-	apiResponse := &api.Response{TempPath: tempPath}
+	fa := &eagerAuthorizer{&api.Response{TempPath: tempPath}}
 	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts)
+	interceptMultipartFiles(response, httpRequest, handler, &testFormProcessor{}, fa, preparer)
 	require.Equal(t, 202, response.Code)
 	cancel() // this will trigger an async cleanup
@@ -303,11 +299,10 @@ func TestUploadProcessingFile(t *testing.T) {
 			httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 			response := httptest.NewRecorder()
+			fa := &eagerAuthorizer{test.preauth}
 			preparer := &DefaultPreparer{}
-			opts, err := preparer.Prepare(test.preauth)
-			require.NoError(t, err)
-			interceptMultipartFiles(response, httpRequest, nilHandler, test.preauth, &testFormProcessor{}, opts)
+			interceptMultipartFiles(response, httpRequest, nilHandler, &testFormProcessor{}, fa, preparer)
 			require.Equal(t, 200, response.Code)
 			require.Equal(t, "test", string(test.content(t)))
@@ -317,7 +312,6 @@ func TestUploadProcessingFile(t *testing.T) {
 func TestInvalidFileNames(t *testing.T) {
 	testhelper.ConfigureSecret()
-	tempPath := t.TempDir()
 	for _, testCase := range []struct {
 		filename       string
@@ -345,12 +339,7 @@ func TestInvalidFileNames(t *testing.T) {
 		httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 		response := httptest.NewRecorder()
-		apiResponse := &api.Response{TempPath: tempPath}
-		preparer := &DefaultPreparer{}
-		opts, err := preparer.Prepare(apiResponse)
-		require.NoError(t, err)
-
-		interceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
+		testInterceptMultipartFiles(t, response, httpRequest, nilHandler, &SavedFileTracker{Request: httpRequest})
 		require.Equal(t, testCase.code, response.Code)
 	}
 }
@@ -542,10 +531,8 @@ func waitUntilDeleted(t *testing.T, path string) {
 func testInterceptMultipartFiles(t *testing.T, w http.ResponseWriter, r *http.Request, h http.Handler, filter MultipartFormProcessor) {
 	t.Helper()
-	apiResponse := &api.Response{TempPath: t.TempDir()}
+	fa := &eagerAuthorizer{&api.Response{TempPath: t.TempDir()}}
 	preparer := &DefaultPreparer{}
-	opts, err := preparer.Prepare(apiResponse)
-	require.NoError(t, err)
-	interceptMultipartFiles(w, r, h, apiResponse, filter, opts)
+	interceptMultipartFiles(w, r, h, filter, fa, preparer)
 }

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.Multipart(&upload.SkipRailsAuthorizer{TempPath: uploadPath}, proxy, preparer)
+	tempfileMultipartProxy := upload.SkipRailsPreAuthMultipart(uploadPath, proxy, preparer)
 	ciAPIProxyQueue := queueing.QueueRequests("ci_api_job_requests", tempfileMultipartProxy, u.APILimit, u.APIQueueLimit, u.APIQueueTimeout)
 	ciAPILongPolling := builds.RegisterHandler(ciAPIProxyQueue, redis.WatchKey, u.APICILongPollingDuration)