Merge branch 'fix/credentials-leakage' into 'master'

Sanitize URLs before logging them

Closes #1052

See merge request gitlab-org/gitaly!624

4 files changed, 228 insertions, 1 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8f0aff489..1a24afd61 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Gitaly changelog
 
+UNRELEASED
+
+- Sanitize URLs before logging them
+  https://gitlab.com/gitlab-org/gitaly/merge_requests/624
+
 v0.89.0
 
 - Report original exceptions to Sentry instead of wrapped ones by the exception bridge
diff --git a/internal/logsanitizer/url.go b/internal/logsanitizer/url.go
new file mode 100644
index 000000000..ecebe6bdb
--- /dev/null
+++ b/internal/logsanitizer/url.go
@@ -0,0 +1,83 @@
+package logsanitizer
+
+import (
+	"regexp"
+
+	"github.com/sirupsen/logrus"
+)
+
+// Pattern taken from Regular Expressions Cookbook, slightly modified though
+//                                    |Scheme                |User                         |Named/IPv4 host|IPv6+ host
+var hostPattern = regexp.MustCompile(`(?i)([a-z][a-z0-9+\-.]*://)([a-z0-9\-._~%!$&'()*+,;=:]+@)([a-z0-9\-._~%]+|\[[a-z0-9\-._~%!$&'()*+,;=:]+\])`)
+
+// URLSanitizerHook stores which gRPC methods to perform sanitization for.
+type URLSanitizerHook struct {
+	// gRPC methods that are likely to have URLs to sanitize
+	possibleGrpcMethods map[string]bool
+}
+
+// NewURLSanitizerHook returns a new logrus hook for sanitizing URLs.
+func NewURLSanitizerHook() *URLSanitizerHook {
+	return &URLSanitizerHook{possibleGrpcMethods: make(map[string]bool)}
+}
+
+// AddPossibleGrpcMethod adds method names that we should sanitize URLs from their logs.
+func (hook *URLSanitizerHook) AddPossibleGrpcMethod(methods ...string) {
+	for _, method := range methods {
+		hook.possibleGrpcMethods[method] = true
+	}
+}
+
+// Fire is called by logrus.
+func (hook *URLSanitizerHook) Fire(entry *logrus.Entry) error {
+	mth, ok := entry.Data["grpc.method"]
+	if !ok {
+		return nil
+	}
+
+	mthStr, ok := mth.(string)
+	if !ok || !hook.possibleGrpcMethods[mthStr] {
+		return nil
+	}
+
+	if _, ok := entry.Data["args"]; ok {
+		sanitizeSpawnLog(entry)
+	} else if _, ok := entry.Data["error"]; ok {
+		sanitizeErrorLog(entry)
+	} else {
+		entry.Message = sanitizeString(entry.Message)
+	}
+
+	return nil
+}
+
+// Levels is called by logrus.
+func (hook *URLSanitizerHook) Levels() []logrus.Level {
+	return logrus.AllLevels
+}
+
+func sanitizeSpawnLog(entry *logrus.Entry) {
+	args, ok := entry.Data["args"].([]string)
+	if !ok {
+		return
+	}
+
+	for i, arg := range args {
+		args[i] = sanitizeString(arg)
+	}
+
+	entry.Data["args"] = args
+}
+
+func sanitizeErrorLog(entry *logrus.Entry) {
+	err, ok := entry.Data["error"].(error)
+	if !ok {
+		return
+	}
+
+	entry.Data["error"] = sanitizeString(err.Error())
+}
+
+func sanitizeString(str string) string {
+	return hostPattern.ReplaceAllString(str, "$1[FILTERED]@$3$4")
+}
diff --git a/internal/logsanitizer/url_test.go b/internal/logsanitizer/url_test.go
new file mode 100644
index 000000000..b3fc116fd
--- /dev/null
+++ b/internal/logsanitizer/url_test.go
@@ -0,0 +1,128 @@
+package logsanitizer
+
+import (
+	"bytes"
+	"fmt"
+	"io/ioutil"
+	"testing"
+
+	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUrlSanitizerHook(t *testing.T) {
+	outBuf := &bytes.Buffer{}
+
+	urlSanitizer := NewURLSanitizerHook()
+	urlSanitizer.AddPossibleGrpcMethod(
+		"UpdateRemoteMirror",
+		"CreateRepositoryFromURL",
+	)
+
+	logger := log.New()
+	logger.Out = outBuf
+	logger.Hooks.Add(urlSanitizer)
+
+	testCases := []struct {
+		desc           string
+		logFunc        func()
+		expectedString string
+	}{
+		{
+			desc: "with args",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "CreateRepositoryFromURL",
+					"args":        []string{"/usr/bin/git", "clone", "--bare", "--", "https://foo_the_user:hUntEr1@gitlab.com/foo/bar", "/home/git/repositories/foo/bar"},
+				}).Info("spawn")
+			},
+			expectedString: "[/usr/bin/git clone --bare -- https://[FILTERED]@gitlab.com/foo/bar /home/git/repositories/foo/bar]",
+		},
+		{
+			desc: "with error",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "UpdateRemoteMirror",
+					"error":       fmt.Errorf("rpc error: code = Unknown desc = remote: Invalid username or password. fatal: Authentication failed for 'https://foo_the_user:hUntEr1@gitlab.com/foo/bar'"),
+				}).Error("ERROR")
+			},
+			expectedString: "rpc error: code = Unknown desc = remote: Invalid username or password. fatal: Authentication failed for 'https://[FILTERED]@gitlab.com/foo/bar'",
+		},
+		{
+			desc: "with message",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "CreateRepositoryFromURL",
+				}).Info("asked for: https://foo_the_user:hUntEr1@gitlab.com/foo/bar")
+			},
+			expectedString: "asked for: https://[FILTERED]@gitlab.com/foo/bar",
+		},
+		{
+			desc: "with gRPC method not added to the list",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "UserDeleteTag",
+				}).Error("fatal: 'https://foo_the_user:hUntEr1@gitlab.com/foo/bar' is not a valid tag name.")
+			},
+			expectedString: "fatal: 'https://foo_the_user:hUntEr1@gitlab.com/foo/bar' is not a valid tag name.",
+		},
+		{
+			desc: "log with URL that does not require sanitization",
+			logFunc: func() {
+				logger.WithFields(log.Fields{
+					"grpc.method": "CreateRepositoryFromURL",
+				}).Info("asked for: https://gitlab.com/gitlab-org/gitaly")
+			},
+			expectedString: "asked for: https://gitlab.com/gitlab-org/gitaly",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.desc, func(t *testing.T) {
+			testCase.logFunc()
+			logOutput := outBuf.String()
+
+			require.Contains(t, logOutput, testCase.expectedString)
+		})
+	}
+}
+
+func BenchmarkUrlSanitizerWithoutSanitization(b *testing.B) {
+	urlSanitizer := NewURLSanitizerHook()
+
+	logger := log.New()
+	logger.Out = ioutil.Discard
+	logger.Hooks.Add(urlSanitizer)
+
+	benchmarkLogging(logger, b)
+}
+
+func BenchmarkUrlSanitizerWithSanitization(b *testing.B) {
+	urlSanitizer := NewURLSanitizerHook()
+	urlSanitizer.AddPossibleGrpcMethod(
+		"UpdateRemoteMirror",
+		"CreateRepositoryFromURL",
+	)
+
+	logger := log.New()
+	logger.Out = ioutil.Discard
+	logger.Hooks.Add(urlSanitizer)
+
+	benchmarkLogging(logger, b)
+}
+
+func benchmarkLogging(logger *log.Logger, b *testing.B) {
+	for n := 0; n < b.N; n++ {
+		logger.WithFields(log.Fields{
+			"grpc.method": "CreateRepositoryFromURL",
+			"args":        []string{"/usr/bin/git", "clone", "--bare", "--", "https://foo_the_user:hUntEr1@gitlab.com/foo/bar", "/home/git/repositories/foo/bar"},
+		}).Info("spawn")
+		logger.WithFields(log.Fields{
+			"grpc.method": "UpdateRemoteMirror",
+			"error":       fmt.Errorf("rpc error: code = Unknown desc = remote: Invalid username or password. fatal: Authentication failed for 'https://foo_the_user:hUntEr1@gitlab.com/foo/bar'"),
+		}).Error("ERROR")
+		logger.WithFields(log.Fields{
+			"grpc.method": "CreateRepositoryFromURL",
+		}).Info("asked for: https://foo_the_user:hUntEr1@gitlab.com/foo/bar")
+	}
+}
diff --git a/internal/server/server.go b/internal/server/server.go
index 4fa980be4..eb5decb8d 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -4,6 +4,7 @@ import (
 	log "github.com/sirupsen/logrus"
 
 	"gitlab.com/gitlab-org/gitaly/internal/helper/fieldextractors"
+	"gitlab.com/gitlab-org/gitaly/internal/logsanitizer"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/cancelhandler"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/limithandler"
 	"gitlab.com/gitlab-org/gitaly/internal/middleware/metadatahandler"
@@ -24,7 +25,17 @@ import (
 
 // New returns a GRPC server with all Gitaly services and interceptors set up.
 func New(rubyServer *rubyserver.Server) *grpc.Server {
-	logrusEntry := log.NewEntry(log.StandardLogger())
+	logger := log.StandardLogger()
+
+	urlSanitizer := logsanitizer.NewURLSanitizerHook()
+	urlSanitizer.AddPossibleGrpcMethod(
+		"CreateRepositoryFromURL",
+		"FetchRemote",
+		"UpdateRemoteMirror",
+	)
+	logger.Hooks.Add(urlSanitizer)
+
+	logrusEntry := log.NewEntry(logger)
 	grpc_logrus.ReplaceGrpcLogger(logrusEntry)
 
 	ctxTagOpts := []grpc_ctxtags.Option{