diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-02-18 09:45:46 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-02-18 09:45:46 +0000 |
commit | a7b3560714b4d9cc4ab32dffcd1f74a284b93580 (patch) | |
tree | 7452bd5c3545c2fa67a28aa013835fb4fa071baf /workhorse | |
parent | ee9173579ae56a3dbfe5afe9f9410c65bb327ca7 (diff) | |
download | gitlab-ce-a7b3560714b4d9cc4ab32dffcd1f74a284b93580.tar.gz |
Add latest changes from gitlab-org/gitlab@14-8-stable-eev14.8.0-rc42
Diffstat (limited to 'workhorse')
19 files changed, 329 insertions, 143 deletions
diff --git a/workhorse/.tool-versions b/workhorse/.tool-versions index bf23029f784..29cc9a03144 100644 --- a/workhorse/.tool-versions +++ b/workhorse/.tool-versions @@ -1 +1 @@ -golang 1.16.12 +golang 1.17.6 diff --git a/workhorse/Makefile b/workhorse/Makefile index 031fe581d28..44b3e2b8248 100644 --- a/workhorse/Makefile +++ b/workhorse/Makefile @@ -12,7 +12,7 @@ ifdef SOURCE_DATE_EPOCH else BUILD_TIME := $(shell date -u "$(DATE_FMT)") endif -GOBUILD := go build -ldflags "-X main.Version=$(VERSION_STRING) -X main.BuildTime=$(BUILD_TIME)" +GO_BUILD_GENERIC_LDFLAGS := -X main.Version=$(VERSION_STRING) -X main.BuildTime=$(BUILD_TIME) GITALY := tmp/tests/gitaly/_build/bin/gitaly GITALY_PID_FILE := gitaly.pid EXE_ALL := gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata gitlab-workhorse @@ -30,31 +30,35 @@ define message @echo "### $(1)" endef +# To compute a unique and deterministic value for GNU build-id, we build the Go binary a second time. +# From the first build, we extract its unique and deterministic Go build-id, and use that to derive +# a comparably unique and deterministic GNU build-id to inject into the final binary. +# If we cannot extract a Go build-id, we punt and fallback to using a random 32-byte hex string. +# This fallback is unique but non-deterministic. Uniqueness is critical, because the GNU build-id +# can be used as a cache key in a build cache. Without the fallback, we risk cache key collisions. +## Skip generation of the GNU build ID if set to speed up builds. +WITHOUT_BUILD_ID ?= .NOTPARALLEL: .PHONY: all all: clean-build $(EXE_ALL) -.PHONY: gitlab-resize-image -gitlab-resize-image: +.PHONY: gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata +gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata: $(call message,Building $@) - $(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@ - -.PHONY: gitlab-zip-cat -gitlab-zip-cat: - $(call message,Building $@) - $(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@ - -.PHONY: gitlab-zip-metadata -gitlab-zip-metadata: - $(call message,Building $@) - $(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@ + go build -ldflags "$(GO_BUILD_GENERIC_LDFLAGS)" -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@ +ifndef WITHOUT_BUILD_ID + go build -ldflags "$(GO_BUILD_GENERIC_LDFLAGS) -B 0x$$(_support/make-gnu-build-id.sh $(BUILD_DIR)/$@)" -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@ +endif .PHONY: gitlab-workhorse gitlab-workhorse: $(call message,Building $@) - $(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG) + go build -ldflags "$(GO_BUILD_GENERIC_LDFLAGS)" -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG) +ifndef WITHOUT_BUILD_ID + go build -ldflags "$(GO_BUILD_GENERIC_LDFLAGS) -B 0x$$(_support/make-gnu-build-id.sh $(BUILD_DIR)/$@)" -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG) +endif .PHONY: install install: $(EXE_ALL) diff --git a/workhorse/_support/make-gnu-build-id.sh b/workhorse/_support/make-gnu-build-id.sh new file mode 100755 index 00000000000..815966dab7e --- /dev/null +++ b/workhorse/_support/make-gnu-build-id.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +main() +{ + GO_BINARY=$1 + + if [ $# -ne 1 ] || [ ! -f $GO_BINARY ] ; then + fail "Usage: $0 [path_to_go_binary]" + fi + + GO_BUILD_ID=$( go tool buildid "$GO_BINARY" || openssl rand -hex 32 ) + if [ -z "$GO_BUILD_ID" ] ; then + fail "ERROR: Could not extract Go build-id or generate a random hex string." + fi + + GNU_BUILD_ID=$( echo $GO_BUILD_ID | sha1sum | cut -d' ' -f1 ) + if [ -z "$GNU_BUILD_ID" ] ; then + fail "ERROR: Could not generate a GNU build-id" + fi + + echo "$GNU_BUILD_ID" +} + +fail() +{ + echo "$@" 1>&2 + exit 1 +} + +main "$@" diff --git a/workhorse/internal/artifacts/artifacts_store_test.go b/workhorse/internal/artifacts/artifacts_store_test.go index a01a723298f..f9fb28cf7ce 100644 --- a/workhorse/internal/artifacts/artifacts_store_test.go +++ b/workhorse/internal/artifacts/artifacts_store_test.go @@ -284,12 +284,9 @@ func TestUploadHandlerMultipartUploadSizeLimit(t *testing.T) { contentBuffer, contentType := createTestMultipartForm(t, make([]byte, uploadSize)) response := testUploadArtifacts(t, contentType, ts.URL+Path, &contentBuffer) require.Equal(t, http.StatusRequestEntityTooLarge, response.Code) - - // Poll because AbortMultipartUpload is async - for i := 0; os.IsMultipartUpload(test.ObjectPath) && i < 100; i++ { - time.Sleep(10 * time.Millisecond) - } - require.False(t, os.IsMultipartUpload(test.ObjectPath), "MultipartUpload should not be in progress anymore") + require.Eventually(t, func() bool { + return !os.IsMultipartUpload(test.ObjectPath) + }, time.Second, time.Millisecond, "MultipartUpload should not be in progress anymore") require.Empty(t, os.GetObjectMD5(test.ObjectPath), "upload should have failed, so the object should not exists") } diff --git a/workhorse/internal/artifacts/artifacts_upload.go b/workhorse/internal/artifacts/artifacts_upload.go index ef289fbb93b..f1fd69082f8 100644 --- a/workhorse/internal/artifacts/artifacts_upload.go +++ b/workhorse/internal/artifacts/artifacts_upload.go @@ -162,6 +162,6 @@ func UploadArtifacts(myAPI *api.API, h http.Handler, p upload.Preparer) http.Han format := r.URL.Query().Get(ArtifactFormatKey) mg := &artifactsUploadProcessor{opts: opts, format: format, SavedFileTracker: upload.SavedFileTracker{Request: r}} - upload.HandleFileUploads(w, r, h, a, mg, opts) + upload.InterceptMultipartFiles(w, r, h, a, mg, opts) }, "/authorize") } diff --git a/workhorse/internal/dependencyproxy/dependencyproxy_test.go b/workhorse/internal/dependencyproxy/dependencyproxy_test.go index d9169b2b4ce..6056433f3b1 100644 --- a/workhorse/internal/dependencyproxy/dependencyproxy_test.go +++ b/workhorse/internal/dependencyproxy/dependencyproxy_test.go @@ -91,12 +91,12 @@ func TestInject(t *testing.T) { })) defer originResourceServer.Close() - // BodyUploader expects http.Handler as its second param, we can create a stub function and verify that + // RequestBody expects http.Handler as its second param, we can create a stub function and verify that // it's only called for successful requests handlerIsCalled := false handlerFunc := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { handlerIsCalled = true }) - bodyUploader := upload.BodyUploader(&fakePreAuthHandler{}, handlerFunc, &upload.DefaultPreparer{}) + bodyUploader := upload.RequestBody(&fakePreAuthHandler{}, handlerFunc, &upload.DefaultPreparer{}) injector := NewInjector() injector.SetUploadHandler(bodyUploader) diff --git a/workhorse/internal/filestore/file_handler_test.go b/workhorse/internal/filestore/file_handler_test.go index f57026a59df..2fd034bb761 100644 --- a/workhorse/internal/filestore/file_handler_test.go +++ b/workhorse/internal/filestore/file_handler_test.go @@ -28,29 +28,15 @@ func testDeadline() time.Time { func requireFileGetsRemovedAsync(t *testing.T, filePath string) { var err error - - // Poll because the file removal is async - for i := 0; i < 100; i++ { + require.Eventually(t, func() bool { _, err = os.Stat(filePath) - if err != nil { - break - } - time.Sleep(100 * time.Millisecond) - } - + return err != nil + }, 10*time.Second, 10*time.Millisecond) require.True(t, os.IsNotExist(err), "File hasn't been deleted during cleanup") } func requireObjectStoreDeletedAsync(t *testing.T, expectedDeletes int, osStub *test.ObjectstoreStub) { - // Poll because the object removal is async - for i := 0; i < 100; i++ { - if osStub.DeletesCnt() == expectedDeletes { - break - } - time.Sleep(10 * time.Millisecond) - } - - require.Equal(t, expectedDeletes, osStub.DeletesCnt(), "Object not deleted") + require.Eventually(t, func() bool { return osStub.DeletesCnt() == expectedDeletes }, time.Second, time.Millisecond, "Object not deleted") } func TestSaveFileWrongSize(t *testing.T) { diff --git a/workhorse/internal/lfs/lfs.go b/workhorse/internal/lfs/lfs.go index 363e2113422..e26f59046ea 100644 --- a/workhorse/internal/lfs/lfs.go +++ b/workhorse/internal/lfs/lfs.go @@ -6,7 +6,6 @@ package lfs import ( "fmt" - "net/http" "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" "gitlab.com/gitlab-org/gitlab/workhorse/internal/config" @@ -49,7 +48,3 @@ func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, uplo return opts, &object{oid: a.LfsOid, size: a.LfsSize}, nil } - -func PutStore(a *api.API, h http.Handler, p upload.Preparer) http.Handler { - return upload.BodyUploader(a, h, p) -} diff --git a/workhorse/internal/objectstore/object_test.go b/workhorse/internal/objectstore/object_test.go index c15248edf78..b9c1fb2087b 100644 --- a/workhorse/internal/objectstore/object_test.go +++ b/workhorse/internal/objectstore/object_test.go @@ -56,13 +56,7 @@ func testObjectUploadNoErrors(t *testing.T, startObjectStore osFactory, useDelet if useDeleteURL { expectedDeleteCnt = 1 } - // Poll because the object removal is async - for i := 0; i < 100; i++ { - if osStub.DeletesCnt() == expectedDeleteCnt { - break - } - time.Sleep(10 * time.Millisecond) - } + require.Eventually(t, func() bool { return osStub.DeletesCnt() == expectedDeleteCnt }, time.Second, time.Millisecond) if useDeleteURL { require.Equal(t, 1, osStub.DeletesCnt(), "Object hasn't been deleted") diff --git a/workhorse/internal/redis/keywatcher_test.go b/workhorse/internal/redis/keywatcher_test.go index 99892bc64b8..7ff5f8204c0 100644 --- a/workhorse/internal/redis/keywatcher_test.go +++ b/workhorse/internal/redis/keywatcher_test.go @@ -180,11 +180,7 @@ func TestShutdown(t *testing.T) { }() go func() { - for countWatchers(runnerKey) == 0 { - time.Sleep(time.Millisecond) - } - - require.Equal(t, 1, countWatchers(runnerKey)) + require.Eventually(t, func() bool { return countWatchers(runnerKey) == 1 }, 10*time.Second, time.Millisecond) Shutdown() wg.Done() @@ -192,11 +188,7 @@ func TestShutdown(t *testing.T) { wg.Wait() - for countWatchers(runnerKey) == 1 { - time.Sleep(time.Millisecond) - } - - require.Equal(t, 0, countWatchers(runnerKey)) + require.Eventually(t, func() bool { return countWatchers(runnerKey) == 0 }, 10*time.Second, time.Millisecond) // Adding a key after the shutdown should result in an immediate response var val WatchKeyStatus diff --git a/workhorse/internal/upload/accelerate.go b/workhorse/internal/upload/accelerate.go index 81f44d33a82..28d3b3dee2e 100644 --- a/workhorse/internal/upload/accelerate.go +++ b/workhorse/internal/upload/accelerate.go @@ -17,16 +17,21 @@ type MultipartClaims struct { jwt.StandardClaims } -func Accelerate(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler { +// Multipart is a request middleware. If the request has a MIME multipart +// request body, the middleware will iterate through the multipart parts. +// When it finds a file part (filename != ""), the middleware will save +// the file contents to a temporary location and replace the file part +// with a reference to the temporary location. +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("Accelerate: error preparing file storage options")) + helper.Fail500(w, r, fmt.Errorf("Multipart: error preparing file storage options")) return } - HandleFileUploads(w, r, h, a, s, opts) + InterceptMultipartFiles(w, r, h, a, s, opts) }, "/authorize") } diff --git a/workhorse/internal/upload/body_uploader.go b/workhorse/internal/upload/body_uploader.go index 4849d9bae75..6c53bd9241b 100644 --- a/workhorse/internal/upload/body_uploader.go +++ b/workhorse/internal/upload/body_uploader.go @@ -16,16 +16,23 @@ type PreAuthorizer interface { PreAuthorizeHandler(next api.HandleFunc, suffix string) http.Handler } -// Verifier allows to check an upload before sending it to rails +// Verifier is an optional pluggable behavior for upload paths. If +// Verify() returns an error, Workhorse will return an error response to +// the client instead of propagating the request to Rails. The motivating +// use case is Git LFS, where Workhorse checks the size and SHA256 +// checksum of the uploaded file. type Verifier interface { - // Verify can abort the upload returning an error + // Verify can abort the upload by returning an error Verify(handler *filestore.FileHandler) error } -// Preparer allows to customize BodyUploader configuration +// Preparer is a pluggable behavior that interprets a Rails API response +// and either tells Workhorse how to handle the upload, via the +// SaveFileOpts and Verifier, or it rejects the request by returning a +// non-nil error. Its intended use is to make sure the upload gets stored +// in the right location: either a local directory, or one of several +// supported object storage backends. type Preparer interface { - // Prepare converts api.Response into a *SaveFileOpts, it can optionally return an Verifier that will be - // invoked after the real upload, before the finalization with rails Prepare(a *api.Response) (*filestore.SaveFileOpts, Verifier, error) } @@ -36,26 +43,26 @@ func (s *DefaultPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, Ver return opts, nil, err } -// BodyUploader is an http.Handler that perform a pre authorization call to rails before hijacking the request body and -// uploading it. -// Providing an Preparer allows to customize the upload process -func BodyUploader(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler { +// RequestBody is a request middleware. It will store the request body to +// a location by determined an api.Response value. It then forwards the +// request to gitlab-rails without the original request body. +func RequestBody(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler { return rails.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) { opts, verifier, err := p.Prepare(a) if err != nil { - helper.Fail500(w, r, fmt.Errorf("BodyUploader: preparation failed: %v", err)) + helper.Fail500(w, r, fmt.Errorf("RequestBody: preparation failed: %v", err)) return } fh, err := filestore.SaveFileFromReader(r.Context(), r.Body, r.ContentLength, opts) if err != nil { - helper.Fail500(w, r, fmt.Errorf("BodyUploader: upload failed: %v", err)) + helper.Fail500(w, r, fmt.Errorf("RequestBody: upload failed: %v", err)) return } if verifier != nil { if err := verifier.Verify(fh); err != nil { - helper.Fail500(w, r, fmt.Errorf("BodyUploader: verification failed: %v", err)) + helper.Fail500(w, r, fmt.Errorf("RequestBody: verification failed: %v", err)) return } } @@ -63,7 +70,7 @@ func BodyUploader(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler data := url.Values{} fields, err := fh.GitLabFinalizeFields("file") if err != nil { - helper.Fail500(w, r, fmt.Errorf("BodyUploader: finalize fields failed: %v", err)) + helper.Fail500(w, r, fmt.Errorf("RequestBody: finalize fields failed: %v", err)) return } @@ -80,7 +87,7 @@ func BodyUploader(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler sft := SavedFileTracker{Request: r} sft.Track("file", fh.LocalPath) if err := sft.Finalize(r.Context()); err != nil { - helper.Fail500(w, r, fmt.Errorf("BodyUploader: finalize failed: %v", err)) + helper.Fail500(w, r, fmt.Errorf("RequestBody: finalize failed: %v", err)) return } diff --git a/workhorse/internal/upload/body_uploader_test.go b/workhorse/internal/upload/body_uploader_test.go index aeb366616ca..b3d561ac131 100644 --- a/workhorse/internal/upload/body_uploader_test.go +++ b/workhorse/internal/upload/body_uploader_test.go @@ -24,7 +24,7 @@ const ( fileLen = len(fileContent) ) -func TestBodyUploader(t *testing.T) { +func TestRequestBody(t *testing.T) { testhelper.ConfigureSecret() body := strings.NewReader(fileContent) @@ -38,7 +38,7 @@ func TestBodyUploader(t *testing.T) { require.Equal(t, fileContent, string(uploadEcho)) } -func TestBodyUploaderCustomPreparer(t *testing.T) { +func TestRequestBodyCustomPreparer(t *testing.T) { body := strings.NewReader(fileContent) resp := testUpload(&rails{}, &alwaysLocalPreparer{}, echoProxy(t, fileLen), body) @@ -49,7 +49,7 @@ func TestBodyUploaderCustomPreparer(t *testing.T) { require.Equal(t, fileContent, string(uploadEcho)) } -func TestBodyUploaderCustomVerifier(t *testing.T) { +func TestRequestBodyCustomVerifier(t *testing.T) { body := strings.NewReader(fileContent) verifier := &mockVerifier{} @@ -62,11 +62,11 @@ func TestBodyUploaderCustomVerifier(t *testing.T) { require.True(t, verifier.invoked, "Verifier.Verify not invoked") } -func TestBodyUploaderAuthorizationFailure(t *testing.T) { +func TestRequestBodyAuthorizationFailure(t *testing.T) { testNoProxyInvocation(t, http.StatusUnauthorized, &rails{unauthorized: true}, &alwaysLocalPreparer{}) } -func TestBodyUploaderErrors(t *testing.T) { +func TestRequestBodyErrors(t *testing.T) { tests := []struct { name string preparer *alwaysLocalPreparer @@ -95,7 +95,7 @@ func testUpload(auth PreAuthorizer, preparer Preparer, proxy http.Handler, body req := httptest.NewRequest("POST", "http://example.com/upload", body) w := httptest.NewRecorder() - BodyUploader(auth, proxy, preparer).ServeHTTP(w, req) + RequestBody(auth, proxy, preparer).ServeHTTP(w, req) return w.Result() } diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index b9324ac8b7b..bbabe840ef5 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -6,8 +6,10 @@ import ( "fmt" "io" "io/ioutil" + "mime" "mime/multipart" "net/http" + "net/textproto" "os" "path/filepath" "strings" @@ -95,7 +97,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr return err } - name := p.FormName() + name, filename := parseAndNormalizeContentDisposition(p.Header) + if name == "" { continue } @@ -104,7 +107,7 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr return ErrInjectedClientParam } - if p.FileName() != "" { + if filename != "" { err = rew.handleFilePart(r.Context(), name, p, opts) } else { err = rew.copyPart(r.Context(), name, p) @@ -118,6 +121,13 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr return nil } +func parseAndNormalizeContentDisposition(header textproto.MIMEHeader) (string, string) { + const key = "Content-Disposition" + mediaType, params, _ := mime.ParseMediaType(header.Get(key)) + header.Set(key, mime.FormatMediaType(mediaType, params)) + return params["name"], params["filename"] +} + func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { if rew.filter.Count() >= maxFilesAllowed { return ErrTooManyFilesUploaded diff --git a/workhorse/internal/upload/rewrite_test.go b/workhorse/internal/upload/rewrite_test.go index e3f33a02489..145f62ee910 100644 --- a/workhorse/internal/upload/rewrite_test.go +++ b/workhorse/internal/upload/rewrite_test.go @@ -1,6 +1,7 @@ package upload import ( + "net/textproto" "os" "runtime" "testing" @@ -54,3 +55,83 @@ func TestImageTypeRecongition(t *testing.T) { }) } } + +func TestParseAndNormalizeContentDisposition(t *testing.T) { + tests := []struct { + desc string + header string + name string + filename string + sanitizedHeader string + }{ + { + desc: "without content disposition", + header: "", + name: "", + filename: "", + sanitizedHeader: "", + }, { + desc: "content disposition without filename", + header: `form-data; name="filename"`, + name: "filename", + filename: "", + sanitizedHeader: `form-data; name=filename`, + }, { + desc: "with filename", + header: `form-data; name="file"; filename=foobar`, + name: "file", + filename: "foobar", + sanitizedHeader: `form-data; filename=foobar; name=file`, + }, { + desc: "with filename*", + header: `form-data; name="file"; filename*=UTF-8''bar`, + name: "file", + filename: "bar", + sanitizedHeader: `form-data; filename=bar; name=file`, + }, { + desc: "filename and filename*", + header: `form-data; name="file"; filename=foobar; filename*=UTF-8''bar`, + name: "file", + filename: "bar", + sanitizedHeader: `form-data; filename=bar; name=file`, + }, { + desc: "with empty filename", + header: `form-data; name="file"; filename=""`, + name: "file", + filename: "", + sanitizedHeader: `form-data; filename=""; name=file`, + }, { + desc: "with complex filename*", + header: `form-data; name="file"; filename*=UTF-8''viel%20Spa%C3%9F`, + name: "file", + filename: "viel Spaß", + sanitizedHeader: `form-data; filename*=utf-8''viel%20Spa%C3%9F; name=file`, + }, { + desc: "with unsupported charset", + header: `form-data; name="file"; filename*=UTF-16''bar`, + name: "file", + filename: "", + sanitizedHeader: `form-data; name=file`, + }, { + desc: "with filename and filename* with unsupported charset", + header: `form-data; name="file"; filename=foobar; filename*=UTF-16''bar`, + name: "file", + filename: "foobar", + sanitizedHeader: `form-data; filename=foobar; name=file`, + }, + } + + for _, testCase := range tests { + t.Run(testCase.desc, func(t *testing.T) { + h := make(textproto.MIMEHeader) + h.Set("Content-Disposition", testCase.header) + h.Set("Content-Type", "application/octet-stream") + + name, filename := parseAndNormalizeContentDisposition(h) + + require.Equal(t, testCase.name, name) + require.Equal(t, testCase.filename, filename) + require.Equal(t, testCase.sanitizedHeader, h.Get("Content-Disposition")) + }) + } +} diff --git a/workhorse/internal/upload/skip_rails_authorizer.go b/workhorse/internal/upload/skip_rails_authorizer.go index c8055a673eb..e74048fb6e3 100644 --- a/workhorse/internal/upload/skip_rails_authorizer.go +++ b/workhorse/internal/upload/skip_rails_authorizer.go @@ -6,15 +6,15 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" ) -// SkipRailsAuthorizer implements a fake PreAuthorizer that do not calls rails API and -// authorize each call as a local only upload to TempPath +// 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 the temporary path for a local only upload + // TempPath is a directory where workhorse can store files that can later + // be accessed by gitlab-rails. TempPath string } -// PreAuthorizeHandler implements PreAuthorizer. It always grant the upload. -// The fake API response contains only TempPath 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}) diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index b408d260379..1806e7563ce 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -15,7 +15,8 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/zipartifacts" ) -// These methods are allowed to have thread-unsafe implementations. +// MultipartFormProcessor abstracts away implementation differences +// between generic MIME multipart file uploads and CI artifact uploads. type MultipartFormProcessor interface { ProcessFile(ctx context.Context, formName string, file *filestore.FileHandler, writer *multipart.Writer) error ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error @@ -24,7 +25,10 @@ type MultipartFormProcessor interface { Count() int } -func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) { +// InterceptMultipartFiles is the core of the implementation of +// Multipart. Because it is also used for CI artifact uploads it is a +// public function. +func InterceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) { var body bytes.Buffer writer := multipart.NewWriter(&body) defer writer.Close() diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 7c22b3b4e28..f262bf94b08 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -1,13 +1,16 @@ package upload import ( + "bufio" "bytes" "context" "fmt" + "io" "io/ioutil" "mime/multipart" "net/http" "net/http/httptest" + "net/textproto" "os" "regexp" "strconv" @@ -75,7 +78,7 @@ func TestUploadHandlerForwardingRawData(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, handler, apiResponse, nil, opts) + InterceptMultipartFiles(response, httpRequest, handler, apiResponse, nil, opts) require.Equal(t, 202, response.Code) require.Equal(t, "RESPONSE", response.Body.String(), "response body") @@ -146,7 +149,7 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, 202, response.Code) cancel() // this will trigger an async cleanup @@ -215,7 +218,7 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, test.response, response.Code) cancel() // this will trigger an async cleanup @@ -245,7 +248,7 @@ func TestUploadProcessingField(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, 500, response.Code) } @@ -276,7 +279,7 @@ func TestUploadingMultipleFiles(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, 400, response.Code) require.Equal(t, "upload request contains more than 10 files\n", response.Body.String()) @@ -332,7 +335,7 @@ func TestUploadProcessingFile(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, 200, response.Code) }) @@ -378,12 +381,95 @@ func TestInvalidFileNames(t *testing.T) { opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts) + InterceptMultipartFiles(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts) require.Equal(t, testCase.code, response.Code) require.Equal(t, testCase.expectedPrefix, opts.TempFilePrefix) } } +func TestContentDispositionRewrite(t *testing.T) { + testhelper.ConfigureSecret() + + tempPath, err := ioutil.TempDir("", "uploads") + require.NoError(t, err) + defer os.RemoveAll(tempPath) + + tests := []struct { + desc string + header string + code int + sanitizedHeader string + }{ + { + desc: "with name", + header: `form-data; name="foo"`, + code: 200, + sanitizedHeader: `form-data; name=foo`, + }, + { + desc: "with name and name*", + header: `form-data; name="foo"; name*=UTF-8''bar`, + code: 200, + sanitizedHeader: `form-data; name=bar`, + }, + { + desc: "with name and invalid name*", + header: `form-data; name="foo"; name*=UTF-16''bar`, + code: 200, + sanitizedHeader: `form-data; name=foo`, + }, + } + + for _, testCase := range tests { + t.Run(testCase.desc, func(t *testing.T) { + h := make(textproto.MIMEHeader) + h.Set("Content-Disposition", testCase.header) + h.Set("Content-Type", "application/octet-stream") + + buffer := &bytes.Buffer{} + writer := multipart.NewWriter(buffer) + file, err := writer.CreatePart(h) + require.NoError(t, err) + fmt.Fprint(file, "test") + writer.Close() + + httpRequest := httptest.NewRequest("POST", "/example", buffer) + httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) + + var upstreamRequestBuffer bytes.Buffer + customHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + r.Write(&upstreamRequestBuffer) + }) + + response := httptest.NewRecorder() + apiResponse := &api.Response{TempPath: tempPath} + preparer := &DefaultPreparer{} + opts, _, err := preparer.Prepare(apiResponse) + require.NoError(t, err) + + InterceptMultipartFiles(response, httpRequest, customHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts) + + upstreamRequest, err := http.ReadRequest(bufio.NewReader(&upstreamRequestBuffer)) + require.NoError(t, err) + + reader, err := upstreamRequest.MultipartReader() + require.NoError(t, err) + + for i := 0; ; i++ { + p, err := reader.NextPart() + if err == io.EOF { + require.Equal(t, i, 1) + break + } + require.NoError(t, err) + require.Equal(t, testCase.sanitizedHeader, p.Header.Get("Content-Disposition")) + } + + require.Equal(t, testCase.code, response.Code) + }) + } +} + func TestUploadHandlerRemovingExif(t *testing.T) { content, err := ioutil.ReadFile("exif/testdata/sample_exif.jpg") require.NoError(t, err) @@ -484,7 +570,7 @@ func runUploadTest(t *testing.T, image []byte, filename string, httpCode int, ts opts, _, err := preparer.Prepare(apiResponse) require.NoError(t, err) - HandleFileUploads(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) + InterceptMultipartFiles(response, httpRequest, handler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, httpCode, response.Code) } @@ -495,15 +581,9 @@ func newProxy(url string) *proxy.Proxy { func waitUntilDeleted(t *testing.T, path string) { var err error - - // Poll because the file removal is async - for i := 0; i < 100; i++ { + require.Eventually(t, func() bool { _, err = os.Stat(path) - if err != nil { - break - } - time.Sleep(100 * time.Millisecond) - } - + return err != nil + }, 10*time.Second, 10*time.Millisecond) require.True(t, os.IsNotExist(err), "expected the file to be deleted") } diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index 06160702f84..b8089865ffe 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -228,17 +228,17 @@ func configureRoutes(u *upstream) { preparers := createUploadPreparers(u.Config) uploadPath := path.Join(u.DocumentRoot, "uploads/tmp") - uploadAccelerateProxy := upload.Accelerate(&upload.SkipRailsAuthorizer{TempPath: uploadPath}, proxy, preparers.uploads) - ciAPIProxyQueue := queueing.QueueRequests("ci_api_job_requests", uploadAccelerateProxy, u.APILimit, u.APIQueueLimit, u.APIQueueTimeout) + tempfileMultipartProxy := upload.Multipart(&upload.SkipRailsAuthorizer{TempPath: uploadPath}, proxy, preparers.uploads) + ciAPIProxyQueue := queueing.QueueRequests("ci_api_job_requests", tempfileMultipartProxy, u.APILimit, u.APIQueueLimit, u.APIQueueTimeout) ciAPILongPolling := builds.RegisterHandler(ciAPIProxyQueue, redis.WatchKey, u.APICILongPollingDuration) - dependencyProxyInjector.SetUploadHandler(upload.BodyUploader(api, signingProxy, preparers.packages)) + dependencyProxyInjector.SetUploadHandler(upload.RequestBody(api, signingProxy, preparers.packages)) // Serve static files or forward the requests defaultUpstream := static.ServeExisting( u.URLPrefix, staticpages.CacheDisabled, - static.DeployPage(static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatHTML, uploadAccelerateProxy)), + static.DeployPage(static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatHTML, tempfileMultipartProxy)), ) probeUpstream := static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatJSON, proxy) healthUpstream := static.ErrorPagesUnless(u.DevelopmentMode, staticpages.ErrorFormatText, proxy) @@ -248,7 +248,7 @@ func configureRoutes(u *upstream) { u.route("GET", gitProjectPattern+`info/refs\z`, git.GetInfoRefsHandler(api)), u.route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.UploadPack(api)), withMatcher(isContentType("application/x-git-upload-pack-request"))), u.route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.ReceivePack(api)), withMatcher(isContentType("application/x-git-receive-pack-request"))), - u.route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, signingProxy, preparers.lfs), withMatcher(isContentType("application/octet-stream"))), + u.route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, upload.RequestBody(api, signingProxy, preparers.lfs), withMatcher(isContentType("application/octet-stream"))), // CI Artifacts u.route("POST", apiPattern+`v4/jobs/[0-9]+/artifacts\z`, contentEncodingHandler(artifacts.UploadArtifacts(api, signingProxy, preparers.artifacts))), @@ -276,56 +276,56 @@ func configureRoutes(u *upstream) { // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56731. // Maven Artifact Repository - u.route("PUT", apiProjectPattern+`packages/maven/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/maven/`, upload.RequestBody(api, signingProxy, preparers.packages)), // Conan Artifact Repository - u.route("PUT", apiPattern+`v4/packages/conan/`, upload.BodyUploader(api, signingProxy, preparers.packages)), - u.route("PUT", apiProjectPattern+`packages/conan/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("PUT", apiPattern+`v4/packages/conan/`, upload.RequestBody(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/conan/`, upload.RequestBody(api, signingProxy, preparers.packages)), // Generic Packages Repository - u.route("PUT", apiProjectPattern+`packages/generic/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/generic/`, upload.RequestBody(api, signingProxy, preparers.packages)), // NuGet Artifact Repository - u.route("PUT", apiProjectPattern+`packages/nuget/`, upload.Accelerate(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/nuget/`, upload.Multipart(api, signingProxy, preparers.packages)), // PyPI Artifact Repository - u.route("POST", apiProjectPattern+`packages/pypi`, upload.Accelerate(api, signingProxy, preparers.packages)), + u.route("POST", apiProjectPattern+`packages/pypi`, upload.Multipart(api, signingProxy, preparers.packages)), // Debian Artifact Repository - u.route("PUT", apiProjectPattern+`packages/debian/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/debian/`, upload.RequestBody(api, signingProxy, preparers.packages)), // Gem Artifact Repository - u.route("POST", apiProjectPattern+`packages/rubygems/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("POST", apiProjectPattern+`packages/rubygems/`, upload.RequestBody(api, signingProxy, preparers.packages)), // Terraform Module Package Repository - u.route("PUT", apiProjectPattern+`packages/terraform/modules/`, upload.BodyUploader(api, signingProxy, preparers.packages)), + u.route("PUT", apiProjectPattern+`packages/terraform/modules/`, upload.RequestBody(api, signingProxy, preparers.packages)), // Helm Artifact Repository - u.route("POST", apiProjectPattern+`packages/helm/api/[^/]+/charts\z`, upload.Accelerate(api, signingProxy, preparers.packages)), + u.route("POST", apiProjectPattern+`packages/helm/api/[^/]+/charts\z`, upload.Multipart(api, signingProxy, preparers.packages)), // We are porting API to disk acceleration // we need to declare each routes until we have fixed all the routes on the rails codebase. // Overall status can be seen at https://gitlab.com/groups/gitlab-org/-/epics/1802#current-status - u.route("POST", apiProjectPattern+`wikis/attachments\z`, uploadAccelerateProxy), - u.route("POST", apiPattern+`graphql\z`, uploadAccelerateProxy), - u.route("POST", apiTopicPattern, uploadAccelerateProxy), - u.route("PUT", apiTopicPattern, uploadAccelerateProxy), - u.route("POST", apiPattern+`v4/groups/import`, upload.Accelerate(api, signingProxy, preparers.uploads)), - u.route("POST", apiPattern+`v4/projects/import`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", apiProjectPattern+`wikis/attachments\z`, tempfileMultipartProxy), + u.route("POST", apiPattern+`graphql\z`, tempfileMultipartProxy), + u.route("POST", apiTopicPattern, tempfileMultipartProxy), + u.route("PUT", apiTopicPattern, tempfileMultipartProxy), + u.route("POST", apiPattern+`v4/groups/import`, upload.Multipart(api, signingProxy, preparers.uploads)), + u.route("POST", apiPattern+`v4/projects/import`, upload.Multipart(api, signingProxy, preparers.uploads)), // Project Import via UI upload acceleration - u.route("POST", importPattern+`gitlab_project`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", importPattern+`gitlab_project`, upload.Multipart(api, signingProxy, preparers.uploads)), // Group Import via UI upload acceleration - u.route("POST", importPattern+`gitlab_group`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", importPattern+`gitlab_group`, upload.Multipart(api, signingProxy, preparers.uploads)), // Metric image upload - u.route("POST", apiProjectPattern+`issues/[0-9]+/metric_images\z`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", apiProjectPattern+`issues/[0-9]+/metric_images\z`, upload.Multipart(api, signingProxy, preparers.uploads)), // Requirements Import via UI upload acceleration - u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, upload.Multipart(api, signingProxy, preparers.uploads)), // Uploads via API - u.route("POST", apiProjectPattern+`uploads\z`, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", apiProjectPattern+`uploads\z`, upload.Multipart(api, signingProxy, preparers.uploads)), // Explicitly proxy API requests u.route("", apiPattern, proxy), @@ -343,9 +343,9 @@ func configureRoutes(u *upstream) { ), // Uploads - u.route("POST", projectPattern+`uploads\z`, upload.Accelerate(api, signingProxy, preparers.uploads)), - u.route("POST", snippetUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)), - u.route("POST", userUploadPattern, upload.Accelerate(api, signingProxy, preparers.uploads)), + u.route("POST", projectPattern+`uploads\z`, upload.Multipart(api, signingProxy, preparers.uploads)), + u.route("POST", snippetUploadPattern, upload.Multipart(api, signingProxy, preparers.uploads)), + u.route("POST", userUploadPattern, upload.Multipart(api, signingProxy, preparers.uploads)), // health checks don't intercept errors and go straight to rails // TODO: We should probably not return a HTML deploy page? @@ -390,6 +390,7 @@ func configureRoutes(u *upstream) { u.route("", "^/api/v4/geo_nodes", defaultUpstream), u.route("", "^/api/v4/geo_replication", defaultUpstream), u.route("", "^/api/v4/geo/proxy_git_ssh", defaultUpstream), + u.route("", "^/api/v4/geo/graphql", defaultUpstream), // Internal API routes u.route("", "^/api/v4/internal", defaultUpstream), |