Merge pull request #472 from stbenjam/minimum-length

secretutil: implement configurable minimum secret length

Author: k8s-ci-robot

config/prow/cluster/prowjob-crd/prowjob_customresourcedefinition.yaml

@@ -149,6 +149,12 @@ spec:
                         items:
                           type: string
                         type: array
+                      minimum_secret_length:
+                        description: |-
+                          MinimumSecretLength is the minimum length a secret must have to be censored.
+                          Secrets shorter than this length will not be censored. If unset, defaults to 0
+                          (all secrets are censored regardless of length).
+                        type: integer
                     type: object
                   cookiefile_secret:
                     description: |-

pkg/apis/prowjobs/v1/types.go

@@ -594,6 +594,11 @@ type CensoringOptions struct {
 	// matches a glob in IncludeDirectories. Entries in this list are relative to $ARTIFACTS,
 	// and are parsed with the go-zglob library, allowing for globbed matches.
 	ExcludeDirectories []string `json:"exclude_directories,omitempty"`
+
+	// MinimumSecretLength is the minimum length a secret must have to be censored.
+	// Secrets shorter than this length will not be censored. If unset, defaults to 0
+	// (all secrets are censored regardless of length).
+	MinimumSecretLength *int `json:"minimum_secret_length,omitempty"`
 }
 
 type SchedulingOptions struct {
@@ -636,6 +641,10 @@ func (g *CensoringOptions) ApplyDefault(def *CensoringOptions) *CensoringOptions
 	if merged.ExcludeDirectories == nil {
 		merged.ExcludeDirectories = def.ExcludeDirectories
 	}
+
+	if merged.MinimumSecretLength == nil {
+		merged.MinimumSecretLength = def.MinimumSecretLength
+	}
 	return &merged
 }
 

pkg/apis/prowjobs/v1/zz_generated.deepcopy.go

@@ -622,6 +622,10 @@ plank:
                 # globbed matches.
                 include_directories:
                     - ""
+                # MinimumSecretLength is the minimum length a secret must have to be censored.
+                # Secrets shorter than this length will not be censored. If unset, defaults to 0
+                # (all secrets are censored regardless of length).
+                minimum_secret_length: 0
             # CookieFileSecret is the name of a kubernetes secret that contains
             # a git http.cookiefile, which should be used during the cloning process.
             cookiefile_secret: ""
@@ -975,6 +979,10 @@ plank:
                 # globbed matches.
                 include_directories:
                     - ""
+                # MinimumSecretLength is the minimum length a secret must have to be censored.
+                # Secrets shorter than this length will not be censored. If unset, defaults to 0
+                # (all secrets are censored regardless of length).
+                minimum_secret_length: 0
             # CookieFileSecret is the name of a kubernetes secret that contains
             # a git http.cookiefile, which should be used during the cloning process.
             cookiefile_secret: ""

pkg/config/prow-config-documented.yaml

@@ -34,16 +34,22 @@ type Censorer interface {
 }
 
 func NewCensorer() *ReloadingCensorer {
+	return NewCensorerWithMinLength(0)
+}
+
+func NewCensorerWithMinLength(minLength int) *ReloadingCensorer {
 	return &ReloadingCensorer{
-		RWMutex:  &sync.RWMutex{},
-		Replacer: bytereplacer.New(),
+		RWMutex:             &sync.RWMutex{},
+		Replacer:            bytereplacer.New(),
+		minimumSecretLength: minLength,
 	}
 }
 
 type ReloadingCensorer struct {
 	*sync.RWMutex
 	*bytereplacer.Replacer
-	largestSecret int
+	largestSecret       int
+	minimumSecretLength int
 }
 
 var _ Censorer = &ReloadingCensorer{}
@@ -108,6 +114,10 @@ func (c *ReloadingCensorer) Refresh(secrets ...string) {
 			// to justify secret booleans, since all values are known and replacing them carries a high price.
 			continue
 		}
+		if len(secret) < c.minimumSecretLength {
+			// Skip secrets that are shorter than the minimum length
+			continue
+		}
 
 		addReplacement(secret)
 		for _, item := range toEncode {

pkg/secretutil/censor.go

@@ -150,3 +150,75 @@ func TestBooleanNotHidden(t *testing.T) {
 		})
 	}
 }
+
+func TestMinimumSecretLength(t *testing.T) {
+	var testCases = []struct {
+		name      string
+		secrets   []string
+		minLength int
+		input     string
+		expected  string
+	}{
+		{
+			name:      "no minimum length - all secrets censored",
+			secrets:   []string{"a", "bb", "ccc", "dddd"},
+			minLength: 0,
+			input:     "data: a bb ccc dddd",
+			expected:  "dXtX: X XX XXX XXXX",
+		},
+		{
+			name:      "minimum length 3 - short secrets not censored",
+			secrets:   []string{"a", "bb", "ccc", "dddd"},
+			minLength: 3,
+			input:     "data: a bb ccc dddd",
+			expected:  "data: a bb XXX XXXX",
+		},
+		{
+			name:      "minimum length 5 - all secrets censored",
+			secrets:   []string{"short", "verylongsecret"},
+			minLength: 5,
+			input:     "data: short verylongsecret",
+			expected:  "data: XXXXX XXXXXXXXXXXXXX",
+		},
+		{
+			name:      "minimum length 10 - no secrets censored",
+			secrets:   []string{"short", "medium"},
+			minLength: 10,
+			input:     "data: short medium",
+			expected:  "data: short medium",
+		},
+		{
+			name:      "base64 encoding also respects minimum length",
+			secrets:   []string{"abc"},
+			minLength: 5,
+			input:     "data: abc YWJj", // YWJj is base64 for "abc"
+			expected:  "data: abc YWJj", // both too short to censor
+		},
+		{
+			name:      "base64 encoding censored when long enough",
+			secrets:   []string{"secret"},
+			minLength: 5,
+			input:     "data: secret c2VjcmV0", // c2VjcmV0 is base64 for "secret"
+			expected:  "data: XXXXXX XXXXXXXX",
+		},
+		{
+			name:      "mixed length secrets with minimum",
+			secrets:   []string{"x", "password123", "key"},
+			minLength: 4,
+			input:     "config: x=1 password123=secret key=value",
+			expected:  "config: x=1 XXXXXXXXXXX=secret key=value",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			censorer := NewCensorerWithMinLength(testCase.minLength)
+			censorer.Refresh(testCase.secrets...)
+			input := []byte(testCase.input)
+			censorer.Censor(&input)
+			if diff := cmp.Diff(testCase.expected, string(input)); diff != "" {
+				t.Errorf("%s: got incorrect text after censor: %v", testCase.name, diff)
+			}
+		})
+	}
+}

pkg/secretutil/censor_test.go

@@ -77,7 +77,14 @@ func (o Options) censor() error {
 		return fmt.Errorf("could not load secrets: %w", err)
 	}
 	logrus.WithField("secrets", len(secrets)).Debug("Loaded secrets to censor.")
-	censorer := secretutil.NewCensorer()
+
+	minLength := 0
+	if o.CensoringOptions.MinimumSecretLength != nil {
+		minLength = *o.CensoringOptions.MinimumSecretLength
+	}
+	logrus.WithField("minimum_secret_length", minLength).Debug("Using minimum secret length for censoring.")
+
+	censorer := secretutil.NewCensorerWithMinLength(minLength)
 	censorer.RefreshBytes(secrets...)
 
 	bufferSize := defaultBufferSize

pkg/sidecar/censor.go

@@ -125,6 +125,11 @@ type CensoringOptions struct {
 	// IniFilenames are secret filenames that should be parsed as INI files in order to
 	// censor the values in the key-value mapping as well as the full content of the file.
 	IniFilenames []string `json:"ini_filenames,omitempty"`
+
+	// MinimumSecretLength is the minimum length a secret must have to be censored.
+	// Secrets shorter than this length will not be censored. If unset, defaults to 0
+	// (all secrets are censored regardless of length).
+	MinimumSecretLength *int `json:"minimum_secret_length,omitempty"`
 }
 
 func (o Options) entries() []wrapper.Options {