From fe89098bc9d847f80d4f188942ae886e51736217 Mon Sep 17 00:00:00 2001 From: Anatoly Fayngelerin Date: Tue, 19 Apr 2022 13:13:54 -0400 Subject: [PATCH] redact s3 credential values when printing config to logs --- pkg/loki/config_wrapper_test.go | 12 +++--- .../chunk/client/aws/s3_storage_client.go | 32 ++++++++++++---- .../client/aws/s3_storage_client_test.go | 37 +++++++++++++++++-- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/pkg/loki/config_wrapper_test.go b/pkg/loki/config_wrapper_test.go index 82547b38ab824..d0150c0c6d8e9 100644 --- a/pkg/loki/config_wrapper_test.go +++ b/pkg/loki/config_wrapper_test.go @@ -267,8 +267,8 @@ memberlist: assert.Equal(t, false, actual.S3ForcePathStyle) assert.Equal(t, "s3://foo-bucket", actual.Endpoint) assert.Equal(t, "us-east1", actual.Region) - assert.Equal(t, "abc123", actual.AccessKeyID) - assert.Equal(t, "def789", actual.SecretAccessKey) + assert.Equal(t, "abc123", actual.AccessKeyID.Value) + assert.Equal(t, "def789", actual.SecretAccessKey.Value) assert.Equal(t, true, actual.Insecure) assert.Equal(t, false, actual.SSEEncryption) assert.Equal(t, 5*time.Minute, actual.HTTPConfig.ResponseHeaderTimeout) @@ -490,8 +490,8 @@ ruler: assert.Equal(t, "s3", config.Ruler.StoreConfig.Type) assert.Equal(t, "s3://foo-bucket", config.Ruler.StoreConfig.S3.Endpoint) assert.Equal(t, "us-east1", config.Ruler.StoreConfig.S3.Region) - assert.Equal(t, "abc123", config.Ruler.StoreConfig.S3.AccessKeyID) - assert.Equal(t, "def789", config.Ruler.StoreConfig.S3.SecretAccessKey) + assert.Equal(t, "abc123", config.Ruler.StoreConfig.S3.AccessKeyID.Value) + assert.Equal(t, "def789", config.Ruler.StoreConfig.S3.SecretAccessKey.Value) // should be set by common config assert.EqualValues(t, "foobar", config.StorageConfig.GCSConfig.BucketName) @@ -520,8 +520,8 @@ storage_config: assert.Equal(t, "s3://foo-bucket", config.StorageConfig.AWSStorageConfig.S3Config.Endpoint) assert.Equal(t, "us-east1", config.StorageConfig.AWSStorageConfig.S3Config.Region) - assert.Equal(t, "abc123", config.StorageConfig.AWSStorageConfig.S3Config.AccessKeyID) - assert.Equal(t, "def789", config.StorageConfig.AWSStorageConfig.S3Config.SecretAccessKey) + assert.Equal(t, "abc123", config.StorageConfig.AWSStorageConfig.S3Config.AccessKeyID.Value) + assert.Equal(t, "def789", config.StorageConfig.AWSStorageConfig.S3Config.SecretAccessKey.Value) // should be set by common config assert.EqualValues(t, "foobar", config.Ruler.StoreConfig.GCS.BucketName) diff --git a/pkg/storage/chunk/client/aws/s3_storage_client.go b/pkg/storage/chunk/client/aws/s3_storage_client.go index 4499520833725..badf31bb59e9c 100644 --- a/pkg/storage/chunk/client/aws/s3_storage_client.go +++ b/pkg/storage/chunk/client/aws/s3_storage_client.go @@ -14,6 +14,8 @@ import ( "strings" "time" + "gopkg.in/yaml.v2" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/credentials" @@ -61,6 +63,20 @@ func init() { s3RequestDuration.Register() } +type secret struct { + Value string +} + +func (secret) MarshalYAML() (interface{}, error) { + return "redacted", nil +} + +func (s *secret) UnmarshalYAML(unmarshal func(interface{}) error) error { + return unmarshal(&s.Value) +} + +var _ yaml.Marshaler = secret{} + // S3Config specifies config for storing chunks on AWS S3. type S3Config struct { S3 flagext.URLValue @@ -69,8 +85,8 @@ type S3Config struct { BucketNames string Endpoint string `yaml:"endpoint"` Region string `yaml:"region"` - AccessKeyID string `yaml:"access_key_id"` - SecretAccessKey string `yaml:"secret_access_key"` + AccessKeyID secret `yaml:"access_key_id"` + SecretAccessKey secret `yaml:"secret_access_key"` Insecure bool `yaml:"insecure"` SSEEncryption bool `yaml:"sse_encryption"` HTTPConfig HTTPConfig `yaml:"http_config"` @@ -103,8 +119,8 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { f.StringVar(&cfg.Endpoint, prefix+"s3.endpoint", "", "S3 Endpoint to connect to.") f.StringVar(&cfg.Region, prefix+"s3.region", "", "AWS region to use.") - f.StringVar(&cfg.AccessKeyID, prefix+"s3.access-key-id", "", "AWS Access Key ID") - f.StringVar(&cfg.SecretAccessKey, prefix+"s3.secret-access-key", "", "AWS Secret Access Key") + f.StringVar(&cfg.AccessKeyID.Value, prefix+"s3.access-key-id", "", "AWS Access Key ID") + f.StringVar(&cfg.SecretAccessKey.Value, prefix+"s3.secret-access-key", "", "AWS Secret Access Key") f.BoolVar(&cfg.Insecure, prefix+"s3.insecure", false, "Disable https on s3 connection.") // TODO Remove in Cortex 1.10.0 @@ -237,13 +253,13 @@ func buildS3Client(cfg S3Config, hedgingCfg hedging.Config, hedging bool) (*s3.S s3Config = s3Config.WithRegion(cfg.Region) } - if cfg.AccessKeyID != "" && cfg.SecretAccessKey == "" || - cfg.AccessKeyID == "" && cfg.SecretAccessKey != "" { + if cfg.AccessKeyID.Value != "" && cfg.SecretAccessKey.Value == "" || + cfg.AccessKeyID.Value == "" && cfg.SecretAccessKey.Value != "" { return nil, errors.New("must supply both an Access Key ID and Secret Access Key or neither") } - if cfg.AccessKeyID != "" && cfg.SecretAccessKey != "" { - creds := credentials.NewStaticCredentials(cfg.AccessKeyID, cfg.SecretAccessKey, "") + if cfg.AccessKeyID.Value != "" && cfg.SecretAccessKey.Value != "" { + creds := credentials.NewStaticCredentials(cfg.AccessKeyID.Value, cfg.SecretAccessKey.Value, "") s3Config = s3Config.WithCredentials(creds) } diff --git a/pkg/storage/chunk/client/aws/s3_storage_client_test.go b/pkg/storage/chunk/client/aws/s3_storage_client_test.go index 66ef3722d9dc5..f61cbeeb5019b 100644 --- a/pkg/storage/chunk/client/aws/s3_storage_client_test.go +++ b/pkg/storage/chunk/client/aws/s3_storage_client_test.go @@ -12,6 +12,8 @@ import ( "testing" "time" + "gopkg.in/yaml.v2" + "github.com/grafana/dskit/backoff" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -37,8 +39,8 @@ func TestRequestMiddleware(t *testing.T) { BucketNames: "buck-o", S3ForcePathStyle: true, Insecure: true, - AccessKeyID: "key", - SecretAccessKey: "secret", + AccessKeyID: secret{"key"}, + SecretAccessKey: secret{"secret"}, } tests := []struct { @@ -126,8 +128,8 @@ func Test_Hedging(t *testing.T) { count := atomic.NewInt32(0) c, err := NewS3ObjectClient(S3Config{ - AccessKeyID: "foo", - SecretAccessKey: "bar", + AccessKeyID: secret{"foo"}, + SecretAccessKey: secret{"bar"}, BackoffConfig: backoff.Config{MaxRetries: 1}, BucketNames: "foo", Inject: func(next http.RoundTripper) http.RoundTripper { @@ -148,3 +150,30 @@ func Test_Hedging(t *testing.T) { }) } } + +func Test_ConfigRedactsCredentials(t *testing.T) { + underTest := S3Config{ + AccessKeyID: secret{"secret key id"}, + SecretAccessKey: secret{"secret access key"}, + } + + output, err := yaml.Marshal(underTest) + require.NoError(t, err) + + require.False(t, bytes.Contains(output, []byte("secret key id"))) + require.False(t, bytes.Contains(output, []byte("secret access id"))) +} + +func Test_ConfigParsesCredentialsInline(t *testing.T) { + var underTest = S3Config{} + yamlCfg := ` +access_key_id: access key id +secret_access_key: secret access key +` + err := yaml.Unmarshal([]byte(yamlCfg), &underTest) + require.NoError(t, err) + + require.Equal(t, underTest.AccessKeyID.Value, "access key id") + require.Equal(t, underTest.SecretAccessKey.Value, "secret access key") + +}