Skip to content

Commit c47845a

Browse files
committed
pkg/cli: Implement redaction validation and user confirmation for debug zip uploads
- Added `validateRedactionStatus` function to check the redaction status of debug zips based on the `debug_zip_command_flags.txt` file. - Introduced user prompts for confirmation when redaction is unclear or not enabled. - Updated `runDebugZipUpload` to validate redaction status before proceeding with uploads. This change improves the safety and user awareness regarding sensitive data in debug zip uploads to Datadog. Part of: CRDB-49203 Epic: CRDB-52093 Release Notes: None
1 parent 0777857 commit c47845a

File tree

5 files changed

+237
-3
lines changed

5 files changed

+237
-3
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
--concurrency=15 --cpu-profile-duration=5s --files-from=2025-06-15 08:45:34 --files-until=2025-06-18 08:45:34 --include-goroutine-stacks=true --include-range-info=true --include-running-job-traces=true --redact=true

pkg/cli/zip.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ type zipRequest struct {
4848
pathName string
4949
}
5050

51+
const (
52+
debugZipCommandFlagsFileName = "debug_zip_command_flags.txt"
53+
)
54+
5155
type debugZipContext struct {
5256
z *zipper
5357
clusterPrinter *zipReporter
@@ -415,7 +419,7 @@ done
415419
return filter
416420
})
417421

418-
if err := z.createRaw(s, zc.prefix+"/debug_zip_command_flags.txt", []byte(flags)); err != nil {
422+
if err := z.createRaw(s, zc.prefix+"/"+debugZipCommandFlagsFileName, []byte(flags)); err != nil {
419423
return err
420424
}
421425

pkg/cli/zip_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,7 +1234,7 @@ func TestCommandFlags(t *testing.T) {
12341234
}
12351235

12361236
for _, f := range r.File {
1237-
if f.Name == "debug/debug_zip_command_flags.txt" {
1237+
if f.Name == "debug/"+debugZipCommandFlagsFileName {
12381238
rc, err := f.Open()
12391239
if err != nil {
12401240
t.Fatal(err)
@@ -1251,7 +1251,7 @@ func TestCommandFlags(t *testing.T) {
12511251
return
12521252
}
12531253
}
1254-
assert.Fail(t, "debug/debug_zip_command_flags.txt is not generated")
1254+
assert.Fail(t, "debug/"+debugZipCommandFlagsFileName+" is not generated")
12551255

12561256
if err = r.Close(); err != nil {
12571257
t.Fatal(err)

pkg/cli/zip_upload.go

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package cli
77

88
import (
9+
"bufio"
910
"bytes"
1011
"compress/gzip"
1112
"context"
@@ -37,6 +38,7 @@ import (
3738
"github.com/cockroachdb/cockroach/pkg/util/system"
3839
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
3940
"github.com/cockroachdb/errors"
41+
"github.com/cockroachdb/errors/oserror"
4042
"github.com/spf13/cobra"
4143
"golang.org/x/oauth2/google"
4244
"google.golang.org/api/option"
@@ -202,13 +204,114 @@ func uploadJSONFile(fileName string, message any, uuid string) error {
202204
// pipeline which enriches the logs with more fields.
203205
var defaultDDTags = []string{"service:CRDB-SH", "env:debug", "source:cockroachdb"}
204206

207+
// buildRedactionWarning creates a warning message about sensitive data in debug zips.
208+
// It includes a common list of sensitive data types that may be present.
209+
func buildRedactionWarning(prefix string) string {
210+
return prefix +
211+
"This means it may contain sensitive data including:\n" +
212+
" • Personally Identifiable Information (PII)\n" +
213+
" • Database credentials and connection strings\n" +
214+
" • Internal cluster details\n" +
215+
" • Potentially sensitive log data\n\n" +
216+
"It is advisable to only upload redacted debug zips to Datadog.\n"
217+
}
218+
219+
// promptUserForConfirmationImpl shows a warning message and prompts the user for confirmation.
220+
// It returns nil if the user confirms, or an error if they decline or if there's an input error.
221+
// In dry-run mode, it shows the warning but skips the prompt.
222+
func promptUserForConfirmationImpl(warningMsg string) error {
223+
// Skip interactive prompt in dry-run mode
224+
if debugZipUploadOpts.dryRun {
225+
fmt.Fprintf(os.Stderr, "%s", warningMsg)
226+
fmt.Fprintf(os.Stderr, "DRY RUN: Would prompt for confirmation here.\n")
227+
return nil
228+
}
229+
230+
fmt.Fprintf(os.Stderr, "%s", warningMsg)
231+
fmt.Fprintf(os.Stderr, "Do you want to continue with the upload? (y/N): ")
232+
233+
reader := bufio.NewReader(os.Stdin)
234+
line, err := reader.ReadString('\n')
235+
if err != nil {
236+
return fmt.Errorf("failed to read user input: %w", err)
237+
}
238+
239+
line = strings.ToLower(strings.TrimSpace(line))
240+
if len(line) == 0 {
241+
line = "n" // Default to 'no' when user presses enter without input
242+
}
243+
244+
if line == "y" || line == "yes" {
245+
fmt.Fprintf(os.Stderr, "Proceeding with upload...\n")
246+
return nil
247+
}
248+
249+
return fmt.Errorf("upload aborted")
250+
}
251+
252+
// promptUserForConfirmation is a variable that can be mocked in tests
253+
var promptUserForConfirmation = promptUserForConfirmationImpl
254+
255+
// validateRedactionStatus checks if the debug zip was created with redaction enabled
256+
// by examining the debugZipCommandFlagsFileName file in the system tenant.
257+
// If redaction is not enabled, it warns the user and prompts for confirmation.
258+
//
259+
// User experience examples:
260+
//
261+
// 1. Redacted zip (--redact=true): Proceeds silently
262+
// 2. Unredacted zip (--redact=false): Shows warning and prompts:
263+
// "⚠️ WARNING: Your debug zip was created WITHOUT redaction..."
264+
// "Do you want to continue with the upload? (y/N): "
265+
// 3. Unknown redaction status: Shows warning and prompts similarly
266+
//
267+
// The function respects dry-run mode by showing warnings without prompting.
268+
func validateRedactionStatus(debugDirPath string) error {
269+
flagsFilePath := path.Join(debugDirPath, debugZipCommandFlagsFileName)
270+
271+
flagsContent, err := os.ReadFile(flagsFilePath)
272+
if err != nil {
273+
if oserror.IsNotExist(err) {
274+
// File doesn't exist - we can't determine redaction status, so warn and prompt
275+
warningMsg := buildRedactionWarning(
276+
"⚠️ WARNING: The debug zip redaction status is unclear.\n")
277+
278+
return promptUserForConfirmation(warningMsg)
279+
}
280+
return fmt.Errorf("⚠️ error: the debug zip redaction status is unclear, err: %w", err)
281+
}
282+
283+
flagsStr := string(flagsContent)
284+
285+
if strings.Contains(flagsStr, "--redact=true") {
286+
return nil
287+
}
288+
289+
var warningMsg string
290+
if strings.Contains(flagsStr, "--redact=false") {
291+
warningMsg = buildRedactionWarning(
292+
"⚠️ WARNING: The debug zip was created WITHOUT redaction.\n",
293+
)
294+
} else {
295+
warningMsg = buildRedactionWarning(
296+
"⚠️ WARNING: The debug zip redaction status is unclear.\n",
297+
)
298+
}
299+
300+
return promptUserForConfirmation(warningMsg)
301+
}
302+
205303
func runDebugZipUpload(cmd *cobra.Command, args []string) error {
206304
runtime.GOMAXPROCS(system.NumCPU())
207305

208306
if err := validateZipUploadReadiness(); err != nil {
209307
return err
210308
}
211309

310+
// Check redaction status before proceeding with upload
311+
if err := validateRedactionStatus(args[0]); err != nil {
312+
return err
313+
}
314+
212315
// a unique ID for this upload session. This should be used to tag all the artifacts uploaded in this session
213316
uploadID := newUploadID(debugZipUploadOpts.clusterName, getCurrentTime())
214317

pkg/cli/zip_upload_test.go

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,11 @@ func TestUploadZipEndToEnd(t *testing.T) {
164164

165165
defer testutils.TestingHook(&gcsLogUpload, writeLogsToGCSHook)()
166166

167+
// Mock the interactive prompt to always accept (return nil) for end-to-end tests
168+
defer testutils.TestingHook(&promptUserForConfirmation, func(warningMsg string) error {
169+
return nil // Always accept in end-to-end tests
170+
})()
171+
167172
datadriven.Walk(t, "testdata/upload", func(t *testing.T, path string) {
168173
datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string {
169174
c := NewCLITest(TestCLIParams{})
@@ -281,6 +286,127 @@ func TestAppendUserTags(t *testing.T) {
281286
}
282287
}
283288

289+
func TestValidateRedactionStatus(t *testing.T) {
290+
defer leaktest.AfterTest(t)()
291+
292+
tests := []struct {
293+
name string
294+
flagsContent string
295+
fileExists bool
296+
dryRun bool
297+
expectError bool
298+
errorContains string
299+
description string
300+
}{
301+
{
302+
name: "redacted zip with other flags proceeds silently",
303+
flagsContent: " --concurrency=15 --cpu-profile-duration=5s --files-from=2025-06-15 08:45:34 --redact=true --include-range-info=true",
304+
fileExists: true,
305+
dryRun: false,
306+
expectError: false,
307+
description: "When --redact=true is found, no warnings should be shown and upload proceeds",
308+
},
309+
{
310+
name: "missing file user declines",
311+
flagsContent: "",
312+
fileExists: false,
313+
dryRun: false,
314+
expectError: true,
315+
errorContains: "upload aborted",
316+
description: "When file is missing and user declines, should return cancellation error",
317+
},
318+
{
319+
name: "missing file user accepts",
320+
flagsContent: "",
321+
fileExists: false,
322+
dryRun: false,
323+
expectError: false,
324+
errorContains: "user accepts",
325+
description: "When file is missing and user accepts, should proceed without error",
326+
},
327+
{
328+
name: "unredacted zip user declines",
329+
flagsContent: " --concurrency=1 --redact=false --nodes=1",
330+
fileExists: true,
331+
dryRun: false,
332+
expectError: true,
333+
errorContains: "upload aborted",
334+
description: "When --redact=false and user declines, should return cancellation error",
335+
},
336+
{
337+
name: "unredacted zip user accepts",
338+
flagsContent: " --concurrency=1 --redact=false --nodes=1",
339+
fileExists: true,
340+
dryRun: false,
341+
expectError: false,
342+
errorContains: "user accepts",
343+
description: "When --redact=false and user accepts, should proceed without error",
344+
},
345+
{
346+
name: "unclear redaction status user declines",
347+
flagsContent: " --concurrency=1 --nodes=1",
348+
fileExists: true,
349+
dryRun: false,
350+
expectError: true,
351+
errorContains: "upload aborted",
352+
description: "When no --redact flag and user declines, should return cancellation error",
353+
},
354+
{
355+
name: "unclear redaction status user accepts",
356+
flagsContent: " --concurrency=1 --nodes=1",
357+
fileExists: true,
358+
dryRun: false,
359+
expectError: false,
360+
errorContains: "user accepts",
361+
description: "When no --redact flag and user accepts, should proceed without error",
362+
},
363+
}
364+
365+
for _, test := range tests {
366+
t.Run(test.name, func(t *testing.T) {
367+
tempDir := t.TempDir()
368+
369+
if test.fileExists {
370+
flagsFile := path.Join(tempDir, debugZipCommandFlagsFileName)
371+
if err := os.WriteFile(flagsFile, []byte(test.flagsContent), 0644); err != nil {
372+
t.Fatal(err)
373+
}
374+
}
375+
376+
originalDryRun := debugZipUploadOpts.dryRun
377+
debugZipUploadOpts.dryRun = test.dryRun
378+
defer func() { debugZipUploadOpts.dryRun = originalDryRun }()
379+
380+
if test.errorContains == "upload aborted" || test.errorContains == "user accepts" {
381+
originalPrompt := promptUserForConfirmation
382+
switch test.errorContains {
383+
case "upload aborted":
384+
promptUserForConfirmation = func(warningMsg string) error {
385+
require.Contains(t, warningMsg, "WARNING:")
386+
return fmt.Errorf("upload aborted")
387+
}
388+
case "user accepts":
389+
promptUserForConfirmation = func(warningMsg string) error {
390+
require.Contains(t, warningMsg, "WARNING:")
391+
return nil
392+
}
393+
}
394+
defer func() { promptUserForConfirmation = originalPrompt }()
395+
}
396+
397+
// Test validation
398+
err := validateRedactionStatus(tempDir)
399+
400+
if test.expectError {
401+
require.Error(t, err, fmt.Sprintf("Test case: %s", test.description))
402+
require.Contains(t, err.Error(), test.errorContains, fmt.Sprintf("Test case: %s", test.description))
403+
} else {
404+
require.NoError(t, err, fmt.Sprintf("Test case: %s", test.description))
405+
}
406+
})
407+
}
408+
}
409+
284410
func TestZipUploadArtifactTypes(t *testing.T) {
285411
defer leaktest.AfterTest(t)()
286412

0 commit comments

Comments
 (0)