From fe1b9ed7e70f2e1467547c628b8d9197742901db Mon Sep 17 00:00:00 2001
From: kennytm <kennytm@gmail.com>
Date: Thu, 21 Nov 2024 19:36:54 +0800
Subject: [PATCH] br: redact secret strings when logging arguments (#57593)

close pingcap/tidb#57585
---
 br/pkg/task/common.go      | 11 ++++--
 br/pkg/task/common_test.go | 68 ++++++++++++++++++++++++++++++++++----
 2 files changed, 71 insertions(+), 8 deletions(-)

diff --git a/br/pkg/task/common.go b/br/pkg/task/common.go
index 08841700936e8..1813741634609 100644
--- a/br/pkg/task/common.go
+++ b/br/pkg/task/common.go
@@ -895,7 +895,8 @@ func ReadBackupMeta(
 // flagToZapField checks whether this flag can be logged,
 // if need to log, return its zap field. Or return a field with hidden value.
 func flagToZapField(f *pflag.Flag) zap.Field {
-	if f.Name == flagStorage {
+	switch f.Name {
+	case flagStorage, FlagStreamFullBackupStorage:
 		hiddenQuery, err := url.Parse(f.Value.String())
 		if err != nil {
 			return zap.String(f.Name, "<invalid URI>")
@@ -903,8 +904,14 @@ func flagToZapField(f *pflag.Flag) zap.Field {
 		// hide all query here.
 		hiddenQuery.RawQuery = ""
 		return zap.Stringer(f.Name, hiddenQuery)
+	case flagFullBackupCipherKey, flagLogBackupCipherKey, "azblob.encryption-key":
+		return zap.String(f.Name, "<redacted>")
+	case flagMasterKeyConfig:
+		// TODO: we don't really need to hide the entirety of --master-key, consider parsing the URL here.
+		return zap.String(f.Name, "<redacted>")
+	default:
+		return zap.Stringer(f.Name, f.Value)
 	}
-	return zap.Stringer(f.Name, f.Value)
 }
 
 // LogArguments prints origin command arguments.
diff --git a/br/pkg/task/common_test.go b/br/pkg/task/common_test.go
index a756aefb795e3..b58f3f52ee57f 100644
--- a/br/pkg/task/common_test.go
+++ b/br/pkg/task/common_test.go
@@ -32,13 +32,69 @@ func (f fakeValue) Type() string {
 }
 
 func TestUrlNoQuery(t *testing.T) {
-	flag := &pflag.Flag{
-		Name:  flagStorage,
-		Value: fakeValue("s3://some/what?secret=a123456789&key=987654321"),
+	testCases := []struct {
+		inputName     string
+		expectedName  string
+		inputValue    string
+		expectedValue string
+	}{
+		{
+			inputName:     flagSendCreds,
+			expectedName:  "send-credentials-to-tikv",
+			inputValue:    "true",
+			expectedValue: "true",
+		},
+		{
+			inputName:     flagStorage,
+			expectedName:  "storage",
+			inputValue:    "s3://some/what?secret=a123456789&key=987654321",
+			expectedValue: "s3://some/what",
+		},
+		{
+			inputName:     FlagStreamFullBackupStorage,
+			expectedName:  "full-backup-storage",
+			inputValue:    "s3://bucket/prefix/?access-key=1&secret-key=2",
+			expectedValue: "s3://bucket/prefix/",
+		},
+		{
+			inputName:     flagFullBackupCipherKey,
+			expectedName:  "crypter.key",
+			inputValue:    "537570657253656372657456616C7565",
+			expectedValue: "<redacted>",
+		},
+		{
+			inputName:     flagLogBackupCipherKey,
+			expectedName:  "log.crypter.key",
+			inputValue:    "537570657253656372657456616C7565",
+			expectedValue: "<redacted>",
+		},
+		{
+			inputName:     "azblob.encryption-key",
+			expectedName:  "azblob.encryption-key",
+			inputValue:    "SUPERSECRET_AZURE_ENCRYPTION_KEY",
+			expectedValue: "<redacted>",
+		},
+		{
+			inputName:     flagMasterKeyConfig,
+			expectedName:  "master-key",
+			inputValue:    "local:///path/abcd,aws-kms:///abcd?AWS_ACCESS_KEY_ID=SECRET1&AWS_SECRET_ACCESS_KEY=SECRET2&REGION=us-east-1,azure-kms:///abcd/v1?AZURE_TENANT_ID=tenant-id&AZURE_CLIENT_ID=client-id&AZURE_CLIENT_SECRET=client-secret&AZURE_VAULT_NAME=vault-name",
+			expectedValue: "<redacted>",
+			// expectedValue: "local:///path/abcd,aws-kms:///abcd,azure-kms:///abcd/v1"
+		},
+	}
+
+	for _, tc := range testCases {
+		flag := pflag.Flag{
+			Name:  tc.inputName,
+			Value: fakeValue(tc.inputValue),
+		}
+		field := flagToZapField(&flag)
+		require.Equal(t, tc.expectedName, field.Key, `test-case [%s="%s"]`, tc.expectedName, tc.expectedValue)
+		if stringer, ok := field.Interface.(fmt.Stringer); ok {
+			field.String = stringer.String()
+		}
+		require.Equal(t, tc.expectedValue, field.String, `test-case [%s="%s"]`, tc.expectedName, tc.expectedValue)
 	}
-	field := flagToZapField(flag)
-	require.Equal(t, flagStorage, field.Key)
-	require.Equal(t, "s3://some/what", field.Interface.(fmt.Stringer).String())
 }
 
 func TestTiDBConfigUnchanged(t *testing.T) {