Merge branch 'jc-check-auth-before-limit-handler' into 'master'

Check auth before limit handler

Closes #2815

See merge request gitlab-org/gitaly!2221

7 files changed, 164 insertions, 11 deletions

diff --git a/_support/Makefile.template b/_support/Makefile.template
index adfe5f192..f3beb71d2 100644
--- a/_support/Makefile.template
+++ b/_support/Makefile.template
@@ -14,6 +14,7 @@ INSTALL_DEST_DIR := $(DESTDIR)$(bindir)
 BUNDLE_FLAGS ?= {{ .BundleFlags }}
 ASSEMBLY_ROOT ?= {{ .BuildDir }}/assembly
 BUILD_TAGS := tracer_static tracer_static_jaeger continuous_profiler_stackdriver
+GO_TEST_LD_FLAGS := -X gitlab.com/gitlab-org/gitaly/auth.timestampThreshold=5s
 SHELL = /usr/bin/env bash -eo pipefail
 # These variables control test options and artifacts
 TEST_OPTIONS ?=
@@ -113,7 +114,7 @@ test-go: prepare-tests {{ .GoJunitReport }}
 	@mkdir -p $(TEST_REPORT_DIR)
 	@cd {{ .SourceDir }} && \
 		echo 0 > $(TEST_EXIT) && \
-		(go test $(TEST_OPTIONS) -v -tags "$(BUILD_TAGS)" -count=1 $(TEST_PACKAGES) 2>&1 | tee $(TEST_OUTPUT) || echo $$? > $(TEST_EXIT)) && \
+		(go test $(TEST_OPTIONS) -v -tags "$(BUILD_TAGS)" -ldflags='$(GO_TEST_LD_FLAGS)' -count=1 $(TEST_PACKAGES) 2>&1 | tee $(TEST_OUTPUT) || echo $$? > $(TEST_EXIT)) && \
 		cat $(TEST_OUTPUT) | {{ .GoJunitReport }} > $(TEST_REPORT) && \
 		exit `cat $(TEST_EXIT)`
 
@@ -126,7 +127,7 @@ test-with-proxies: prepare-tests
 .PHONY: test-with-praefect
 test-with-praefect: build prepare-tests
 	@cd {{ .SourceDir }} &&\
-	GITALY_TEST_PRAEFECT_BIN={{ .BuildDir }}/bin/praefect  go test -tags "$(BUILD_TAGS)" -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching
+	GITALY_TEST_PRAEFECT_BIN={{ .BuildDir }}/bin/praefect go test -tags "$(BUILD_TAGS)" -ldflags='$(GO_TEST_LD_FLAGS)' -count=1 {{ join .AllPackages " " }} # count=1 bypasses go 1.10 test caching
 
 .PHONY: race-go
 race-go: TEST_OPTIONS = -race
@@ -229,7 +230,7 @@ cover: prepare-tests
 	@echo "NOTE: make cover does not exit 1 on failure, don't use it to check for tests success!"
 	mkdir -p "{{ .CoverageDir }}"
 	rm -f "{{ .CoverageDir }}/all.merged" "{{ .CoverageDir }}/all.html"
-	@cd {{ .SourceDir }} && go test -coverprofile "{{ .CoverageDir }}/all.merged" {{ join .AllPackages " " }}
+	@cd {{ .SourceDir }} && go test -ldflags='$(GO_TEST_LD_FLAGS)' -coverprofile "{{ .CoverageDir }}/all.merged" {{ join .AllPackages " " }}
 	@cd {{ .SourceDir }} && go tool cover -html  "{{ .CoverageDir }}/all.merged" -o "{{ .CoverageDir }}/all.html"
 	@echo ""
 	@echo "=====> Total test coverage: <====="
diff --git a/auth/extract_test.go b/auth/extract_test.go
index 510fb1790..e002762b3 100644
--- a/auth/extract_test.go
+++ b/auth/extract_test.go
@@ -10,6 +10,16 @@ import (
 )
 
 func TestCheckTokenV2(t *testing.T) {
+	// the tests below had their hmac signatures generated with the default 30s
+	// in our tests, we modify this number with ldflags but this test should continue
+	// to use the 30s number
+
+	defer func(d time.Duration) {
+		timestampThresholdDuration = d
+	}(timestampThresholdDuration)
+
+	timestampThresholdDuration = 30 * time.Second
+
 	targetTime := time.Unix(1535671600, 0)
 	secret := []byte("foo")
 
diff --git a/auth/token.go b/auth/token.go
index dee53227c..12c11740c 100644
--- a/auth/token.go
+++ b/auth/token.go
@@ -16,11 +16,10 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-const (
-	timestampThreshold = 30 * time.Second
-)
+var timestampThresholdDuration time.Duration
 
 var (
+	timestampThreshold = "30s"
 	errUnauthenticated = status.Errorf(codes.Unauthenticated, "authentication required")
 	errDenied          = status.Errorf(codes.PermissionDenied, "permission denied")
 
@@ -33,8 +32,19 @@ var (
 	)
 )
 
+// TimestampThreshold is used by tests
+func TimestampThreshold() time.Duration {
+	return timestampThresholdDuration
+}
+
 func init() {
 	prometheus.MustRegister(authErrors)
+
+	var err error
+	timestampThresholdDuration, err = time.ParseDuration(timestampThreshold)
+	if err != nil {
+		panic(err)
+	}
 }
 
 // AuthInfo contains the authentication information coming from a request
@@ -58,7 +68,7 @@ func CheckToken(ctx context.Context, secret string, targetTime time.Time) error
 	}
 
 	if authInfo.Version == "v2" {
-		if v2HmacInfoValid(authInfo.Message, authInfo.SignedMessage, []byte(secret), targetTime, timestampThreshold) {
+		if v2HmacInfoValid(authInfo.Message, authInfo.SignedMessage, []byte(secret), targetTime, timestampThresholdDuration) {
 			return nil
 		}
 	}
diff --git a/changelogs/unreleased/jc-check-auth-before-limit-handler.yml b/changelogs/unreleased/jc-check-auth-before-limit-handler.yml
new file mode 100644
index 000000000..5dfee95b4
--- /dev/null
+++ b/changelogs/unreleased/jc-check-auth-before-limit-handler.yml
@@ -0,0 +1,5 @@
+---
+title: Check auth before limit handler
+merge_request: 2221
+author:
+type: fixed
diff --git a/internal/server/.gitignore b/internal/server/.gitignore
new file mode 100644
index 000000000..ace1063ab
--- /dev/null
+++ b/internal/server/.gitignore
@@ -0,0 +1 @@
+/testdata
diff --git a/internal/server/auth_test.go b/internal/server/auth_test.go
index 8b43a78e9..737476bb1 100644
--- a/internal/server/auth_test.go
+++ b/internal/server/auth_test.go
@@ -3,15 +3,20 @@ package server
 import (
 	netctx "context"
 	"crypto/x509"
+	"fmt"
 	"io/ioutil"
 	"net"
+	"os"
+	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/config/auth"
+	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -165,8 +170,21 @@ func healthCheck(conn *grpc.ClientConn) error {
 	return err
 }
 
-func runServer(t *testing.T) (*grpc.Server, string) {
-	srv := NewInsecure(nil, config.Config)
+func newOperationClient(t *testing.T, serverSocketPath string) (gitalypb.OperationServiceClient, *grpc.ClientConn) {
+	connOpts := []grpc.DialOption{
+		grpc.WithInsecure(),
+		grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(config.Config.Auth.Token)),
+	}
+	conn, err := grpc.Dial(serverSocketPath, connOpts...)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return gitalypb.NewOperationServiceClient(conn), conn
+}
+
+func runServerWithRuby(t *testing.T, ruby *rubyserver.Server) (*grpc.Server, string) {
+	srv := NewInsecure(ruby, config.Config)
 
 	serverSocketPath := testhelper.GetTemporaryGitalySocketFileName()
 
@@ -177,6 +195,10 @@ func runServer(t *testing.T) (*grpc.Server, string) {
 	return srv, "unix://" + serverSocketPath
 }
 
+func runServer(t *testing.T) (*grpc.Server, string) {
+	return runServerWithRuby(t, nil)
+}
+
 func runSecureServer(t *testing.T) (*grpc.Server, string) {
 	config.Config.TLS = config.TLS{
 		CertPath: "testdata/gitalycert.pem",
@@ -255,3 +277,107 @@ func TestStreamingNoAuth(t *testing.T) {
 
 	testhelper.RequireGrpcError(t, err, codes.Unauthenticated)
 }
+
+func TestMain(m *testing.M) {
+	testhelper.Configure()
+	os.Exit(testMain(m))
+}
+
+func testMain(m *testing.M) int {
+	testhelper.ConfigureGitalyHooksBinary()
+
+	return m.Run()
+}
+
+func TestAuthBeforeLimit(t *testing.T) {
+	defer func(cfg config.Cfg) {
+		config.Config = cfg
+	}(config.Config)
+
+	config.Config.Auth.Token = "abc123"
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	cwd, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	gitlabShellDir := filepath.Join(cwd, "testdata", "gitlab-shell")
+	os.RemoveAll(gitlabShellDir)
+
+	if err := os.MkdirAll(gitlabShellDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	config.Config.GitlabShell.Dir = gitlabShellDir
+
+	url, cleanup := testhelper.SetupAndStartGitlabServer(t, &testhelper.GitlabTestServerOptions{
+		SecretToken:                 "secretToken",
+		GLID:                        testhelper.GlID,
+		GLRepository:                testRepo.GlRepository,
+		PostReceiveCounterDecreased: true,
+		Protocol:                    "web",
+	})
+	defer cleanup()
+
+	config.Config.Concurrency = []config.Concurrency{{
+		RPC:        "/gitaly.OperationService/UserCreateTag",
+		MaxPerRepo: 1,
+	}}
+	config.ConfigureConcurrencyLimits()
+
+	config.Config.Gitlab.URL = url
+	var RubyServer rubyserver.Server
+	if err := RubyServer.Start(); err != nil {
+		t.Fatal(err)
+	}
+	server, serverSocketPath := runServerWithRuby(t, &RubyServer)
+	defer server.Stop()
+
+	client, conn := newOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	targetRevision := "c7fbe50c7c7419d9701eebe64b1fdacc3df5b9dd"
+	require.NoError(t, err)
+
+	inputTagName := "to-be-créated-soon"
+
+	request := &gitalypb.UserCreateTagRequest{
+		Repository:     testRepo,
+		TagName:        []byte(inputTagName),
+		TargetRevision: []byte(targetRevision),
+		User:           testhelper.TestUser,
+		Message:        []byte("a new tag!"),
+	}
+
+	cleanupCustomHook, err := testhelper.WriteCustomHook(testRepoPath, "pre-receive", []byte(fmt.Sprintf(`#!/bin/bash
+sleep %vs
+`, gitalyauth.TimestampThreshold().Seconds())))
+
+	require.NoError(t, err)
+	defer cleanupCustomHook()
+
+	errChan := make(chan error)
+
+	for i := 0; i < 2; i++ {
+		go func() {
+			_, err := client.UserCreateTag(ctx, request)
+			errChan <- err
+		}()
+	}
+
+	timer := time.NewTimer(1 * time.Minute)
+
+	for i := 0; i < 2; i++ {
+		select {
+		case <-timer.C:
+			require.Fail(t, "time limit reached waiting for calls to finish")
+		case err := <-errChan:
+			require.NoError(t, err)
+		}
+	}
+}
diff --git a/internal/server/server.go b/internal/server/server.go
index 2240a4442..ca7962fbf 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -85,8 +85,8 @@ func createNewServer(rubyServer *rubyserver.Server, cfg config.Cfg, secure bool)
 			grpc_logrus.StreamServerInterceptor(logrusEntry),
 			sentryhandler.StreamLogHandler,
 			cancelhandler.Stream, // Should be below LogHandler
-			lh.StreamInterceptor(),
 			auth.StreamServerInterceptor(config.Config.Auth),
+			lh.StreamInterceptor(), // Should be below auth handler to prevent v2 hmac tokens from timing out while queued
 			grpctracing.StreamServerTracingInterceptor(),
 			cache.StreamInvalidator(diskcache.LeaseKeyer{}, protoregistry.GitalyProtoPreregistered),
 			// Panic handler should remain last so that application panics will be
@@ -101,8 +101,8 @@ func createNewServer(rubyServer *rubyserver.Server, cfg config.Cfg, secure bool)
 			grpc_logrus.UnaryServerInterceptor(logrusEntry),
 			sentryhandler.UnaryLogHandler,
 			cancelhandler.Unary, // Should be below LogHandler
-			lh.UnaryInterceptor(),
 			auth.UnaryServerInterceptor(config.Config.Auth),
+			lh.UnaryInterceptor(), // Should be below auth handler to prevent v2 hmac tokens from timing out while queued
 			grpctracing.UnaryServerTracingInterceptor(),
 			cache.UnaryInvalidator(diskcache.LeaseKeyer{}, protoregistry.GitalyProtoPreregistered),
 			// Panic handler should remain last so that application panics will be