Address review comments - part 2

Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>

cmd/cosign/cli/env.go

@@ -18,6 +18,7 @@ package cli
 import (
 	"fmt"
 	"os"
+	"sort"
 	"strings"
 
 	"github.com/spf13/cobra"
@@ -34,13 +35,8 @@ func Env() *cobra.Command {
 		Short: "Prints Cosign environment variables",
 		Args:  cobra.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			// Print COSIGN_ environment variables
-			env.PrintEnv(o.ShowDescriptions, o.ShowSensitiveValues)
-
-			// Print external environment variables (SIGSTORE_ and TUF_)
-			for _, e := range getExternalEnv() {
-				fmt.Println(e)
-			}
+			envVars := env.EnvironmentVariables()
+			printEnv(envVars, o.ShowDescriptions, o.ShowSensitiveValues)
 
 			return nil
 		},
@@ -50,21 +46,78 @@ func Env() *cobra.Command {
 	return cmd
 }
 
-func getExternalEnv() []string {
-	out := []string{}
+// NB: printEnv intentionally takes map of env vars to make it easier to unit test it
+func printEnv(envVars map[env.Variable]env.VariableOpts, showDescription, showSensitive bool) {
+	// Sort keys to print them in a predictable order
+	keys := sortEnvKeys(envVars)
+
+	// Print known/registered environment variables
+	for _, env := range keys {
+		opts := envVars[env]
+
+		// Get value of environment variable
+		// NB: This package works with environment variables, so we ignore forbidigo linter.
+		// We could use env.Getenv here, but that would make unit testing too complicated
+		// as unit tests are using non-registered variables
+		val := os.Getenv(env.String()) //nolint:forbidigo
+
+		// If showDescription is set, print description for that variable
+		if showDescription {
+			fmt.Printf("# %s %s\n", env.String(), opts.Description)
+			fmt.Printf("# Expects: %s\n", opts.Expects)
+		}
+
+		// If variable is sensitive, and we don't want to show sensitive values,
+		// print environment variable name and some asterisk symbols.
+		// If sensitive variable isn't set or doesn't have any value, we'll just
+		// print like non-sensitive variable
+		if opts.Sensitive && !showSensitive && val != "" {
+			fmt.Printf("%s=\"******\"\n", env.String())
+		} else {
+			fmt.Printf("%s=%q\n", env.String(), val)
+		}
+	}
+
+	// Print not registered environment variables
+	var nonRegEnv []string
 	for _, e := range os.Environ() {
 		// Prefixes to look for. err on the side of showing too much rather
 		// than too little. We'll only output things that have values set.
 		for _, prefix := range []string{
+			// We want to print eventually non-registered cosign variables (even if this shouldn't happen)
+			"COSIGN_",
 			// Can modify Sigstore/TUF client behavior - https://github.com/sigstore/sigstore/blob/35d6a82c15183f7fe7a07eca45e17e378aa32126/pkg/tuf/client.go#L52
 			"SIGSTORE_",
 			"TUF_",
 		} {
 			if strings.HasPrefix(e, prefix) {
-				out = append(out, e)
-				continue
+				// Skip registered environment variables (those are already printed above).
+				// os.Environ returns key=value pairs, so we use split by '=' to get the key
+				key := strings.Split(e, "=")[0]
+				if _, ok := envVars[env.Variable(key)]; ok {
+					continue
+				}
+				nonRegEnv = append(nonRegEnv, e)
 			}
 		}
 	}
-	return out
+	if len(nonRegEnv) > 0 && showDescription {
+		fmt.Println("# Environment variables below are not registered with cosign,\n# but might still influence cosign's behavior.")
+	}
+	for _, e := range nonRegEnv {
+		fmt.Println(e)
+	}
+}
+
+func sortEnvKeys(envVars map[env.Variable]env.VariableOpts) []env.Variable {
+	keys := []env.Variable{}
+	for k := range envVars {
+		keys = append(keys, k)
+	}
+
+	sort.Slice(keys, func(i, j int) bool {
+		return strings.Compare(keys[i].String(), keys[j].String()) < 0
+	})
+
+	return keys
 }

cmd/cosign/cli/env_test.go

@@ -16,22 +16,204 @@
 package cli
 
 import (
+	"io"
 	"os"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
+	"github.com/sigstore/cosign/pkg/cosign/env"
 )
 
-func TestGetEnv(t *testing.T) {
-	os.Setenv("SIGSTORE_TEST", "test")
-	os.Setenv("TUF_ROOT", "bar")
-	got := getExternalEnv()
-	want := []string{
-		"SIGSTORE_TEST=test",
-		"TUF_ROOT=bar",
+const (
+	VariableTest1 env.Variable = "COSIGN_TEST1"
+	VariableTest2 env.Variable = "COSIGN_TEST2"
+
+	expectedWithoutDescription = `COSIGN_TEST1="abcd"
+COSIGN_TEST2=""
+`
+	expectedWithDescription = `# COSIGN_TEST1 is the first test variable
+# Expects: test1 value
+COSIGN_TEST1="abcd"
+# COSIGN_TEST2 is the second test variable
+# Expects: test2 value
+COSIGN_TEST2=""
+`
+
+	expectedWithHiddenSensitive = `# COSIGN_TEST1 is the first test variable
+# Expects: test1 value
+COSIGN_TEST1="abcd"
+# COSIGN_TEST2 is the second test variable
+# Expects: test2 value
+COSIGN_TEST2="******"
+`
+
+	expectedWithSensitive = `# COSIGN_TEST1 is the first test variable
+# Expects: test1 value
+COSIGN_TEST1="abcd"
+# COSIGN_TEST2 is the second test variable
+# Expects: test2 value
+COSIGN_TEST2="1234"
+`
+
+	expectedSensitiveWithoutDescription = `COSIGN_TEST1="abcd"
+COSIGN_TEST2="1234"
+`
+
+	expectedWithNonRegisteredEnv = `# COSIGN_TEST1 is the first test variable
+# Expects: test1 value
+COSIGN_TEST1="abcd"
+# COSIGN_TEST2 is the second test variable
+# Expects: test2 value
+COSIGN_TEST2=""
+# Environment variables below are not registered with cosign,
+# but might still influence cosign's behavior.
+COSIGN_TEST3=abcd
+`
+
+	expectedWithNonRegisteredEnvNoDesc = `COSIGN_TEST1="abcd"
+COSIGN_TEST2=""
+COSIGN_TEST3=abcd
+`
+)
+
+func TestPrintEnv(t *testing.T) {
+	variables := map[env.Variable]env.VariableOpts{
+		VariableTest1: {
+			Description: "is the first test variable",
+			Expects:     "test1 value",
+			Sensitive:   false,
+		},
+		VariableTest2: {
+			Description: "is the second test variable",
+			Expects:     "test2 value",
+			Sensitive:   true,
+		},
 	}
 
-	if diff := cmp.Diff(got, want); diff != "" {
-		t.Error(diff)
+	tests := []struct {
+		name                 string
+		environmentVariables map[string]string
+		registeredVariables  map[env.Variable]env.VariableOpts
+		showDescriptions     bool
+		showSensitiveValues  bool
+		expectedOutput       string
+	}{
+		{
+			name: "no descriptions and sensitive variables",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "",
+			},
+			registeredVariables: variables,
+			showDescriptions:    false,
+			showSensitiveValues: false,
+			expectedOutput:      expectedWithoutDescription,
+		},
+		{
+			name: "descriptions but sensitive variable is unset",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "",
+			},
+			registeredVariables: variables,
+			showDescriptions:    true,
+			showSensitiveValues: false,
+			expectedOutput:      expectedWithDescription,
+		},
+		{
+			name: "sensitive variable is non-empty but show sensitive variables is disabled",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "1234",
+			},
+			registeredVariables: variables,
+			showDescriptions:    true,
+			showSensitiveValues: false,
+			expectedOutput:      expectedWithHiddenSensitive,
+		},
+		{
+			name: "sensitive variable is empty",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "",
+			},
+			registeredVariables: variables,
+			showDescriptions:    true,
+			showSensitiveValues: true,
+			expectedOutput:      expectedWithDescription,
+		},
+		{
+			name: "sensitive variable is non-empty and show sensitive variables is enabled",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "1234",
+			},
+			registeredVariables: variables,
+			showDescriptions:    true,
+			showSensitiveValues: true,
+			expectedOutput:      expectedWithSensitive,
+		},
+		{
+			name: "sensitive variable is non-empty but show descriptions is disabled",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "1234",
+			},
+			registeredVariables: variables,
+			showDescriptions:    false,
+			showSensitiveValues: true,
+			expectedOutput:      expectedSensitiveWithoutDescription,
+		},
+		{
+			name: "print unregistered variable with description",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "",
+				"COSIGN_TEST3": "abcd",
+			},
+			registeredVariables: variables,
+			showDescriptions:    true,
+			showSensitiveValues: false,
+			expectedOutput:      expectedWithNonRegisteredEnv,
+		},
+		{
+			name: "print unregistered variable without description",
+			environmentVariables: map[string]string{
+				"COSIGN_TEST1": "abcd",
+				"COSIGN_TEST2": "",
+				"COSIGN_TEST3": "abcd",
+			},
+			registeredVariables: variables,
+			showDescriptions:    false,
+			showSensitiveValues: false,
+			expectedOutput:      expectedWithNonRegisteredEnvNoDesc,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Set needed environment variables
+			for k, v := range tt.environmentVariables {
+				os.Setenv(k, v)
+			}
+
+			orgStdout := os.Stdout
+			r, w, _ := os.Pipe()
+			os.Stdout = w
+
+			printEnv(tt.registeredVariables, tt.showDescriptions, tt.showSensitiveValues)
+
+			w.Close()
+			out, _ := io.ReadAll(r)
+			os.Stdout = orgStdout
+
+			if tt.expectedOutput != string(out) {
+				t.Errorf("Expected to get %q\n, but got %q", tt.expectedOutput, string(out))
+			}
+
+			// Unset needed environment variables to ensure clean state between tests
+			for k := range tt.environmentVariables {
+				os.Unsetenv(k)
+			}
+		})
 	}
 }

pkg/cosign/env/env.go

@@ -18,7 +18,6 @@ package env
 import (
 	"fmt"
 	"os"
-	"sort"
 	"strings"
 )
 
@@ -69,6 +68,8 @@ const (
 )
 
 var (
+	// NB: this is intentionally private to avoid anyone changing this from
+	// code. There's a getter function used to get this slice if needed.
 	environmentVariables = map[Variable]VariableOpts{
 		VariableExperimental: {
 			Description: "enables experimental cosign features",
@@ -171,6 +172,10 @@ var (
 	}
 )
 
+func EnvironmentVariables() map[Variable]VariableOpts {
+	return environmentVariables
+}
+
 func mustRegisterEnv(name Variable) {
 	opts, ok := environmentVariables[name]
 	if !ok {
@@ -192,44 +197,3 @@ func LookupEnv(name Variable) (string, bool) {
 
 	return os.LookupEnv(name.String())
 }
-
-func PrintEnv(showDescription, showSensitive bool) {
-	// Sort keys to print them in predictable order
-	keys := sortKeys()
-
-	for _, env := range keys {
-		opts := environmentVariables[env]
-
-		// Get value of environment variable
-		val := os.Getenv(env.String())
-
-		// If showDescription is set, print description for that variable
-		if showDescription {
-			fmt.Printf("# %s %s\n", env.String(), opts.Description)
-			fmt.Printf("# Expects: %s\n", opts.Expects)
-		}
-
-		// If variable is sensitive, and we don't want to show sensitive values,
-		// print environment variable name and some asterisk symbols.
-		// If sensitive variable isn't set or doesn't have any value, we'll just
-		// print like non-sensitive variable
-		if opts.Sensitive && !showSensitive && val != "" {
-			fmt.Printf("%s=\"******\"\n", env.String())
-		} else {
-			fmt.Printf("%s=%q\n", env.String(), val)
-		}
-	}
-}
-
-func sortKeys() []Variable {
-	keys := []Variable{}
-	for k := range environmentVariables {
-		keys = append(keys, k)
-	}
-
-	sort.Slice(keys, func(i, j int) bool {
-		return strings.Compare(keys[i].String(), keys[j].String()) < 0
-	})
-
-	return keys
-}