Sign artifact multipart fields in Workhorse

This adds the `Gitlab-Workhorse-Multipart-Fields` HTTP header, which
contains a list of signed multipart keys, for the CI artifacts upload
endpoints. This is already done for multipart attachments but was
not done for the the CI artifacts case.

Without this header, Rails can't guarantee that the file attachments
were validated by Workhorse.

This is the Workhorse part of the solution for
https://gitlab.com/gitlab-org/gitlab/-/issues/213139. This needs to be
used by Rails:
https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/403

CHANGELOG

 # Changelog for gitlab-workhorse
 
+v 8.30.1
+
+- Sign artifact multipart fields in Workhorse
+
 v 8.30.0
 
 - Proxy ActionCable websocket connection !454
@@ -21,6 +25,14 @@ v 8.26.0
 
 - Add route for project imports direct upload via UI !470
 
+v 8.25.2
+
+- Sign artifact multipart fields in Workhorse
+
+v 8.25.1
+
+- Reject parameters that override upload fields
+
 v 8.25.0
 
 - Add route for project imports direct upload !459
@@ -38,10 +50,26 @@ v 8.22.0
 - Bump the version of golang.org/x/sys !456
 - Add friendly development error page for 502 !453
 
+v 8.21.2
+
+- Sign artifact multipart fields in Workhorse
+
+v 8.21.1
+
+- Reject parameters that override upload fields
+
 v 8.21.0
 
 - Add route for group imports direct upload !455
 
+v 8.20.2
+
+- Sign artifact multipart fields in Workhorse
+
+v 8.20.1
+
+- Reject parameters that override upload fields
+
 v 8.20.0
 
 - Sign file upload requests modified by workhorse

internal/artifacts/artifacts_upload.go

@@ -18,8 +18,9 @@ import (
 )
 
 type artifactsUploadProcessor struct {
-	opts   *filestore.SaveFileOpts
-	stored bool
+	opts *filestore.SaveFileOpts
+
+	upload.SavedFileTracker
 }
 
 func (a *artifactsUploadProcessor) generateMetadataFromZip(ctx context.Context, file *filestore.FileHandler) (*filestore.FileHandler, error) {
@@ -79,10 +80,10 @@ func (a *artifactsUploadProcessor) ProcessFile(ctx context.Context, formName str
 	if formName != "file" {
 		return fmt.Errorf("invalid form field: %q", formName)
 	}
-	if a.stored {
+	if a.Count() > 0 {
 		return fmt.Errorf("artifacts request contains more than one file")
 	}
-	a.stored = true
+	a.Track(formName, file.LocalPath)
 
 	select {
 	case <-ctx.Done():
@@ -99,26 +100,20 @@ func (a *artifactsUploadProcessor) ProcessFile(ctx context.Context, formName str
 			for k, v := range metadata.GitLabFinalizeFields("metadata") {
 				writer.WriteField(k, v)
 			}
+
+			a.Track("metadata", metadata.LocalPath)
 		}
 	}
 	return nil
 }
 
-func (a *artifactsUploadProcessor) ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error {
-	return nil
-}
-
-func (a *artifactsUploadProcessor) Finalize(ctx context.Context) error {
-	return nil
-}
-
 func (a *artifactsUploadProcessor) Name() string {
 	return "artifacts"
 }
 
 func UploadArtifacts(myAPI *api.API, h http.Handler) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		mg := &artifactsUploadProcessor{opts: filestore.GetOpts(a)}
+		mg := &artifactsUploadProcessor{opts: filestore.GetOpts(a), SavedFileTracker: upload.SavedFileTracker{Request: r}}
 
 		upload.HandleFileUploads(w, r, h, a, mg)
 	}, "/authorize")

internal/artifacts/artifacts_upload_test.go

@@ -14,13 +14,18 @@ import (
 	"os"
 	"testing"
 
+	jwt "github.com/dgrijalva/jwt-go"
+
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/proxy"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/zipartifacts"
+
+	"github.com/stretchr/testify/require"
 )
 
 const (
@@ -128,7 +133,18 @@ func TestUploadHandlerAddingMetadata(t *testing.T) {
 	}
 	defer os.RemoveAll(tempPath)
 
-	ts := testArtifactsUploadServer(t, api.Response{TempPath: tempPath}, nil)
+	ts := testArtifactsUploadServer(t, api.Response{TempPath: tempPath},
+		func(w http.ResponseWriter, r *http.Request) {
+			jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), testhelper.ParseJWT)
+			require.NoError(t, err)
+
+			rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{})
+			require.Equal(t, 2, len(rewrittenFields))
+
+			require.Contains(t, rewrittenFields, "file")
+			require.Contains(t, rewrittenFields, "metadata")
+		},
+	)
 	defer ts.Close()
 
 	var buffer bytes.Buffer

internal/testhelper/testhelper.go

@@ -15,6 +15,8 @@ import (
 	"strings"
 	"testing"
 
+	jwt "github.com/dgrijalva/jwt-go"
+
 	"gitlab.com/gitlab-org/labkit/log"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
@@ -174,3 +176,18 @@ func LoadFile(t *testing.T, filePath string) string {
 	}
 	return string(content)
 }
+
+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"])
+	}
+
+	ConfigureSecret()
+	secretBytes, err := secret.Bytes()
+	if err != nil {
+		return nil, fmt.Errorf("read secret from file: %v", err)
+	}
+
+	return secretBytes, nil
+}

internal/upload/accelerate.go

 package upload
 
 import (
-	"context"
-	"fmt"
-	"mime/multipart"
 	"net/http"
 
 	jwt "github.com/dgrijalva/jwt-go"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
 )
 
 const RewrittenFieldsHeader = "Gitlab-Workhorse-Multipart-Fields"
 
-type savedFileTracker struct {
-	request         *http.Request
-	rewrittenFields map[string]string
-}
-
 type MultipartClaims struct {
 	RewrittenFields map[string]string `json:"rewritten_fields"`
 	jwt.StandardClaims
@@ -27,38 +18,7 @@ type MultipartClaims struct {
 
 func Accelerate(rails filestore.PreAuthorizer, h http.Handler) http.Handler {
 	return rails.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
-		s := &savedFileTracker{request: r}
+		s := &SavedFileTracker{Request: r}
 		HandleFileUploads(w, r, h, a, s)
 	}, "/authorize")
 }
-
-func (s *savedFileTracker) ProcessFile(_ context.Context, fieldName string, file *filestore.FileHandler, _ *multipart.Writer) error {
-	if s.rewrittenFields == nil {
-		s.rewrittenFields = make(map[string]string)
-	}
-	s.rewrittenFields[fieldName] = file.LocalPath
-	return nil
-}
-
-func (s *savedFileTracker) ProcessField(_ context.Context, _ string, _ *multipart.Writer) error {
-	return nil
-}
-
-func (s *savedFileTracker) Finalize(_ context.Context) error {
-	if s.rewrittenFields == nil {
-		return nil
-	}
-
-	claims := MultipartClaims{s.rewrittenFields, secret.DefaultClaims}
-	tokenString, err := secret.JWTTokenString(claims)
-	if err != nil {
-		return fmt.Errorf("savedFileTracker.Finalize: %v", err)
-	}
-
-	s.request.Header.Set(RewrittenFieldsHeader, tokenString)
-	return nil
-}
-
-func (a *savedFileTracker) Name() string {
-	return "accelerate"
-}

internal/upload/saved_file_tracker.go

+package upload
+
+import (
+	"context"
+	"fmt"
+	"mime/multipart"
+	"net/http"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/secret"
+)
+
+type SavedFileTracker struct {
+	Request         *http.Request
+	rewrittenFields map[string]string
+}
+
+func (s *SavedFileTracker) Track(fieldName string, localPath string) {
+	if s.rewrittenFields == nil {
+		s.rewrittenFields = make(map[string]string)
+	}
+	s.rewrittenFields[fieldName] = localPath
+}
+
+func (s *SavedFileTracker) Count() int {
+	return len(s.rewrittenFields)
+}
+
+func (s *SavedFileTracker) ProcessFile(_ context.Context, fieldName string, file *filestore.FileHandler, _ *multipart.Writer) error {
+	s.Track(fieldName, file.LocalPath)
+	return nil
+}
+
+func (s *SavedFileTracker) ProcessField(_ context.Context, _ string, _ *multipart.Writer) error {
+	return nil
+}
+
+func (s *SavedFileTracker) Finalize(_ context.Context) error {
+	if s.rewrittenFields == nil {
+		return nil
+	}
+
+	claims := MultipartClaims{RewrittenFields: s.rewrittenFields, StandardClaims: secret.DefaultClaims}
+	tokenString, err := secret.JWTTokenString(claims)
+	if err != nil {
+		return fmt.Errorf("savedFileTracker.Finalize: %v", err)
+	}
+
+	s.Request.Header.Set(RewrittenFieldsHeader, tokenString)
+	return nil
+}
+
+func (s *SavedFileTracker) Name() string {
+	return "accelerate"
+}

internal/upload/saved_file_tracker_test.go

+package upload
+
+import (
+	"context"
+
+	jwt "github.com/dgrijalva/jwt-go"
+
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/filestore"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
+)
+
+func TestSavedFileTracking(t *testing.T) {
+	testhelper.ConfigureSecret()
+
+	r, err := http.NewRequest("PUT", "/url/path", nil)
+	require.NoError(t, err)
+
+	tracker := SavedFileTracker{Request: r}
+	require.Equal(t, "accelerate", tracker.Name())
+
+	file := &filestore.FileHandler{}
+	ctx := context.Background()
+	tracker.ProcessFile(ctx, "test", file, nil)
+	require.Equal(t, 1, tracker.Count())
+
+	tracker.Finalize(ctx)
+	jwtToken, err := jwt.Parse(r.Header.Get(RewrittenFieldsHeader), testhelper.ParseJWT)
+	require.NoError(t, err)
+
+	rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{})
+	require.Equal(t, 1, len(rewrittenFields))
+
+	require.Contains(t, rewrittenFields, "test")
+}

internal/upload/uploads_test.go

@@ -390,7 +390,7 @@ func TestInvalidFileNames(t *testing.T) {
 		httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
 
 		response := httptest.NewRecorder()
-		HandleFileUploads(response, httpRequest, nilHandler, &api.Response{TempPath: tempPath}, &savedFileTracker{request: httpRequest})
+		HandleFileUploads(response, httpRequest, nilHandler, &api.Response{TempPath: tempPath}, &SavedFileTracker{Request: httpRequest})
 		testhelper.AssertResponseCode(t, response, testCase.code)
 	}
 }

upload_test.go

@@ -63,7 +63,7 @@ func TestArtifactsUpload(t *testing.T) {
 func expectSignedRequest(t *testing.T, r *http.Request) {
 	t.Helper()
 
-	_, err := jwt.Parse(r.Header.Get(secret.RequestHeader), parseJWT)
+	_, err := jwt.Parse(r.Header.Get(secret.RequestHeader), testhelper.ParseJWT)
 	require.NoError(t, err)
 }
 
@@ -109,21 +109,6 @@ func signedUploadTestServer(t *testing.T, extraTests func(r *http.Request)) *htt
 	})
 }
 
-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 {
-		return nil, fmt.Errorf("read secret from file: %v", err)
-	}
-
-	return secretBytes, nil
-}
-
 func TestAcceleratedUpload(t *testing.T) {
 	tests := []struct {
 		method             string
@@ -150,7 +135,7 @@ func TestAcceleratedUpload(t *testing.T) {
 						expectSignedRequest(t, r)
 					}
 
-					jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), parseJWT)
+					jwtToken, err := jwt.Parse(r.Header.Get(upload.RewrittenFieldsHeader), testhelper.ParseJWT)
 					require.NoError(t, err)
 
 					rewrittenFields := jwtToken.Claims.(jwt.MapClaims)["rewritten_fields"].(map[string]interface{})