From a5d32f29da98c7b54358501c70f037758334346d Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Fri, 10 Oct 2025 12:21:29 -0700 Subject: [PATCH 1/3] Sanitize Azure HTTP responses in BSL status messages Azure storage errors include verbose HTTP response details and XML in error messages, making the BSL status.message field cluttered and hard to read. This change adds sanitization to extract only the error code and meaningful message. Before: BackupStorageLocation "test" is unavailable: rpc error: code = Unknown desc = GET https://... RESPONSE 404: 404 The specified container does not exist. ERROR CODE: ContainerNotFound After: BackupStorageLocation "test" is unavailable: rpc error: code = Unknown desc = ContainerNotFound: The specified container does not exist. AWS and GCP error messages are preserved as-is since they don't contain verbose HTTP responses. Fixes #8368 Signed-off-by: Shubham Pampattiwar --- .../backup_storage_location_controller.go | 94 ++++++++++++++++++- ...backup_storage_location_controller_test.go | 78 +++++++++++++++ 2 files changed, 171 insertions(+), 1 deletion(-) diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index ba765607d..a8edb565e 100644 --- a/pkg/controller/backup_storage_location_controller.go +++ b/pkg/controller/backup_storage_location_controller.go @@ -18,6 +18,7 @@ package controller import ( "context" + "regexp" "strings" "time" @@ -46,6 +47,97 @@ const ( bslValidationEnqueuePeriod = 10 * time.Second ) +// sanitizeStorageError cleans up verbose HTTP responses from cloud provider errors, +// particularly Azure which includes full HTTP response details and XML in error messages. +// It extracts the error code and message while removing HTTP headers and response bodies. +func sanitizeStorageError(err error) string { + if err == nil { + return "" + } + + errMsg := err.Error() + + // Check if this looks like an Azure HTTP response error + // Azure errors contain patterns like "RESPONSE 404:" and "ERROR CODE:" + if !strings.Contains(errMsg, "RESPONSE") || !strings.Contains(errMsg, "ERROR CODE:") { + // Not an Azure-style error, return as-is + return errMsg + } + + // Extract the error code (e.g., "ContainerNotFound", "BlobNotFound") + errorCodeRegex := regexp.MustCompile(`ERROR CODE:\s*(\w+)`) + errorCodeMatch := errorCodeRegex.FindStringSubmatch(errMsg) + var errorCode string + if len(errorCodeMatch) > 1 { + errorCode = errorCodeMatch[1] + } + + // Extract the error message from the XML or plain text + // Look for message between tags or after "RESPONSE XXX:" + var errorMessage string + + // Try to extract from XML first + messageRegex := regexp.MustCompile(`(.*?)`) + messageMatch := messageRegex.FindStringSubmatch(errMsg) + if len(messageMatch) > 1 { + errorMessage = messageMatch[1] + // Remove RequestId and Time from the message + if idx := strings.Index(errorMessage, "\nRequestId:"); idx != -1 { + errorMessage = errorMessage[:idx] + } + } else { + // Try to extract from plain text response (e.g., "RESPONSE 404: 404 The specified container does not exist.") + responseRegex := regexp.MustCompile(`RESPONSE\s+\d+:\s+\d+\s+([^\n]+)`) + responseMatch := responseRegex.FindStringSubmatch(errMsg) + if len(responseMatch) > 1 { + errorMessage = strings.TrimSpace(responseMatch[1]) + } + } + + // Build a clean error message + var cleanMsg string + if errorCode != "" && errorMessage != "" { + cleanMsg = errorCode + ": " + errorMessage + } else if errorCode != "" { + cleanMsg = errorCode + } else if errorMessage != "" { + cleanMsg = errorMessage + } else { + // Fallback: try to extract the desc part from gRPC error + descRegex := regexp.MustCompile(`desc\s*=\s*(.+)`) + descMatch := descRegex.FindStringSubmatch(errMsg) + if len(descMatch) > 1 { + // Take everything up to the first newline or "RESPONSE" marker + desc := descMatch[1] + if idx := strings.Index(desc, "\n"); idx != -1 { + desc = desc[:idx] + } + if idx := strings.Index(desc, "RESPONSE"); idx != -1 { + desc = strings.TrimSpace(desc[:idx]) + } + cleanMsg = desc + } else { + // Last resort: return first line + if idx := strings.Index(errMsg, "\n"); idx != -1 { + cleanMsg = errMsg[:idx] + } else { + cleanMsg = errMsg + } + } + } + + // Preserve the prefix part of the error (e.g., "rpc error: code = Unknown desc = ") + // but replace the verbose description with our clean message + if strings.Contains(errMsg, "desc = ") { + parts := strings.SplitN(errMsg, "desc = ", 2) + if len(parts) == 2 { + return parts[0] + "desc = " + cleanMsg + } + } + + return cleanMsg +} + // BackupStorageLocationReconciler reconciles a BackupStorageLocation object type backupStorageLocationReconciler struct { ctx context.Context @@ -127,7 +219,7 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr err = errors.Wrapf(err, "BackupStorageLocation %q is unavailable", location.Name) unavailableErrors = append(unavailableErrors, err.Error()) location.Status.Phase = velerov1api.BackupStorageLocationPhaseUnavailable - location.Status.Message = err.Error() + location.Status.Message = sanitizeStorageError(err) } else { log.Info("BackupStorageLocations is valid, marking as available") location.Status.Phase = velerov1api.BackupStorageLocationPhaseAvailable diff --git a/pkg/controller/backup_storage_location_controller_test.go b/pkg/controller/backup_storage_location_controller_test.go index b07c4a1fb..0ff3c3b00 100644 --- a/pkg/controller/backup_storage_location_controller_test.go +++ b/pkg/controller/backup_storage_location_controller_test.go @@ -303,3 +303,81 @@ func TestBSLReconcile(t *testing.T) { }) } } + +func TestSanitizeStorageError(t *testing.T) { + tests := []struct { + name string + input error + expected string + }{ + { + name: "Nil error", + input: nil, + expected: "", + }, + { + name: "Simple error without Azure formatting", + input: errors.New("simple error message"), + expected: "simple error message", + }, + { + name: "AWS style error", + input: errors.New("NoSuchBucket: The specified bucket does not exist"), + expected: "NoSuchBucket: The specified bucket does not exist", + }, + { + name: "Azure container not found error with full HTTP response", + input: errors.New(`rpc error: code = Unknown desc = GET https://oadp100711zl59k.blob.core.windows.net/oadp100711zl59k1 +-------------------------------------------------------------------------------- +RESPONSE 404: 404 The specified container does not exist. +ERROR CODE: ContainerNotFound +-------------------------------------------------------------------------------- +ContainerNotFoundThe specified container does not exist. +RequestId:63cf34d8-801e-0078-09b4-2e4682000000 +Time:2024-11-04T12:23:04.5623627Z +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = ContainerNotFound: The specified container does not exist.", + }, + { + name: "Azure blob not found error", + input: errors.New(`rpc error: code = Unknown desc = GET https://storage.blob.core.windows.net/container/blob +-------------------------------------------------------------------------------- +RESPONSE 404: 404 The specified blob does not exist. +ERROR CODE: BlobNotFound +-------------------------------------------------------------------------------- +BlobNotFoundThe specified blob does not exist. +RequestId:12345678-1234-1234-1234-123456789012 +Time:2024-11-04T12:23:04.5623627Z +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = BlobNotFound: The specified blob does not exist.", + }, + { + name: "Azure error with plain text response (no XML)", + input: errors.New(`rpc error: code = Unknown desc = GET https://storage.blob.core.windows.net/container +-------------------------------------------------------------------------------- +RESPONSE 404: 404 The specified container does not exist. +ERROR CODE: ContainerNotFound +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = ContainerNotFound: The specified container does not exist.", + }, + { + name: "Azure error without XML message but with error code", + input: errors.New(`rpc error: code = Unknown desc = operation failed +RESPONSE 403: 403 Forbidden +ERROR CODE: AuthorizationFailure +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = AuthorizationFailure: Forbidden", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual := sanitizeStorageError(test.input) + assert.Equal(t, test.expected, actual) + }) + } +} From 60dd3dc832296bad7bcd517f77c33bcc8fb3a24a Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Fri, 10 Oct 2025 12:25:30 -0700 Subject: [PATCH 2/3] Add changelog for PR #9321 Signed-off-by: Shubham Pampattiwar --- changelogs/unreleased/9321-shubham-pampattiwar | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/9321-shubham-pampattiwar diff --git a/changelogs/unreleased/9321-shubham-pampattiwar b/changelogs/unreleased/9321-shubham-pampattiwar new file mode 100644 index 000000000..7fd9cec10 --- /dev/null +++ b/changelogs/unreleased/9321-shubham-pampattiwar @@ -0,0 +1 @@ +Sanitize Azure HTTP responses in BSL status messages From 20af2c20c5b41b0245f6eb8d51853a4761ac3583 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 12 Nov 2025 15:39:38 -0800 Subject: [PATCH 3/3] Address PR review comments: sanitize errors and add SAS token scrubbing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses three review comments on PR #9321: 1. Keep sanitization in controller (response to @ywk253100) - Maintaining centralized error handling for easier extension - Azure-specific patterns detected and others passed through unchanged 2. Sanitize unavailableErrors array (@priyansh17) - Now using sanitizeStorageError() for both unavailableErrors array and location.Status.Message for consistency 3. Add SAS token scrubbing (@anshulahuja98) - Scrubs Azure SAS token parameters to prevent credential leakage - Redacts: sig, se, st, sp, spr, sv, sr, sip, srt, ss - Example: ?sig=secret becomes ?sig=***REDACTED*** Added comprehensive test coverage for SAS token scrubbing with 4 new test cases covering various scenarios. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: Shubham Pampattiwar --- .../backup_storage_location_controller.go | 9 ++++- ...backup_storage_location_controller_test.go | 34 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/pkg/controller/backup_storage_location_controller.go b/pkg/controller/backup_storage_location_controller.go index a8edb565e..2e43a6187 100644 --- a/pkg/controller/backup_storage_location_controller.go +++ b/pkg/controller/backup_storage_location_controller.go @@ -50,6 +50,7 @@ const ( // sanitizeStorageError cleans up verbose HTTP responses from cloud provider errors, // particularly Azure which includes full HTTP response details and XML in error messages. // It extracts the error code and message while removing HTTP headers and response bodies. +// It also scrubs sensitive information like SAS tokens from URLs. func sanitizeStorageError(err error) string { if err == nil { return "" @@ -57,6 +58,12 @@ func sanitizeStorageError(err error) string { errMsg := err.Error() + // Scrub sensitive information from URLs (SAS tokens, credentials, etc.) + // Azure SAS token parameters: sig, se, st, sp, spr, sv, sr, sip, srt, ss + // These appear as query parameters in URLs like: ?sig=value&se=value + sasParamsRegex := regexp.MustCompile(`([?&])(sig|se|st|sp|spr|sv|sr|sip|srt|ss)=([^&\s<>\n]+)`) + errMsg = sasParamsRegex.ReplaceAllString(errMsg, `${1}${2}=***REDACTED***`) + // Check if this looks like an Azure HTTP response error // Azure errors contain patterns like "RESPONSE 404:" and "ERROR CODE:" if !strings.Contains(errMsg, "RESPONSE") || !strings.Contains(errMsg, "ERROR CODE:") { @@ -217,7 +224,7 @@ func (r *backupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr if err != nil { log.Info("BackupStorageLocation is invalid, marking as unavailable") err = errors.Wrapf(err, "BackupStorageLocation %q is unavailable", location.Name) - unavailableErrors = append(unavailableErrors, err.Error()) + unavailableErrors = append(unavailableErrors, sanitizeStorageError(err)) location.Status.Phase = velerov1api.BackupStorageLocationPhaseUnavailable location.Status.Message = sanitizeStorageError(err) } else { diff --git a/pkg/controller/backup_storage_location_controller_test.go b/pkg/controller/backup_storage_location_controller_test.go index 0ff3c3b00..8a5dc4a4a 100644 --- a/pkg/controller/backup_storage_location_controller_test.go +++ b/pkg/controller/backup_storage_location_controller_test.go @@ -372,6 +372,40 @@ ERROR CODE: AuthorizationFailure `), expected: "rpc error: code = Unknown desc = AuthorizationFailure: Forbidden", }, + { + name: "Error with Azure SAS token in URL", + input: errors.New(`rpc error: code = Unknown desc = GET https://storage.blob.core.windows.net/backup?sv=2020-08-04&sig=abc123secrettoken&se=2024-12-31T23:59:59Z&sp=rwdl +-------------------------------------------------------------------------------- +RESPONSE 404: 404 The specified container does not exist. +ERROR CODE: ContainerNotFound +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = ContainerNotFound: The specified container does not exist.", + }, + { + name: "Error with multiple SAS parameters", + input: errors.New(`GET https://mystorageaccount.blob.core.windows.net/container?sv=2020-08-04&ss=b&srt=sco&sp=rwdlac&se=2024-12-31&st=2024-01-01&sip=168.1.5.60&spr=https&sig=SIGNATURE_HASH`), + expected: "GET https://mystorageaccount.blob.core.windows.net/container?sv=***REDACTED***&ss=***REDACTED***&srt=***REDACTED***&sp=***REDACTED***&se=***REDACTED***&st=***REDACTED***&sip=***REDACTED***&spr=***REDACTED***&sig=***REDACTED***", + }, + { + name: "Simple URL without SAS tokens unchanged", + input: errors.New("GET https://storage.blob.core.windows.net/container/blob"), + expected: "GET https://storage.blob.core.windows.net/container/blob", + }, + { + name: "Azure error with SAS token in full HTTP response", + input: errors.New(`rpc error: code = Unknown desc = GET https://oadp100711zl59k.blob.core.windows.net/backup?sig=secretsignature123&se=2024-12-31 +-------------------------------------------------------------------------------- +RESPONSE 404: 404 The specified container does not exist. +ERROR CODE: ContainerNotFound +-------------------------------------------------------------------------------- +ContainerNotFoundThe specified container does not exist. +RequestId:63cf34d8-801e-0078-09b4-2e4682000000 +Time:2024-11-04T12:23:04.5623627Z +-------------------------------------------------------------------------------- +`), + expected: "rpc error: code = Unknown desc = ContainerNotFound: The specified container does not exist.", + }, } for _, test := range tests {