Skip to content

Commit 495d69a

Browse files
committed
fix(teststate): redact SSH key pairs and stop leaking secrets via the overwrite warning
SaveRedacted suppressed the marshalled JSON but not the overwrite warning, which renders the value with %v, so saving a secret over an existing file still wrote it to the log. Render a placeholder there when the value is redacted, and cover it with a test. ssh.SaveSSHKeyPair wrote KeyPair.PrivateKey to the log because it used Save rather than SaveRedacted. This predates the refactor, but the refactor introduced SaveRedacted and applied it to the aws sibling, so fix it here. Also drop t.Parallel from the aws redaction test, which swaps the package level logger.Default that the rest of that package reads, add direct tests for core/teststate, and register teststate with the external consumer gate.
1 parent 8a8d7d2 commit 495d69a

6 files changed

Lines changed: 255 additions & 7 deletions

File tree

modules/aws/save_test_data_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ func (l *tStringLogger) Logf(t gotesting.TestingT, format string, args ...any) {
2424
l.sb.WriteRune('\n')
2525
}
2626

27+
// Not parallel: this test swaps the package-level logger.Default, which every other test in this package reads.
28+
// Running it alongside them is a data race.
2729
func TestSaveAndLoadEC2KeyPair(t *testing.T) {
28-
t.Parallel()
29-
3030
def, slogger := logger.Default, &tStringLogger{}
3131
logger.Default = logger.New(slogger)
3232

modules/core/teststate/teststate.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// and reused in later validation and teardown stages.
33
//
44
// These primitives are type agnostic. Modules that own a type typically wrap them in a named helper (for example,
5-
// terraform.SaveOptions or aws.SaveEc2KeyPair) so that callers do not have to spell out paths or filenames. Callers
5+
// aws.SaveEc2KeyPair or k8s.SaveKubectlOptions) so that callers do not have to spell out paths or filenames. Callers
66
// with types that have no owning module can use Save and Load directly.
77
//
88
// This package lives in core rather than teststructure so that modules such as aws, k8s, packer, and ssh can provide
@@ -23,6 +23,9 @@ import (
2323
// DirName is the name of the folder, relative to a test folder, in which test data is stored.
2424
const DirName = ".test-data"
2525

26+
// redactedPlaceholder stands in for a value whose contents must not reach the test log.
27+
const redactedPlaceholder = "[REDACTED]"
28+
2629
// FormatPath formats a path to save test data with the given filename in the given test folder.
2730
func FormatPath(testFolder string, filename string) string {
2831
return filepath.Join(testFolder, DirName, filename)
@@ -50,11 +53,18 @@ func SaveRedacted(t testing.TestingT, path string, overwrite bool, value any) {
5053
func save(t testing.TestingT, path string, overwrite bool, value any, loggedVal bool) {
5154
logger.Default.Logf(t, "Storing test data in %s so it can be reused later", path)
5255

56+
// The overwrite warnings render the value, so a redacted save must show a placeholder instead. Otherwise a
57+
// secret suppressed from the "Marshalled JSON" line below would still reach the log through this branch.
58+
loggedRepr := any(redactedPlaceholder)
59+
if loggedVal {
60+
loggedRepr = value
61+
}
62+
5363
if IsPresent(t, path) {
5464
if overwrite {
55-
logger.Default.Logf(t, "[WARNING] The named test data at path %s is non-empty. Save operation will overwrite existing value with \"%v\".\n.", path, value)
65+
logger.Default.Logf(t, "[WARNING] The named test data at path %s is non-empty. Save operation will overwrite existing value with \"%v\".\n.", path, loggedRepr)
5666
} else {
57-
logger.Default.Logf(t, "[WARNING] The named test data at path %s is non-empty. Skipping save operation to prevent overwriting existing value with \"%v\".\n.", path, value)
67+
logger.Default.Logf(t, "[WARNING] The named test data at path %s is non-empty. Skipping save operation to prevent overwriting existing value with \"%v\".\n.", path, loggedRepr)
5868

5969
return
6070
}
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
package teststate_test
2+
3+
import (
4+
"fmt"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/gruntwork-io/terratest/modules/core/v2/logger"
10+
"github.com/gruntwork-io/terratest/modules/core/v2/teststate"
11+
gotesting "github.com/gruntwork-io/terratest/modules/core/v2/testing"
12+
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
14+
)
15+
16+
// tStringLogger captures everything written to the logger so a test can assert on what was, and was not, logged.
17+
type tStringLogger struct {
18+
sb strings.Builder
19+
}
20+
21+
func (l *tStringLogger) Logf(t gotesting.TestingT, format string, args ...any) {
22+
t.Helper()
23+
fmt.Fprintf(&l.sb, format, args...)
24+
l.sb.WriteRune('\n')
25+
}
26+
27+
// captureLog swaps logger.Default for the duration of the test. Tests using it must not call t.Parallel.
28+
func captureLog(t *testing.T) *tStringLogger {
29+
t.Helper()
30+
31+
def, slogger := logger.Default, &tStringLogger{}
32+
logger.Default = logger.New(slogger)
33+
34+
t.Cleanup(func() { logger.Default = def })
35+
36+
return slogger
37+
}
38+
39+
func TestFormatPath(t *testing.T) {
40+
t.Parallel()
41+
42+
assert.Equal(t, filepath.Join("/foo", ".test-data", "Bar.json"), teststate.FormatPath("/foo", "Bar.json"))
43+
}
44+
45+
func TestSaveAndLoadRoundTrip(t *testing.T) {
46+
t.Parallel()
47+
48+
type payload struct {
49+
Name string
50+
Count int
51+
}
52+
53+
path := teststate.FormatPath(t.TempDir(), "Payload.json")
54+
expected := payload{Name: "terratest", Count: 3}
55+
56+
teststate.Save(t, path, true, expected)
57+
58+
var actual payload
59+
60+
teststate.Load(t, path, &actual)
61+
assert.Equal(t, expected, actual)
62+
}
63+
64+
func TestSaveOverwriteSemantics(t *testing.T) {
65+
t.Parallel()
66+
67+
path := teststate.FormatPath(t.TempDir(), "Value.json")
68+
69+
teststate.Save(t, path, true, "first")
70+
71+
// overwrite=false must leave the existing value alone.
72+
teststate.Save(t, path, false, "second")
73+
74+
var got string
75+
76+
teststate.Load(t, path, &got)
77+
assert.Equal(t, "first", got, "overwrite=false must not clobber an existing value")
78+
79+
// overwrite=true must replace it.
80+
teststate.Save(t, path, true, "third")
81+
teststate.Load(t, path, &got)
82+
assert.Equal(t, "third", got)
83+
}
84+
85+
// TestSaveLogsValueAndSaveRedactedDoesNot is the behavioural contract that callers holding secrets depend on.
86+
func TestSaveLogsValueAndSaveRedactedDoesNot(t *testing.T) {
87+
const secret = "-----BEGIN RSA PRIVATE KEY-----sentinel-----END RSA PRIVATE KEY-----"
88+
89+
t.Run("Save logs the marshalled value", func(t *testing.T) {
90+
slogger := captureLog(t)
91+
teststate.Save(t, teststate.FormatPath(t.TempDir(), "Plain.json"), true, secret)
92+
assert.Contains(t, slogger.sb.String(), secret, "Save is expected to log the marshalled JSON")
93+
})
94+
95+
t.Run("SaveRedacted does not", func(t *testing.T) {
96+
slogger := captureLog(t)
97+
teststate.SaveRedacted(t, teststate.FormatPath(t.TempDir(), "Secret.json"), true, secret)
98+
assert.NotContains(t, slogger.sb.String(), secret, "SaveRedacted must not log the marshalled JSON")
99+
})
100+
}
101+
102+
// TestSaveRedactedDoesNotLeakViaOverwriteWarning covers the second log statement in the save path. The overwrite
103+
// warning renders the value with %v, which is not suppressed by the redacted flag, so a redacted save over an
104+
// existing file could still leak. Values reaching SaveRedacted are secrets by definition.
105+
func TestSaveRedactedDoesNotLeakViaOverwriteWarning(t *testing.T) {
106+
const secret = "-----BEGIN RSA PRIVATE KEY-----sentinel-----END RSA PRIVATE KEY-----"
107+
108+
path := teststate.FormatPath(t.TempDir(), "Secret.json")
109+
110+
// First save creates the file, so the second one takes the overwrite-warning branch.
111+
teststate.SaveRedacted(t, path, true, secret)
112+
113+
slogger := captureLog(t)
114+
teststate.SaveRedacted(t, path, true, secret)
115+
116+
assert.NotContains(t, slogger.sb.String(), secret,
117+
"the overwrite warning must not render a redacted value")
118+
}
119+
120+
func TestIsPresent(t *testing.T) {
121+
t.Parallel()
122+
123+
path := teststate.FormatPath(t.TempDir(), "Maybe.json")
124+
assert.False(t, teststate.IsPresent(t, path), "a missing file is not present")
125+
126+
teststate.Save(t, path, true, "value")
127+
assert.True(t, teststate.IsPresent(t, path))
128+
129+
// An empty JSON value counts as absent, so a stage can re-create it.
130+
teststate.Save(t, path, true, "")
131+
assert.False(t, teststate.IsPresent(t, path), "an empty value counts as absent")
132+
}
133+
134+
func TestIsEmptyJSON(t *testing.T) {
135+
t.Parallel()
136+
137+
testCases := []struct {
138+
name string
139+
bytes string
140+
empty bool
141+
}{
142+
{"no bytes", "", true},
143+
{"null", "null", true},
144+
{"false", "false", true},
145+
{"zero", "0", true},
146+
{"empty string", `""`, true},
147+
{"empty array", "[]", true},
148+
{"empty object", "{}", true},
149+
{"true", "true", false},
150+
{"non-zero", "42", false},
151+
{"string", `"value"`, false},
152+
{"array", `[1]`, false},
153+
{"object", `{"a":1}`, false},
154+
}
155+
156+
for _, testCase := range testCases {
157+
t.Run(testCase.name, func(t *testing.T) {
158+
t.Parallel()
159+
assert.Equal(t, testCase.empty, teststate.IsEmptyJSON(t, []byte(testCase.bytes)))
160+
})
161+
}
162+
}
163+
164+
func TestCleanup(t *testing.T) {
165+
t.Parallel()
166+
167+
folder := t.TempDir()
168+
path := teststate.FormatPath(folder, "Value.json")
169+
170+
teststate.Save(t, path, true, "value")
171+
require.FileExists(t, path)
172+
173+
teststate.Cleanup(t, path)
174+
assert.NoFileExists(t, path)
175+
176+
// Cleaning up an already-absent path is a no-op, not a failure.
177+
assert.NotPanics(t, func() { teststate.Cleanup(t, path) })
178+
}
179+
180+
func TestCleanupFolder(t *testing.T) {
181+
t.Parallel()
182+
183+
folder := t.TempDir()
184+
teststate.Save(t, teststate.FormatPath(folder, "One.json"), true, "1")
185+
teststate.Save(t, teststate.FormatPath(folder, "Two.json"), true, "2")
186+
187+
require.NoError(t, teststate.CleanupFolderE(t, folder))
188+
assert.NoDirExists(t, filepath.Join(folder, ".test-data"))
189+
190+
// Cleaning an absent folder is a no-op.
191+
require.NoError(t, teststate.CleanupFolderE(t, folder))
192+
}

modules/ssh/save_test_data.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@ const sshKeyPairFilename = "SshKeyPair.json"
1010

1111
// SaveSSHKeyPair serializes and saves an SSH key pair into the given folder. This allows you to create an SSH key pair
1212
// during setup and to reuse that key pair later during validation and teardown.
13+
//
14+
// The key pair is saved with teststate.SaveRedacted so that KeyPair.PrivateKey is not written to the test log.
1315
func SaveSSHKeyPair(t testing.TestingT, testFolder string, keyPair *KeyPair) {
14-
teststate.Save(t, formatSSHKeyPairPath(testFolder), true, keyPair)
16+
teststate.SaveRedacted(t, formatSSHKeyPairPath(testFolder), true, keyPair)
1517
}
1618

1719
// LoadSSHKeyPair loads and unserializes an SSH key pair from the given folder. This allows you to reuse an SSH key pair

modules/ssh/save_test_data_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,28 @@
11
package ssh_test
22

33
import (
4+
"fmt"
5+
"strings"
46
"testing"
57

8+
"github.com/gruntwork-io/terratest/modules/core/v2/logger"
9+
gotesting "github.com/gruntwork-io/terratest/modules/core/v2/testing"
610
"github.com/gruntwork-io/terratest/modules/ssh/v2"
711
"github.com/stretchr/testify/assert"
812
"github.com/stretchr/testify/require"
913
)
1014

15+
// tStringLogger captures everything written to the logger so a test can assert on what was, and was not, logged.
16+
type tStringLogger struct {
17+
sb strings.Builder
18+
}
19+
20+
func (l *tStringLogger) Logf(t gotesting.TestingT, format string, args ...any) {
21+
t.Helper()
22+
fmt.Fprintf(&l.sb, format, args...)
23+
l.sb.WriteRune('\n')
24+
}
25+
1126
func TestSaveAndLoadSSHKeyPair(t *testing.T) {
1227
t.Parallel()
1328

@@ -20,3 +35,32 @@ func TestSaveAndLoadSSHKeyPair(t *testing.T) {
2035
actualData := ssh.LoadSSHKeyPair(t, tmpFolder)
2136
assert.Equal(t, expectedData, actualData)
2237
}
38+
39+
// TestSaveSSHKeyPairDoesNotLogPrivateKey pins that SaveSSHKeyPair redacts the marshalled value. KeyPair.PrivateKey
40+
// is a PEM private key, and the test log is routinely captured by CI, so it must never appear there.
41+
//
42+
// Not parallel: this test swaps the package-level logger.Default, which other tests in this package read.
43+
func TestSaveSSHKeyPairDoesNotLogPrivateKey(t *testing.T) {
44+
def, slogger := logger.Default, &tStringLogger{}
45+
logger.Default = logger.New(slogger)
46+
47+
t.Cleanup(func() {
48+
logger.Default = def
49+
})
50+
51+
keyPair, err := ssh.GenerateRSAKeyPairE(t, 2048) //nolint:mnd // RSA key size for testing
52+
require.NoError(t, err)
53+
54+
tmpFolder := t.TempDir()
55+
ssh.SaveSSHKeyPair(t, tmpFolder, keyPair)
56+
57+
logged := slogger.sb.String()
58+
assert.NotContains(t, logged, keyPair.PrivateKey, "the private key must never be logged")
59+
assert.NotContains(t, logged, "PRIVATE KEY", "no PEM block should reach the log")
60+
61+
// Confirm the logger really was wired up, so the assertions above are not vacuous.
62+
assert.Contains(t, logged, "Storing test data in", "the save operation should still be logged")
63+
64+
// The key pair must still round-trip; redaction applies to the log, not to the file.
65+
assert.Equal(t, keyPair, ssh.LoadSSHKeyPair(t, tmpFolder))
66+
}

scripts/check-release-mode.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ consumer_imports() {
3636
for s in $ORDER; do
3737
if [ "$s" = core ]; then
3838
# Keep in sync with core/v2's public leaf packages.
39-
for pkg in random files formatting logger shell retry testing; do
39+
for pkg in random files formatting logger shell retry testing teststate; do
4040
echo " _ \"$MODULE_BASE/core/v2/$pkg\""
4141
done
4242
else

0 commit comments

Comments
 (0)