diff --git a/authorization_test.go b/authorization_test.go index 865546ac90159bf84074a5eef53b2be26aa923ae..565c8f09441e5c0dad37219bc27cf423a3e96811 100644 --- a/authorization_test.go +++ b/authorization_test.go @@ -7,7 +7,7 @@ import ( "regexp" "testing" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" diff --git a/changelogs/unreleased/261-10io-add-signed-params-to-file-uploads.yml b/changelogs/unreleased/261-10io-add-signed-params-to-file-uploads.yml new file mode 100644 index 0000000000000000000000000000000000000000..129e8a1d0637ba07513dd842710805cab1fe8636 --- /dev/null +++ b/changelogs/unreleased/261-10io-add-signed-params-to-file-uploads.yml @@ -0,0 +1,5 @@ +--- +title: Add a signed field on upload requests containing all the workhorse parameters +merge_request: 490 +author: +type: added diff --git a/internal/artifacts/artifacts_upload.go b/internal/artifacts/artifacts_upload.go index 7ca0c535fe546b75bd50b8e0e02530205641160a..adb5e24a7cab8528414bcd61f189a8764b1c236b 100644 --- a/internal/artifacts/artifacts_upload.go +++ b/internal/artifacts/artifacts_upload.go @@ -97,7 +97,12 @@ func (a *artifactsUploadProcessor) ProcessFile(ctx context.Context, formName str } if metadata != nil { - for k, v := range metadata.GitLabFinalizeFields("metadata") { + fields, err := metadata.GitLabFinalizeFields("metadata") + if err != nil { + return fmt.Errorf("finalize metadata field error: %v", err) + } + + for k, v := range fields { writer.WriteField(k, v) } diff --git a/internal/artifacts/artifacts_upload_test.go b/internal/artifacts/artifacts_upload_test.go index 958dbc52f935912cbd9824249ad82707eecfab02..63dba5b6d8be0247fd3dc5f4572d0bc4aa69d003 100644 --- a/internal/artifacts/artifacts_upload_test.go +++ b/internal/artifacts/artifacts_upload_test.go @@ -13,7 +13,7 @@ import ( "os" "testing" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" @@ -157,14 +157,16 @@ func testUploadArtifacts(t *testing.T, contentType, url string, body io.Reader) func TestUploadHandlerAddingMetadata(t *testing.T) { s := setupWithTmpPath(t, "file", func(w http.ResponseWriter, r *http.Request) { - jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), testhelper.ParseJWT) + token, err := jwt.ParseWithClaims(r.Header.Get(upload.RewrittenFieldsHeader), &upload.MultipartClaims{}, testhelper.ParseJWT) require.NoError(t, err) - rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{}) + rewrittenFields := token.Claims.(*upload.MultipartClaims).RewrittenFields require.Equal(t, 2, len(rewrittenFields)) require.Contains(t, rewrittenFields, "file") require.Contains(t, rewrittenFields, "metadata") + require.Contains(t, r.PostForm, "file.gitlab-workhorse-upload") + require.Contains(t, r.PostForm, "metadata.gitlab-workhorse-upload") }, ) defer s.cleanup() diff --git a/internal/filestore/file_handler.go b/internal/filestore/file_handler.go index 7c61032516b955a2e6da5dcefbdbe2492fbc4cd6..55c6591f8a7336c08d768c5060d3d018ee8a4708 100644 --- a/internal/filestore/file_handler.go +++ b/internal/filestore/file_handler.go @@ -9,7 +9,10 @@ import ( "os" "strconv" + "github.com/dgrijalva/jwt-go" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/secret" ) type SizeError error @@ -39,6 +42,11 @@ type FileHandler struct { hashes map[string]string } +type uploadClaims struct { + Upload map[string]string `json:"upload"` + jwt.StandardClaims +} + // SHA256 hash of the handled file func (fh *FileHandler) SHA256() string { return fh.hashes["sha256"] @@ -50,8 +58,10 @@ func (fh *FileHandler) MD5() string { } // GitLabFinalizeFields returns a map with all the fields GitLab Rails needs in order to finalize the upload. -func (fh *FileHandler) GitLabFinalizeFields(prefix string) map[string]string { +func (fh *FileHandler) GitLabFinalizeFields(prefix string) (map[string]string, error) { + // TODO: remove `data` these once rails fully and exclusively support `signedData` (https://gitlab.com/gitlab-org/gitlab-workhorse/-/issues/263) data := make(map[string]string) + signedData := make(map[string]string) key := func(field string) string { if prefix == "" { return field @@ -60,16 +70,30 @@ func (fh *FileHandler) GitLabFinalizeFields(prefix string) map[string]string { return fmt.Sprintf("%s.%s", prefix, field) } - data[key("name")] = fh.Name - data[key("path")] = fh.LocalPath - data[key("remote_url")] = fh.RemoteURL - data[key("remote_id")] = fh.RemoteID - data[key("size")] = strconv.FormatInt(fh.Size, 10) + for k, v := range map[string]string{ + "name": fh.Name, + "path": fh.LocalPath, + "remote_url": fh.RemoteURL, + "remote_id": fh.RemoteID, + "size": strconv.FormatInt(fh.Size, 10), + } { + data[key(k)] = v + signedData[k] = v + } + for hashName, hash := range fh.hashes { data[key(hashName)] = hash + signedData[hashName] = hash + } + + claims := uploadClaims{Upload: signedData, StandardClaims: secret.DefaultClaims} + jwtData, err := secret.JWTTokenString(claims) + if err != nil { + return nil, err } + data[key("gitlab-workhorse-upload")] = jwtData - return data + return data, nil } // SaveFileFromReader persists the provided reader content to all the location specified in opts. A cleanup will be performed once ctx is Done diff --git a/internal/filestore/file_handler_test.go b/internal/filestore/file_handler_test.go index a1a2978b4ac26c06636c2e5ce035a2ffec138360..f75dc2ea6acdb273d7820cf4c23086a9bab32226 100644 --- a/internal/filestore/file_handler_test.go +++ b/internal/filestore/file_handler_test.go @@ -11,11 +11,13 @@ import ( "testing" "time" + "github.com/dgrijalva/jwt-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab-workhorse/internal/objectstore/test" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper" ) func testDeadline() time.Time { @@ -154,6 +156,8 @@ func TestSaveFileFromDiskToLocalPath(t *testing.T) { } func TestSaveFile(t *testing.T) { + testhelper.ConfigureSecret() + type remote int const ( notRemote remote = iota @@ -227,8 +231,8 @@ func TestSaveFile(t *testing.T) { assert.NoError(err) require.NotNil(t, fh) - assert.Equal(opts.RemoteID, fh.RemoteID) - assert.Equal(opts.RemoteURL, fh.RemoteURL) + require.Equal(t, opts.RemoteID, fh.RemoteID) + require.Equal(t, opts.RemoteURL, fh.RemoteURL) if spec.local { assert.NotEmpty(fh.LocalPath, "File not persisted on disk") @@ -236,7 +240,7 @@ func TestSaveFile(t *testing.T) { assert.NoError(err) dir := path.Dir(fh.LocalPath) - assert.Equal(opts.LocalTempPath, dir) + require.Equal(t, opts.LocalTempPath, dir) filename := path.Base(fh.LocalPath) beginsWithPrefix := strings.HasPrefix(filename, opts.TempFilePrefix) assert.True(beginsWithPrefix, fmt.Sprintf("LocalPath filename %q do not begin with TempFilePrefix %q", filename, opts.TempFilePrefix)) @@ -244,34 +248,29 @@ func TestSaveFile(t *testing.T) { assert.Empty(fh.LocalPath, "LocalPath must be empty for non local uploads") } - assert.Equal(test.ObjectSize, fh.Size) - assert.Equal(test.ObjectMD5, fh.MD5()) - assert.Equal(test.ObjectSHA256, fh.SHA256()) + require.Equal(t, test.ObjectSize, fh.Size) + require.Equal(t, test.ObjectMD5, fh.MD5()) + require.Equal(t, test.ObjectSHA256, fh.SHA256()) - assert.Equal(expectedPuts, osStub.PutsCnt(), "ObjectStore PutObject count mismatch") - assert.Equal(0, osStub.DeletesCnt(), "File deleted too early") + require.Equal(t, expectedPuts, osStub.PutsCnt(), "ObjectStore PutObject count mismatch") + require.Equal(t, 0, osStub.DeletesCnt(), "File deleted too early") cancel() // this will trigger an async cleanup assertObjectStoreDeletedAsync(t, expectedDeletes, osStub) assertFileGetsRemovedAsync(t, fh.LocalPath) // checking generated fields - fields := fh.GitLabFinalizeFields("file") - - assert.Equal(fh.Name, fields["file.name"]) - assert.Equal(fh.LocalPath, fields["file.path"]) - assert.Equal(fh.RemoteURL, fields["file.remote_url"]) - assert.Equal(fh.RemoteID, fields["file.remote_id"]) - assert.Equal(strconv.FormatInt(test.ObjectSize, 10), fields["file.size"]) - assert.Equal(test.ObjectMD5, fields["file.md5"]) - assert.Equal(test.ObjectSHA1, fields["file.sha1"]) - assert.Equal(test.ObjectSHA256, fields["file.sha256"]) - assert.Equal(test.ObjectSHA512, fields["file.sha512"]) - if spec.remote == notRemote { - assert.NotContains(fields, "file.etag") - } else { - assert.Contains(fields, "file.etag") - } + fields, err := fh.GitLabFinalizeFields("file") + require.NoError(t, err) + + checkFileHandlerWithFields(t, fh, fields, "file", spec.remote == notRemote) + + token, jwtErr := jwt.ParseWithClaims(fields["file.gitlab-workhorse-upload"], &testhelper.UploadClaims{}, testhelper.ParseJWT) + require.NoError(t, jwtErr) + + uploadFields := token.Claims.(*testhelper.UploadClaims).Upload + + checkFileHandlerWithFields(t, fh, uploadFields, "", spec.remote == notRemote) }) } } @@ -305,3 +304,28 @@ func TestSaveMultipartInBodyFailure(t *testing.T) { require.Error(t, err) assert.EqualError(err, test.MultipartUploadInternalError().Error()) } + +func checkFileHandlerWithFields(t *testing.T, fh *filestore.FileHandler, fields map[string]string, prefix string, remote bool) { + key := func(field string) string { + if prefix == "" { + return field + } + + return fmt.Sprintf("%s.%s", prefix, field) + } + + require.Equal(t, fh.Name, fields[key("name")]) + require.Equal(t, fh.LocalPath, fields[key("path")]) + require.Equal(t, fh.RemoteURL, fields[key("remote_url")]) + require.Equal(t, fh.RemoteID, fields[key("remote_id")]) + require.Equal(t, strconv.FormatInt(test.ObjectSize, 10), fields[key("size")]) + require.Equal(t, test.ObjectMD5, fields[key("md5")]) + require.Equal(t, test.ObjectSHA1, fields[key("sha1")]) + require.Equal(t, test.ObjectSHA256, fields[key("sha256")]) + require.Equal(t, test.ObjectSHA512, fields[key("sha512")]) + if remote { + require.NotContains(t, fields, key("etag")) + } else { + require.Contains(t, fields, key("etag")) + } +} diff --git a/internal/lfs/lfs.go b/internal/lfs/lfs.go index 526c6fa0beda09851d9582d591215aad6bd8a8d8..349af34cffe2aa8bd1f26eb8eb3d5b80c5159f7a 100644 --- a/internal/lfs/lfs.go +++ b/internal/lfs/lfs.go @@ -10,6 +10,7 @@ import ( "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/upload" ) type object struct { @@ -31,7 +32,7 @@ func (l *object) Verify(fh *filestore.FileHandler) error { type uploadPreparer struct{} -func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, filestore.UploadVerifier, error) { +func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, upload.Verifier, error) { opts := filestore.GetOpts(a) opts.TempFilePrefix = a.LfsOid @@ -39,5 +40,5 @@ func (l *uploadPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, file } func PutStore(a *api.API, h http.Handler) http.Handler { - return filestore.BodyUploader(a, h, &uploadPreparer{}) + return upload.BodyUploader(a, h, &uploadPreparer{}) } diff --git a/internal/secret/jwt.go b/internal/secret/jwt.go index 3d19d1dfb47771a89a8cd34f184fcbc4def9b3d8..04335e58f760e2f6e8c0c038f3d5f40fd554ce2e 100644 --- a/internal/secret/jwt.go +++ b/internal/secret/jwt.go @@ -3,7 +3,7 @@ package secret import ( "fmt" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" ) var ( diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 23d04ba412a7d227d21e36dad0914bb4782e7f26..d4c810fa36aea0272238ffc6fa6ffad26bdb4f9a 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -15,7 +15,7 @@ import ( "strings" "testing" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/labkit/log" @@ -191,3 +191,9 @@ func ParseJWT(token *jwt.Token) (interface{}, error) { return secretBytes, nil } + +// UploadClaims represents the JWT claim for upload parameters +type UploadClaims struct { + Upload map[string]string `json:"upload"` + jwt.StandardClaims +} diff --git a/internal/upload/accelerate.go b/internal/upload/accelerate.go index 6bdab27b8dedf31f86dd1c0e0a9cf9dd72a30c0e..453d96f2d24df852045acdbf895e3818d0363793 100644 --- a/internal/upload/accelerate.go +++ b/internal/upload/accelerate.go @@ -3,10 +3,9 @@ package upload import ( "net/http" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" - "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" ) const RewrittenFieldsHeader = "Gitlab-Workhorse-Multipart-Fields" @@ -16,7 +15,7 @@ type MultipartClaims struct { jwt.StandardClaims } -func Accelerate(rails filestore.PreAuthorizer, h http.Handler) http.Handler { +func Accelerate(rails PreAuthorizer, h http.Handler) http.Handler { return rails.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) { s := &SavedFileTracker{Request: r} HandleFileUploads(w, r, h, a, s) diff --git a/internal/filestore/body_uploader.go b/internal/upload/body_uploader.go similarity index 56% rename from internal/filestore/body_uploader.go rename to internal/upload/body_uploader.go index 8f5a35039ae7ba6f836b239220b23f9e957c00ea..35426345f73dbf5c5eb2670f18693900f740e4d5 100644 --- a/internal/filestore/body_uploader.go +++ b/internal/upload/body_uploader.go @@ -1,4 +1,4 @@ -package filestore +package upload import ( "fmt" @@ -8,6 +8,7 @@ import ( "strings" "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" ) @@ -15,29 +16,29 @@ type PreAuthorizer interface { PreAuthorizeHandler(next api.HandleFunc, suffix string) http.Handler } -// UploadVerifier allows to check an upload before sending it to rails -type UploadVerifier interface { +// Verifier allows to check an upload before sending it to rails +type Verifier interface { // Verify can abort the upload returning an error - Verify(handler *FileHandler) error + Verify(handler *filestore.FileHandler) error } -// UploadPreparer allows to customize BodyUploader configuration -type UploadPreparer interface { - // Prepare converts api.Response into a *SaveFileOpts, it can optionally return an UploadVerifier that will be +// Preparer allows to customize BodyUploader configuration +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) (*SaveFileOpts, UploadVerifier, error) + Prepare(a *api.Response) (*filestore.SaveFileOpts, Verifier, error) } type defaultPreparer struct{} -func (s *defaultPreparer) Prepare(a *api.Response) (*SaveFileOpts, UploadVerifier, error) { - return GetOpts(a), nil, nil +func (s *defaultPreparer) Prepare(a *api.Response) (*filestore.SaveFileOpts, Verifier, error) { + return filestore.GetOpts(a), nil, nil } // BodyUploader is an http.Handler that perform a pre authorization call to rails before hijacking the request body and // uploading it. -// Providing an UploadPreparer allows to customize the upload process -func BodyUploader(rails PreAuthorizer, h http.Handler, p UploadPreparer) http.Handler { +// Providing an Preparer allows to customize the upload process +func BodyUploader(rails PreAuthorizer, h http.Handler, p Preparer) http.Handler { if p == nil { p = &defaultPreparer{} } @@ -49,7 +50,7 @@ func BodyUploader(rails PreAuthorizer, h http.Handler, p UploadPreparer) http.Ha return } - fh, err := SaveFileFromReader(r.Context(), r.Body, r.ContentLength, opts) + 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)) return @@ -63,7 +64,13 @@ func BodyUploader(rails PreAuthorizer, h http.Handler, p UploadPreparer) http.Ha } data := url.Values{} - for k, v := range fh.GitLabFinalizeFields("file") { + fields, err := fh.GitLabFinalizeFields("file") + if err != nil { + helper.Fail500(w, r, fmt.Errorf("BodyUploader: finalize fields failed: %v", err)) + return + } + + for k, v := range fields { data.Set(k, v) } @@ -73,6 +80,13 @@ func BodyUploader(rails PreAuthorizer, h http.Handler, p UploadPreparer) http.Ha r.ContentLength = int64(len(body)) r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + 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)) + return + } + // And proxy the request h.ServeHTTP(w, r) }, "/authorize") diff --git a/internal/filestore/body_uploader_test.go b/internal/upload/body_uploader_test.go similarity index 74% rename from internal/filestore/body_uploader_test.go rename to internal/upload/body_uploader_test.go index 65da9c8aa80f9de38d27c84cf2fbc02fde56541f..368323efc06e080b23b1c9e4a370f487c9c25e37 100644 --- a/internal/filestore/body_uploader_test.go +++ b/internal/upload/body_uploader_test.go @@ -1,4 +1,4 @@ -package filestore_test +package upload import ( "fmt" @@ -11,10 +11,12 @@ import ( "strings" "testing" + "github.com/dgrijalva/jwt-go" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-workhorse/internal/api" "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" + "gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper" ) const ( @@ -23,6 +25,8 @@ const ( ) func TestBodyUploader(t *testing.T) { + testhelper.ConfigureSecret() + body := strings.NewReader(fileContent) resp := testUpload(&rails{}, nil, echoProxy(t, fileLen), body) @@ -78,7 +82,7 @@ func TestBodyUploaderErrors(t *testing.T) { } } -func testNoProxyInvocation(t *testing.T, expectedStatus int, auth filestore.PreAuthorizer, preparer filestore.UploadPreparer) { +func testNoProxyInvocation(t *testing.T, expectedStatus int, auth PreAuthorizer, preparer Preparer) { proxy := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { require.Fail(t, "request proxied upstream") }) @@ -87,11 +91,11 @@ func testNoProxyInvocation(t *testing.T, expectedStatus int, auth filestore.PreA require.Equal(t, expectedStatus, resp.StatusCode) } -func testUpload(auth filestore.PreAuthorizer, preparer filestore.UploadPreparer, proxy http.Handler, body io.Reader) *http.Response { +func testUpload(auth PreAuthorizer, preparer Preparer, proxy http.Handler, body io.Reader) *http.Response { req := httptest.NewRequest("POST", "http://example.com/upload", body) w := httptest.NewRecorder() - filestore.BodyUploader(auth, proxy, preparer).ServeHTTP(w, req) + BodyUploader(auth, proxy, preparer).ServeHTTP(w, req) return w.Result() } @@ -112,8 +116,31 @@ func echoProxy(t *testing.T, expectedBodyLength int) http.Handler { require.Contains(r.PostForm, "file.path") require.Contains(r.PostForm, "file.size") + require.Contains(r.PostForm, "file.gitlab-workhorse-upload") require.Equal(strconv.Itoa(expectedBodyLength), r.PostFormValue("file.size")) + token, err := jwt.ParseWithClaims(r.Header.Get(RewrittenFieldsHeader), &MultipartClaims{}, testhelper.ParseJWT) + require.NoError(err, "Wrong JWT header") + + rewrittenFields := token.Claims.(*MultipartClaims).RewrittenFields + if len(rewrittenFields) != 1 || len(rewrittenFields["file"]) == 0 { + t.Fatalf("Unexpected rewritten_fields value: %v", rewrittenFields) + } + + token, jwtErr := jwt.ParseWithClaims(r.PostFormValue("file.gitlab-workhorse-upload"), &testhelper.UploadClaims{}, testhelper.ParseJWT) + require.NoError(jwtErr, "Wrong signed upload fields") + + uploadFields := token.Claims.(*testhelper.UploadClaims).Upload + require.Contains(uploadFields, "name") + require.Contains(uploadFields, "path") + require.Contains(uploadFields, "remote_url") + require.Contains(uploadFields, "remote_id") + require.Contains(uploadFields, "size") + require.Contains(uploadFields, "md5") + require.Contains(uploadFields, "sha1") + require.Contains(uploadFields, "sha256") + require.Contains(uploadFields, "sha512") + path := r.PostFormValue("file.path") uploaded, err := os.Open(path) require.NoError(err, "File not uploaded") @@ -140,11 +167,11 @@ func (r *rails) PreAuthorizeHandler(next api.HandleFunc, _ string) http.Handler } type alwaysLocalPreparer struct { - verifier filestore.UploadVerifier + verifier Verifier prepareError error } -func (a *alwaysLocalPreparer) Prepare(_ *api.Response) (*filestore.SaveFileOpts, filestore.UploadVerifier, error) { +func (a *alwaysLocalPreparer) Prepare(_ *api.Response) (*filestore.SaveFileOpts, Verifier, error) { return filestore.GetOpts(&api.Response{TempPath: os.TempDir()}), a.verifier, a.prepareError } diff --git a/internal/upload/rewrite.go b/internal/upload/rewrite.go index c118839f5e6f2d7230a159b550dd842f3f8be57f..dfa7010ecec15effc0393c40d3fd575bbf1d47bb 100644 --- a/internal/upload/rewrite.go +++ b/internal/upload/rewrite.go @@ -150,7 +150,12 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa } } - for key, value := range fh.GitLabFinalizeFields(name) { + fields, err := fh.GitLabFinalizeFields(name) + if err != nil { + return fmt.Errorf("failed to finalize fields: %v", err) + } + + for key, value := range fields { rew.writer.WriteField(key, value) rew.finalizedFields[key] = true } diff --git a/internal/upload/saved_file_tracker_test.go b/internal/upload/saved_file_tracker_test.go index c49bf4584d4e25a2c5adc7c8cb92b73c3b3b9e26..e5a5e8f23a79bee5ce990c4ce359d447853a0089 100644 --- a/internal/upload/saved_file_tracker_test.go +++ b/internal/upload/saved_file_tracker_test.go @@ -3,7 +3,7 @@ package upload import ( "context" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "net/http" "testing" @@ -29,10 +29,10 @@ func TestSavedFileTracking(t *testing.T) { require.Equal(t, 1, tracker.Count()) tracker.Finalize(ctx) - jwtToken, err := jwt.Parse(r.Header.Get(RewrittenFieldsHeader), testhelper.ParseJWT) + token, err := jwt.ParseWithClaims(r.Header.Get(RewrittenFieldsHeader), &MultipartClaims{}, testhelper.ParseJWT) require.NoError(t, err) - rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{}) + rewrittenFields := token.Claims.(*MultipartClaims).RewrittenFields require.Equal(t, 1, len(rewrittenFields)) require.Contains(t, rewrittenFields, "test") diff --git a/internal/upload/uploads_test.go b/internal/upload/uploads_test.go index 0089a456bace3aa60aba3374ce4c400d30995211..d0faafef93ae06269a620a8bc6191c9d00c4eb73 100644 --- a/internal/upload/uploads_test.go +++ b/internal/upload/uploads_test.go @@ -159,8 +159,8 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { } } - if valueCnt := len(r.MultipartForm.Value); valueCnt != 10 { - t.Fatal("Expected to receive exactly 10 values but got", valueCnt) + if valueCnt := len(r.MultipartForm.Value); valueCnt != 11 { + t.Fatal("Expected to receive exactly 11 values but got", valueCnt) } w.WriteHeader(202) diff --git a/internal/upstream/routes.go b/internal/upstream/routes.go index 541b196570403228b0e40e46e5faa750415b5f33..a131a0cabc3040814a7dee952b320803e2383f03 100644 --- a/internal/upstream/routes.go +++ b/internal/upstream/routes.go @@ -14,7 +14,6 @@ import ( "gitlab.com/gitlab-org/gitlab-workhorse/internal/artifacts" "gitlab.com/gitlab-org/gitlab-workhorse/internal/builds" "gitlab.com/gitlab-org/gitlab-workhorse/internal/channel" - "gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab-workhorse/internal/git" "gitlab.com/gitlab-org/gitlab-workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab-workhorse/internal/lfs" @@ -207,10 +206,10 @@ func (u *upstream) configureRoutes() { route("", ciAPIPattern+`v1/builds/register.json\z`, ciAPILongPolling), // Maven Artifact Repository - route("PUT", apiPattern+`v4/projects/[0-9]+/packages/maven/`, filestore.BodyUploader(api, signingProxy, nil)), + route("PUT", apiPattern+`v4/projects/[0-9]+/packages/maven/`, upload.BodyUploader(api, signingProxy, nil)), // Conan Artifact Repository - route("PUT", apiPattern+`v4/packages/conan/`, filestore.BodyUploader(api, signingProxy, nil)), + route("PUT", apiPattern+`v4/packages/conan/`, upload.BodyUploader(api, signingProxy, nil)), // NuGet Artifact Repository route("PUT", apiPattern+`v4/projects/[0-9]+/packages/nuget/`, upload.Accelerate(api, signingProxy)), diff --git a/upload_test.go b/upload_test.go index 3deb04fad668ae3ae56c742fd2b9af4e6f589ce6..2b4f6a787204d488e7434f5c91cf18c3feabbd3f 100644 --- a/upload_test.go +++ b/upload_test.go @@ -14,7 +14,7 @@ import ( "strings" "testing" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -83,7 +83,7 @@ func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest. if err != nil { t.Fatal(err) } - nValues := 9 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512 for just the upload (no metadata because we are not POSTing a valid zip file) + nValues := 10 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512, gitlab-workhorse-upload for just the upload (no metadata because we are not POSTing a valid zip file) if len(r.MultipartForm.Value) != nValues { t.Errorf("Expected to receive exactly %d values", nValues) } @@ -135,13 +135,27 @@ func TestAcceleratedUpload(t *testing.T) { expectSignedRequest(t, r) } - jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), testhelper.ParseJWT) + token, err := jwt.ParseWithClaims(r.Header.Get(upload.RewrittenFieldsHeader), &upload.MultipartClaims{}, testhelper.ParseJWT) require.NoError(t, err) - rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{}) - if len(rewrittenFields) != 1 || len(rewrittenFields["file"].(string)) == 0 { + rewrittenFields := token.Claims.(*upload.MultipartClaims).RewrittenFields + if len(rewrittenFields) != 1 || len(rewrittenFields["file"]) == 0 { t.Fatalf("Unexpected rewritten_fields value: %v", rewrittenFields) } + + token, jwtErr := jwt.ParseWithClaims(r.PostFormValue("file.gitlab-workhorse-upload"), &testhelper.UploadClaims{}, testhelper.ParseJWT) + require.NoError(t, jwtErr) + + uploadFields := token.Claims.(*testhelper.UploadClaims).Upload + require.Contains(t, uploadFields, "name") + require.Contains(t, uploadFields, "path") + require.Contains(t, uploadFields, "remote_url") + require.Contains(t, uploadFields, "remote_id") + require.Contains(t, uploadFields, "size") + require.Contains(t, uploadFields, "md5") + require.Contains(t, uploadFields, "sha1") + require.Contains(t, uploadFields, "sha256") + require.Contains(t, uploadFields, "sha512") }) defer ts.Close()