search

pingcap / tidb

TiDB - the open-source, cloud-native, distributed SQL database designed for modern applications.
https://pingcap.com
Apache License 2.0
37.34k stars 5.85k forks source link

BR: --crypter.key & --full-backup-storage are not redacted from the log #57585

Closed kennytm closed 1 day ago

kennytm commented 1 day ago

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

br restore point -s 's3://foo/bar/?access-key=SecretValue&secret-access-key=SuperSecretValue' \
    --full-backup-storage 's3://foo/bar2/?access-key=SecretValue&secret-access-key=SuperSecretValue' \
    --s3.endpoint http://127.0.0.1:9999 \
    --crypter.method aes128-ctr \
    --crypter.key 537570657253656372657456616C7565

then read the first few lines of the BR log

2. What did you expect to see? (Required)

The secret keys "SecretValue", "SuperSecretValue" and "537570657253656372657456616C7565" do not appear in the log.

3. What did you see instead (Required)

They are written in plaintext.

[2024/11/21 16:03:36.806 +08:00] [INFO] [common.go:765] [arguments] [__command="br restore point"] [crypter.key=537570657253656372657456616C7565] [crypter.method=aes128-ctr] [full-backup-storage="s3://foo/bar2/?access-key=SecretValue&secret-access-key=SuperSecretValue"] [s3.endpoint=http://127.0.0.1:9999] [storage=s3://foo/bar/]

4. What is your TiDB version? (Required)

BR v8.4.0, v8.2.0, v7.5.4, v7.1.5 (probably every version)

kennytm commented 1 day ago

Note that the --storage field is correctly redacted. This is handled by

https://github.com/pingcap/tidb/blob/c091dba89718599dc1b3d3e45f9b0308a8d49ef0/br/pkg/task/common.go#L897-L908

We should apply it for --crypter.key and --full-backup-storage too.