From c2f193cd09da31f811eae192bf375eda42016ecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=B1=B1=E5=B2=9A?= <36239017+YuJuncen@users.noreply.github.com> Date: Fri, 15 May 2020 17:33:11 +0800 Subject: [PATCH] Don't log secret of s3. (#292) * 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 * Apply suggestions from code review Co-authored-by: kennytm * *: remove query string on logging arguments. Co-authored-by: kennytm --- cmd/backup.go | 2 +- cmd/restore.go | 2 +- cmd/validate.go | 2 +- pkg/backup/client.go | 6 +++++- pkg/task/common.go | 29 +++++++++++++++++++++++++++++ pkg/task/common_test.go | 39 +++++++++++++++++++++++++++++++++++++++ pkg/utils/version.go | 11 ----------- tests/br_s3/run.sh | 28 ++++++++++++++++++++++++++-- 8 files changed, 102 insertions(+), 17 deletions(-) create mode 100644 pkg/task/common_test.go diff --git a/cmd/backup.go b/cmd/backup.go index d37229e0a..0958e92db 100644 --- a/cmd/backup.go +++ b/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 diff --git a/cmd/restore.go b/cmd/restore.go index 547501d95..d4f2e8ec6 100644 --- a/cmd/restore.go +++ b/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() diff --git a/cmd/validate.go b/cmd/validate.go index 386a7bb47..5968db2d1 100644 --- a/cmd/validate.go +++ b/cmd/validate.go @@ -37,7 +37,7 @@ func NewValidateCommand() *cobra.Command { return err } utils.LogBRInfo() - utils.LogArguments(c) + task.LogArguments(c) return nil }, } diff --git a/pkg/backup/client.go b/pkg/backup/client.go index 0a22fa40e..8012ac7b2 100644 --- a/pkg/backup/client.go +++ b/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) diff --git a/pkg/task/common.go b/pkg/task/common.go index 94e91495b..c7962a94a 100644 --- a/pkg/task/common.go +++ b/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, "") + } + // 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...) +} diff --git a/pkg/task/common_test.go b/pkg/task/common_test.go new file mode 100644 index 000000000..6c4869809 --- /dev/null +++ b/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") +} diff --git a/pkg/utils/version.go b/pkg/utils/version.go index b8248f969..e9c81b68e 100644 --- a/pkg/utils/version.go +++ b/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...) -} diff --git a/tests/br_s3/run.sh b/tests/br_s3/run.sh index 394ddcf0e..8b2cdace0 100755 --- a/tests/br_s3/run.sh +++ b/tests/br_s3/run.sh @@ -58,7 +58,19 @@ 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};" @@ -66,7 +78,19 @@ 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}')