diff --git a/internal/cryptoutil/cryptoutil.go b/internal/cryptoutil/cryptoutil.go index aea8a2bf8..f0f8ccb04 100644 --- a/internal/cryptoutil/cryptoutil.go +++ b/internal/cryptoutil/cryptoutil.go @@ -13,6 +13,7 @@ import ( "io" "os" "os/exec" + "regexp" "strconv" "strings" @@ -216,14 +217,30 @@ type kmsPublicKey struct { kms, key string } +// sensitiveURIParams matches the KMS URI attributes whose values are +// credentials: pin-value (yubikey, pkcs11, capi), management-key (yubikey), +// and client-secret (azurekms). The value runs until the next URI delimiter +// (";", "&", "?"), whitespace (an argv or word boundary), or a double quote (a +// Go-quoted prose boundary), so redaction leaves the surrounding command line +// and error text intact. +var sensitiveURIParams = regexp.MustCompile(`(?i)\b(pin-value|management-key|client-secret)=[^;&?\s"]*`) + +// redactSecrets replaces the values of sensitive KMS URI attributes in s with +// "REDACTED", so that a command line or its output can be embedded in an error +// message without disclosing PINs or other credentials. +func redactSecrets(s string) string { + return sensitiveURIParams.ReplaceAllString(s, "${1}=REDACTED") +} + // exitError returns the error displayed on stderr after running the given -// command. +// command, with the values of sensitive KMS URI attributes redacted. func exitError(cmd *exec.Cmd, err error) error { + command := redactSecrets(cmd.String()) var ee *exec.ExitError if errors.As(err, &ee) { - return fmt.Errorf("command %q failed with:\n%s", cmd.String(), ee.Stderr) + return fmt.Errorf("command %q failed with:\n%s", command, redactSecrets(string(ee.Stderr))) } - return fmt.Errorf("command %q failed with: %w", cmd.String(), err) + return fmt.Errorf("command %q failed with: %w", command, err) } // newKMSSigner creates a signer using `step-kms-plugin` as the signer. diff --git a/internal/cryptoutil/cryptoutil_test.go b/internal/cryptoutil/cryptoutil_test.go new file mode 100644 index 000000000..7e58209c4 --- /dev/null +++ b/internal/cryptoutil/cryptoutil_test.go @@ -0,0 +1,95 @@ +package cryptoutil + +import ( + "errors" + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestExitErrorRedactsCommandLine(t *testing.T) { + cmd := exec.Command("step-kms-plugin", "sign", "--format", "base64", + "--kms", "yubikey:serial=7654321;pin-value=123456;management-key=010203040506070801020304050607080102030405060708", + "yubikey:slot-id=9c;pin-value=positional-pin") + ee := &exec.ExitError{ + ProcessState: &os.ProcessState{}, + Stderr: []byte("Error: command failed: smart card error 6982: security status not satisfied"), + } + + err := exitError(cmd, ee) + require.Error(t, err) + + s := err.Error() + assert.NotContains(t, s, "123456") + assert.NotContains(t, s, "010203040506070801020304050607080102030405060708") + // A secret can appear in a positional argument too: --attestation-uri is + // passed as both the --kms value and the positional key argument. + assert.NotContains(t, s, "positional-pin") + assert.Contains(t, s, "pin-value=REDACTED") + assert.Contains(t, s, "management-key=REDACTED") + assert.Contains(t, s, "serial=7654321") + assert.Contains(t, s, "slot-id=9c") + assert.Contains(t, s, "smart card error 6982") +} + +func TestExitErrorRedactsStderr(t *testing.T) { + cmd := exec.Command("step-kms-plugin", "attest", + "--kms", "yubikey:serial=1;pin-value=secret123", "yubikey:slot-id=9c") + ee := &exec.ExitError{ + ProcessState: &os.ProcessState{}, + Stderr: []byte("Error: yubikey:serial=1;pin-value=secret123 does not implement Attester"), + } + + err := exitError(cmd, ee) + require.Error(t, err) + + s := err.Error() + assert.NotContains(t, s, "secret123") + assert.Contains(t, s, "does not implement Attester") +} + +func TestExitErrorRedactsWrappedError(t *testing.T) { + cmd := exec.Command("step-kms-plugin", "key", + "--kms", "pkcs11:token=smallstep?pin-value=badger", "pkcs11:id=7777") + sentinel := errors.New("something went wrong") + + err := exitError(cmd, sentinel) + require.Error(t, err) + require.ErrorIs(t, err, sentinel) + + s := err.Error() + assert.NotContains(t, s, "badger") + assert.Contains(t, s, "pin-value=REDACTED") + assert.Contains(t, s, "token=smallstep") +} + +func TestRedactSecrets(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"yubikey pin-value mid-uri", "yubikey:serial=123;pin-value=secret;slot-id=9c", "yubikey:serial=123;pin-value=REDACTED;slot-id=9c"}, + {"yubikey management-key", "yubikey:management-key=010203;pin-value=abc", "yubikey:management-key=REDACTED;pin-value=REDACTED"}, + {"pkcs11 query attribute", "pkcs11:token=step?pin-value=pass&max-sessions=2", "pkcs11:token=step?pin-value=REDACTED&max-sessions=2"}, + {"azurekms client-secret", "azurekms:name=k;vault=v?client-id=id&client-secret=hunter2&tenant-id=t", "azurekms:name=k;vault=v?client-id=id&client-secret=REDACTED&tenant-id=t"}, + {"case-insensitive key", "yubikey:PIN-VALUE=abc", "yubikey:PIN-VALUE=REDACTED"}, + {"value at end of string", "yubikey:serial=1;pin-value=xyz", "yubikey:serial=1;pin-value=REDACTED"}, + {"empty value", "yubikey:pin-value=;slot-id=9a", "yubikey:pin-value=REDACTED;slot-id=9a"}, + {"value containing equals and percent", "yubikey:pin-value=a=b%20c;slot-id=9a", "yubikey:pin-value=REDACTED;slot-id=9a"}, + {"value with leading single quote", "yubikey:pin-value='q';slot-id=9c", "yubikey:pin-value=REDACTED;slot-id=9c"}, + {"value with embedded single quote", "pkcs11:token=step?pin-value=my'pin&max-sessions=2", "pkcs11:token=step?pin-value=REDACTED&max-sessions=2"}, + {"pin-source is a path, not a secret", "yubikey:pin-source=/etc/pin.txt;management-key-source=/etc/mk.txt", "yubikey:pin-source=/etc/pin.txt;management-key-source=/etc/mk.txt"}, + {"uri quoted inside prose", `error parsing "yubikey:serial=1;pin-value=abc": invalid uri`, `error parsing "yubikey:serial=1;pin-value=REDACTED": invalid uri`}, + {"multiple uris in one string", "--kms yubikey:pin-value=aaa yubikey:slot-id=9c;pin-value=bbb", "--kms yubikey:pin-value=REDACTED yubikey:slot-id=9c;pin-value=REDACTED"}, + {"no secrets", "tpmkms:name=my-key;device=/dev/tpmrm0", "tpmkms:name=my-key;device=/dev/tpmrm0"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, redactSecrets(tt.in)) + }) + } +}