Don't log secret of s3. (#292) (#293)

* backup: don't log secret of s3.

* br_s3: always print log.

* *: remove query string on logging arguments.

* br_s3: reset BR_LOG_TO_TERM on fail.

* Apply suggestions from code review

Co-authored-by: kennytm <kennytm@gmail.com>

* Apply suggestions from code review

Co-authored-by: kennytm <kennytm@gmail.com>

* *: remove query string on logging arguments.

Co-authored-by: Hillium <maruruku@stu.csust.edu.cn>
Co-authored-by: 山岚 <36239017+YuJuncen@users.noreply.github.com>
Co-authored-by: kennytm <kennytm@gmail.com>

cmd/backup.go

@@ -42,7 +42,7 @@ func NewBackupCommand() *cobra.Command {
 				return err
 			}
 			utils.LogBRInfo()
-			utils.LogArguments(c)
+			task.LogArguments(c)
 
 			// Do not run ddl worker in BR.
 			ddl.RunWorker = false

cmd/restore.go

@@ -53,7 +53,7 @@ func NewRestoreCommand() *cobra.Command {
 				return err
 			}
 			utils.LogBRInfo()
-			utils.LogArguments(c)
+			task.LogArguments(c)
 
 			// Do not run stat worker in BR.
 			session.DisableStats4Test()

cmd/validate.go

@@ -37,7 +37,7 @@ func NewValidateCommand() *cobra.Command {
 				return err
 			}
 			utils.LogBRInfo()
-			utils.LogArguments(c)
+			task.LogArguments(c)
 			return nil
 		},
 	}

pkg/backup/client.go

@@ -760,7 +760,11 @@ func SendBackup(
 	req kvproto.BackupRequest,
 	respFn func(*kvproto.BackupResponse) error,
 ) error {
-	log.Info("try backup", zap.Any("backup request", req))
+	log.Info("try backup",
+		zap.Binary("StartKey", req.StartKey),
+		zap.Binary("EndKey", req.EndKey),
+		zap.Uint64("storeID", storeID),
+	)
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	bcli, err := client.Backup(ctx, &req)

pkg/task/common.go

@@ -6,18 +6,21 @@ import (
 	"context"
 	"crypto/tls"
 	"fmt"
+	"net/url"
 	"regexp"
 	"strings"
 
 	"github.com/gogo/protobuf/proto"
 	"github.com/pingcap/errors"
 	"github.com/pingcap/kvproto/pkg/backup"
+	"github.com/pingcap/log"
 	pd "github.com/pingcap/pd/v4/client"
 	"github.com/pingcap/tidb-tools/pkg/filter"
 	"github.com/pingcap/tidb/store/tikv"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"go.etcd.io/etcd/pkg/transport"
+	"go.uber.org/zap"
 
 	"github.com/pingcap/br/pkg/conn"
 	"github.com/pingcap/br/pkg/glue"
@@ -286,3 +289,29 @@ func escapeFilterName(name string) string {
 	}
 	return "~^" + regexp.QuoteMeta(name) + "$"
 }
+
+// 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 {
+		hiddenQuery, err := url.Parse(f.Value.String())
+		if err != nil {
+			return zap.String(f.Name, "<invalid URI>")
+		}
+		// hide all query here.
+		hiddenQuery.RawQuery = ""
+		return zap.Stringer(f.Name, hiddenQuery)
+	}
+	return zap.Stringer(f.Name, f.Value)
+}
+
+// LogArguments prints origin command arguments.
+func LogArguments(cmd *cobra.Command) {
+	var fields []zap.Field
+	cmd.Flags().VisitAll(func(f *pflag.Flag) {
+		if f.Changed {
+			fields = append(fields, flagToZapField(f))
+		}
+	})
+	log.Info("arguments", fields...)
+}

pkg/task/common_test.go

@@ -0,0 +1,39 @@
+// Copyright 2020 PingCAP, Inc. Licensed under Apache-2.0.
+
+package task
+
+import (
+	"fmt"
+
+	. "github.com/pingcap/check"
+	"github.com/spf13/pflag"
+)
+
+var _ = Suite(&testCommonSuite{})
+
+type testCommonSuite struct{}
+
+type fakeValue string
+
+func (f fakeValue) String() string {
+	return string(f)
+}
+
+func (f fakeValue) Set(string) error {
+	panic("implement me")
+}
+
+func (f fakeValue) Type() string {
+	panic("implement me")
+}
+
+func (*testCommonSuite) TestUrlNoQuery(c *C) {
+	flag := &pflag.Flag{
+		Name:  flagStorage,
+		Value: fakeValue("s3://some/what?secret=a123456789&key=987654321"),
+	}
+
+	field := flagToZapField(flag)
+	c.Assert(field.Key, Equals, flagStorage)
+	c.Assert(field.Interface.(fmt.Stringer).String(), Equals, "s3://some/what")
+}

pkg/utils/version.go

@@ -9,8 +9,6 @@ import (
 
 	"github.com/pingcap/log"
 	"github.com/pingcap/tidb/util/israce"
-	"github.com/spf13/cobra"
-	"github.com/spf13/pflag"
 	"go.uber.org/zap"
 )
 
@@ -45,12 +43,3 @@ func BRInfo() string {
 	fmt.Fprintf(&buf, "Race Enabled: %t", israce.RaceEnabled)
 	return buf.String()
 }
-
-// LogArguments prints origin command arguments.
-func LogArguments(cmd *cobra.Command) {
-	var fields []zap.Field
-	cmd.Flags().VisitAll(func(f *pflag.Flag) {
-		fields = append(fields, zap.Stringer(f.Name, f.Value))
-	})
-	log.Info("arguments", fields...)
-}

tests/br_s3/run.sh

@@ -58,15 +58,39 @@ done
 
 # backup full
 echo "backup start..."
-run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT"
+BACKUP_LOG="backup.log"
+rm -f $BACKUP_LOG
+unset BR_LOG_TO_TERM
+run_br --pd $PD_ADDR backup full -s "s3://mybucket/$DB?endpoint=http://$S3_ENDPOINT" \
+    --log-file $BACKUP_LOG || \
+    ( cat $BACKUP_LOG && BR_LOG_TO_TERM=1 && exit 1 )
+cat $BACKUP_LOG
+BR_LOG_TO_TERM=1
+
+if grep -i $MINIO_SECRET_KEY $BACKUP_LOG; then
+    echo "Secret key logged in log. Please remove them."
+    exit 1
+fi
 
 for i in $(seq $DB_COUNT); do
     run_sql "DROP DATABASE $DB${i};"
 done
 
 # restore full
 echo "restore start..."
-run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT"
+RESTORE_LOG="restore.log"
+rm -f $RESTORE_LOG
+unset BR_LOG_TO_TERM
+run_br restore full -s "s3://mybucket/$DB" --pd $PD_ADDR --s3.endpoint="http://$S3_ENDPOINT" \
+    --log-file $RESTORE_LOG || \
+    ( cat $RESTORE_LOG && BR_LOG_TO_TERM=1 && exit 1 )
+cat $RESTORE_LOG
+BR_LOG_TO_TERM=1
+
+if grep -i $MINIO_SECRET_KEY $RESTORE_LOG; then
+    echo "Secret key logged in log. Please remove them."
+    exit 1
+fi
 
 for i in $(seq $DB_COUNT); do
     row_count_new[${i}]=$(run_sql "SELECT COUNT(*) FROM $DB${i}.$TABLE;" | awk '/COUNT/{print $2}')