Merge branch 'security/gitlab-issue-213139' into 'master'

Sign artifact multipart fields in Workhorse

See merge request gitlab-org/security/gitlab-workhorse!7

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

@@ -19,7 +19,8 @@ import (
 
 type artifactsUploadProcessor struct {
 	opts *filestore.SaveFileOpts
-	stored bool
+
+	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{})