Merge branch 'security-reject-unkhown-methods' into 'master'

Reject unknown HTTP methods

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

changelogs/unreleased/security-reject-unkhown-methods.yml

+---
+title: Reject unknown http methods
+merge_request:
+author:
+type: security

internal/rejectmethods/middleware.go

+package rejectmethods
+
+import (
+	"net/http"
+
+	"github.com/prometheus/client_golang/prometheus"
+)
+
+var acceptedMethods = map[string]bool{
+	http.MethodGet:     true,
+	http.MethodHead:    true,
+	http.MethodPost:    true,
+	http.MethodPut:     true,
+	http.MethodPatch:   true,
+	http.MethodDelete:  true,
+	http.MethodConnect: true,
+	http.MethodOptions: true,
+	http.MethodTrace:   true,
+}
+
+var rejectedRequestsCount = prometheus.NewCounter(
+	prometheus.CounterOpts{
+		Name: "gitlab_workhorse_unknown_method_rejected_requests",
+		Help: "The number of requests with unknown HTTP method which were rejected",
+	},
+)
+
+// NewMiddleware returns middleware which rejects all unknown http methods
+func NewMiddleware(handler http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if acceptedMethods[r.Method] {
+			handler.ServeHTTP(w, r)
+		} else {
+			rejectedRequestsCount.Inc()
+			http.Error(w, http.StatusText(http.StatusMethodNotAllowed), http.StatusMethodNotAllowed)
+		}
+	})
+}

internal/rejectmethods/middleware_test.go

+package rejectmethods
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewMiddleware(t *testing.T) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		io.WriteString(w, "OK\n")
+	})
+
+	middleware := NewMiddleware(handler)
+
+	acceptedMethods := []string{"GET", "HEAD", "POST", "PUT", "PATCH", "CONNECT", "OPTIONS", "TRACE"}
+	for _, method := range acceptedMethods {
+		t.Run(method, func(t *testing.T) {
+			tmpRequest, _ := http.NewRequest(method, "/", nil)
+			recorder := httptest.NewRecorder()
+
+			middleware.ServeHTTP(recorder, tmpRequest)
+
+			result := recorder.Result()
+
+			require.Equal(t, http.StatusOK, result.StatusCode)
+		})
+	}
+
+	t.Run("UNKNOWN", func(t *testing.T) {
+		tmpRequest, _ := http.NewRequest("UNKNOWN", "/", nil)
+		recorder := httptest.NewRecorder()
+
+		middleware.ServeHTTP(recorder, tmpRequest)
+
+		result := recorder.Result()
+
+		require.Equal(t, http.StatusMethodNotAllowed, result.StatusCode)
+	})
+}

internal/upstream/upstream.go

@@ -17,6 +17,7 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/rejectmethods"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upload"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream/roundtripper"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/urlprefix"
@@ -63,6 +64,8 @@ func NewUpstream(cfg config.Config, accessLogger *logrus.Logger) http.Handler {
 	}
 
 	handler := correlation.InjectCorrelationID(&up, correlationOpts...)
+	// TODO: move to LabKit https://gitlab.com/gitlab-org/gitlab-workhorse/-/issues/339
+	handler = rejectmethods.NewMiddleware(handler)
 	return handler
 }
 

main_test.go

@@ -642,6 +642,24 @@ func TestPropagateCorrelationIdHeader(t *testing.T) {
 	}
 }
 
+func TestRejectUnknownMethod(t *testing.T) {
+	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+	})
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	req, err := http.NewRequest("UNKNOWN", ws.URL+"/api/v3/projects/123/repository/not/special", nil)
+	require.NoError(t, err)
+
+	resp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+
+	require.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode)
+}
+
 func setupStaticFile(fpath, content string) error {
 	return setupStaticFileHelper(fpath, content, testDocumentRoot)
 }