security: Redact credentials when marshalled to YAML (#6186)

pkg/loki/config_wrapper_test.go

@@ -270,8 +270,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.Value)
-				assert.Equal(t, "def789", actual.SecretAccessKey.Value)
+				assert.Equal(t, "abc123", actual.AccessKeyID)
+				assert.Equal(t, "def789", actual.SecretAccessKey.String())
 				assert.Equal(t, true, actual.Insecure)
 				assert.Equal(t, false, actual.SSEEncryption)
 				assert.Equal(t, 5*time.Minute, actual.HTTPConfig.ResponseHeaderTimeout)
@@ -406,7 +406,7 @@ memberlist:
 				assert.Equal(t, "arcosx", actual.BucketName)
 				assert.Equal(t, "bj.bcebos.com", actual.Endpoint)
 				assert.Equal(t, "baidu", actual.AccessKeyID)
-				assert.Equal(t, "bce", actual.SecretAccessKey)
+				assert.Equal(t, "bce", actual.SecretAccessKey.String())
 			}
 
 			// should remain empty
@@ -538,8 +538,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.Value)
-			assert.Equal(t, "def789", config.Ruler.StoreConfig.S3.SecretAccessKey.Value)
+			assert.Equal(t, "abc123", config.Ruler.StoreConfig.S3.AccessKeyID)
+			assert.Equal(t, "def789", config.Ruler.StoreConfig.S3.SecretAccessKey.String())
 
 			// should be set by common config
 			assert.EqualValues(t, "foobar", config.StorageConfig.GCSConfig.BucketName)
@@ -568,8 +568,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.Value)
-			assert.Equal(t, "def789", config.StorageConfig.AWSStorageConfig.S3Config.SecretAccessKey.Value)
+			assert.Equal(t, "abc123", config.StorageConfig.AWSStorageConfig.S3Config.AccessKeyID)
+			assert.Equal(t, "def789", config.StorageConfig.AWSStorageConfig.S3Config.SecretAccessKey.String())
 
 			// should be set by common config
 			assert.EqualValues(t, "foobar", config.Ruler.StoreConfig.GCS.BucketName)

pkg/storage/chunk/client/aws/s3_storage_client.go

@@ -14,8 +14,6 @@ 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"
@@ -63,20 +61,6 @@ 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
@@ -85,8 +69,8 @@ type S3Config struct {
 	BucketNames      string
 	Endpoint         string              `yaml:"endpoint"`
 	Region           string              `yaml:"region"`
-	AccessKeyID      secret              `yaml:"access_key_id"`
-	SecretAccessKey  secret              `yaml:"secret_access_key"`
+	AccessKeyID      string              `yaml:"access_key_id"`
+	SecretAccessKey  flagext.Secret      `yaml:"secret_access_key"`
 	Insecure         bool                `yaml:"insecure"`
 	SSEEncryption    bool                `yaml:"sse_encryption"`
 	HTTPConfig       HTTPConfig          `yaml:"http_config"`
@@ -119,8 +103,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.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.StringVar(&cfg.AccessKeyID, prefix+"s3.access-key-id", "", "AWS Access Key ID")
+	f.Var(&cfg.SecretAccessKey, 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
@@ -253,13 +237,13 @@ func buildS3Client(cfg S3Config, hedgingCfg hedging.Config, hedging bool) (*s3.S
 		s3Config = s3Config.WithRegion(cfg.Region)
 	}
 
-	if cfg.AccessKeyID.Value != "" && cfg.SecretAccessKey.Value == "" ||
-		cfg.AccessKeyID.Value == "" && cfg.SecretAccessKey.Value != "" {
+	if cfg.AccessKeyID != "" && cfg.SecretAccessKey.String() == "" ||
+		cfg.AccessKeyID == "" && cfg.SecretAccessKey.String() != "" {
 		return nil, errors.New("must supply both an Access Key ID and Secret Access Key or neither")
 	}
 
-	if cfg.AccessKeyID.Value != "" && cfg.SecretAccessKey.Value != "" {
-		creds := credentials.NewStaticCredentials(cfg.AccessKeyID.Value, cfg.SecretAccessKey.Value, "")
+	if cfg.AccessKeyID != "" && cfg.SecretAccessKey.String() != "" {
+		creds := credentials.NewStaticCredentials(cfg.AccessKeyID, cfg.SecretAccessKey.String(), "")
 		s3Config = s3Config.WithCredentials(creds)
 	}
 

pkg/storage/chunk/client/aws/s3_storage_client_test.go

@@ -15,6 +15,7 @@ import (
 	"gopkg.in/yaml.v2"
 
 	"github.com/grafana/dskit/backoff"
+	"github.com/grafana/dskit/flagext"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/atomic"
@@ -39,8 +40,8 @@ func TestRequestMiddleware(t *testing.T) {
 		BucketNames:      "buck-o",
 		S3ForcePathStyle: true,
 		Insecure:         true,
-		AccessKeyID:      secret{"key"},
-		SecretAccessKey:  secret{"secret"},
+		AccessKeyID:      "key",
+		SecretAccessKey:  flagext.SecretWithValue("secret"),
 	}
 
 	tests := []struct {
@@ -128,8 +129,8 @@ func Test_Hedging(t *testing.T) {
 			count := atomic.NewInt32(0)
 
 			c, err := NewS3ObjectClient(S3Config{
-				AccessKeyID:     secret{"foo"},
-				SecretAccessKey: secret{"bar"},
+				AccessKeyID:     "foo",
+				SecretAccessKey: flagext.SecretWithValue("bar"),
 				BackoffConfig:   backoff.Config{MaxRetries: 1},
 				BucketNames:     "foo",
 				Inject: func(next http.RoundTripper) http.RoundTripper {
@@ -153,14 +154,14 @@ func Test_Hedging(t *testing.T) {
 
 func Test_ConfigRedactsCredentials(t *testing.T) {
 	underTest := S3Config{
-		AccessKeyID:     secret{"secret key id"},
-		SecretAccessKey: secret{"secret access key"},
+		AccessKeyID:     "access key id",
+		SecretAccessKey: flagext.SecretWithValue("secret access key"),
 	}
 
 	output, err := yaml.Marshal(underTest)
 	require.NoError(t, err)
 
-	require.False(t, bytes.Contains(output, []byte("secret key id")))
+	require.True(t, bytes.Contains(output, []byte("access key id")))
 	require.False(t, bytes.Contains(output, []byte("secret access id")))
 }
 
@@ -173,7 +174,7 @@ 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")
+	require.Equal(t, underTest.AccessKeyID, "access key id")
+	require.Equal(t, underTest.SecretAccessKey.String(), "secret access key")
 
 }

pkg/storage/chunk/client/baidubce/bos_storage_client.go

@@ -10,6 +10,7 @@ import (
 	"github.com/baidubce/bce-sdk-go/bce"
 	"github.com/baidubce/bce-sdk-go/services/bos"
 	"github.com/baidubce/bce-sdk-go/services/bos/api"
+	"github.com/grafana/dskit/flagext"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/weaveworks/common/instrument"
@@ -36,10 +37,10 @@ func init() {
 }
 
 type BOSStorageConfig struct {
-	BucketName      string `yaml:"bucket_name"`
-	Endpoint        string `yaml:"endpoint"`
-	AccessKeyID     string `yaml:"access_key_id"`
-	SecretAccessKey string `yaml:"secret_access_key"`
+	BucketName      string         `yaml:"bucket_name"`
+	Endpoint        string         `yaml:"endpoint"`
+	AccessKeyID     string         `yaml:"access_key_id"`
+	SecretAccessKey flagext.Secret `yaml:"secret_access_key"`
 }
 
 // RegisterFlags adds the flags required to config this to the given FlagSet
@@ -52,7 +53,7 @@ func (cfg *BOSStorageConfig) RegisterFlagsWithPrefix(prefix string, f *flag.Flag
 	f.StringVar(&cfg.BucketName, prefix+"baidubce.bucket-name", "", "Name of BOS bucket.")
 	f.StringVar(&cfg.Endpoint, prefix+"baidubce.endpoint", DefaultEndpoint, "BOS endpoint to connect to.")
 	f.StringVar(&cfg.AccessKeyID, prefix+"baidubce.access-key-id", "", "Baidu Cloud Engine (BCE) Access Key ID.")
-	f.StringVar(&cfg.SecretAccessKey, prefix+"baidubce.secret-access-key", "", "Baidu Cloud Engine (BCE) Secret Access Key.")
+	f.Var(&cfg.SecretAccessKey, prefix+"baidubce.secret-access-key", "Baidu Cloud Engine (BCE) Secret Access Key.")
 }
 
 type BOSObjectStorage struct {
@@ -63,7 +64,7 @@ type BOSObjectStorage struct {
 func NewBOSObjectStorage(cfg *BOSStorageConfig) (*BOSObjectStorage, error) {
 	clientConfig := bos.BosClientConfiguration{
 		Ak:               cfg.AccessKeyID,
-		Sk:               cfg.SecretAccessKey,
+		Sk:               cfg.SecretAccessKey.String(),
 		Endpoint:         cfg.Endpoint,
 		RedirectDisabled: false,
 	}

pkg/storage/chunk/client/baidubce/bos_storage_client_test.go

@@ -0,0 +1,24 @@
+package baidubce
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gopkg.in/yaml.v2"
+
+	"github.com/grafana/dskit/flagext"
+)
+
+func Test_ConfigRedactsCredentials(t *testing.T) {
+	underTest := BOSStorageConfig{
+		AccessKeyID:     "access key id",
+		SecretAccessKey: flagext.SecretWithValue("secret access key"),
+	}
+
+	output, err := yaml.Marshal(underTest)
+	require.NoError(t, err)
+
+	require.True(t, bytes.Contains(output, []byte("access key id")))
+	require.False(t, bytes.Contains(output, []byte("secret access id")))
+}