Skip to content

Commit c707f77

Browse files
andrey-noskovk8s-infra-cherrypick-robot
andrey-noskov
authored and
k8s-infra-cherrypick-robot
committed
fix: redact all query parameters in CRI error logs
Signed-off-by: Andrey Noskov <[email protected]>
1 parent 6f57386 commit c707f77

3 files changed

Lines changed: 229 additions & 0 deletions

File tree

‎internal/cri/instrument/instrumented_service.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,8 @@ func (in *instrumentedService) PullImage(ctx context.Context, r *runtime.PullIma
355355
log.G(ctx).Infof("PullImage %q", r.GetImage().GetImage())
356356
defer func() {
357357
if err != nil {
358+
// Sanitize error to remove sensitive information
359+
err = ctrdutil.SanitizeError(err)
358360
log.G(ctx).WithError(err).Errorf("PullImage %q failed", r.GetImage().GetImage())
359361
} else {
360362
log.G(ctx).Infof("PullImage %q returns image reference %q",
@@ -373,6 +375,8 @@ func (in *instrumentedService) ListImages(ctx context.Context, r *runtime.ListIm
373375
log.G(ctx).Tracef("ListImages with filter %+v", r.GetFilter())
374376
defer func() {
375377
if err != nil {
378+
// Sanitize error to remove sensitive information
379+
err = ctrdutil.SanitizeError(err)
376380
log.G(ctx).WithError(err).Errorf("ListImages with filter %+v failed", r.GetFilter())
377381
} else {
378382
log.G(ctx).Tracef("ListImages with filter %+v returns image list %+v",
@@ -390,6 +394,8 @@ func (in *instrumentedService) ImageStatus(ctx context.Context, r *runtime.Image
390394
log.G(ctx).Tracef("ImageStatus for %q", r.GetImage().GetImage())
391395
defer func() {
392396
if err != nil {
397+
// Sanitize error to remove sensitive information
398+
err = ctrdutil.SanitizeError(err)
393399
log.G(ctx).WithError(err).Errorf("ImageStatus for %q failed", r.GetImage().GetImage())
394400
} else {
395401
log.G(ctx).Tracef("ImageStatus for %q returns image status %+v",
@@ -408,6 +414,8 @@ func (in *instrumentedService) RemoveImage(ctx context.Context, r *runtime.Remov
408414
log.G(ctx).Infof("RemoveImage %q", r.GetImage().GetImage())
409415
defer func() {
410416
if err != nil {
417+
// Sanitize error to remove sensitive information
418+
err = ctrdutil.SanitizeError(err)
411419
log.G(ctx).WithError(err).Errorf("RemoveImage %q failed", r.GetImage().GetImage())
412420
} else {
413421
log.G(ctx).Infof("RemoveImage %q returns successfully", r.GetImage().GetImage())

‎internal/cri/util/sanitize.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package util
18+
19+
import (
20+
"errors"
21+
"net/url"
22+
"strings"
23+
)
24+
25+
// SanitizeError sanitizes an error by redacting sensitive information in URLs.
26+
// If the error contains a *url.Error, it parses and sanitizes the URL.
27+
// Otherwise, it returns the error unchanged.
28+
func SanitizeError(err error) error {
29+
if err == nil {
30+
return nil
31+
}
32+
33+
// Check if the error is or contains a *url.Error
34+
var urlErr *url.Error
35+
if errors.As(err, &urlErr) {
36+
// Parse and sanitize the URL
37+
sanitizedURL := sanitizeURL(urlErr.URL)
38+
if sanitizedURL != urlErr.URL {
39+
// Wrap with sanitized url.Error
40+
return &sanitizedError{
41+
original: err,
42+
sanitizedURL: sanitizedURL,
43+
urlError: urlErr,
44+
}
45+
}
46+
return err
47+
}
48+
49+
// No sanitization needed for non-URL errors
50+
return err
51+
}
52+
53+
// sanitizeURL properly parses a URL and redacts all query parameters.
54+
func sanitizeURL(rawURL string) string {
55+
parsed, err := url.Parse(rawURL)
56+
if err != nil {
57+
// If URL parsing fails, return original (malformed URLs shouldn't leak tokens)
58+
return rawURL
59+
}
60+
61+
// Check if URL has query parameters
62+
query := parsed.Query()
63+
if len(query) == 0 {
64+
return rawURL
65+
}
66+
67+
// Redact all query parameters
68+
for param := range query {
69+
query.Set(param, "[REDACTED]")
70+
}
71+
72+
// Reconstruct URL with sanitized query
73+
parsed.RawQuery = query.Encode()
74+
return parsed.String()
75+
}
76+
77+
// sanitizedError wraps an error containing a *url.Error with a sanitized URL.
78+
type sanitizedError struct {
79+
original error
80+
sanitizedURL string
81+
urlError *url.Error
82+
}
83+
84+
// Error returns the error message with the sanitized URL.
85+
func (e *sanitizedError) Error() string {
86+
// Replace all occurrences of the original URL with the sanitized version
87+
return strings.ReplaceAll(e.original.Error(), e.urlError.URL, e.sanitizedURL)
88+
}
89+
90+
// Unwrap returns the original error for error chain traversal.
91+
func (e *sanitizedError) Unwrap() error {
92+
return e.original
93+
}

‎internal/cri/util/sanitize_test.go‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
/*
2+
Copyright The containerd Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package util
18+
19+
import (
20+
"errors"
21+
"fmt"
22+
"net/url"
23+
"testing"
24+
25+
"github.com/stretchr/testify/assert"
26+
"github.com/stretchr/testify/require"
27+
)
28+
29+
func TestSanitizeError_SimpleURLError(t *testing.T) {
30+
// Create a url.Error with sensitive info
31+
originalURL := "https://storage.blob.core.windows.net/container/blob?sig=SECRET&sv=2020"
32+
urlErr := &url.Error{
33+
Op: "Get",
34+
URL: originalURL,
35+
Err: fmt.Errorf("connection timeout"),
36+
}
37+
38+
// Sanitize
39+
sanitized := SanitizeError(urlErr)
40+
require.NotNil(t, sanitized)
41+
42+
// Check it's a sanitizedError with correct properties
43+
sanitizedErr, ok := sanitized.(*sanitizedError)
44+
require.True(t, ok, "Should return *sanitizedError type")
45+
assert.Equal(t, urlErr, sanitizedErr.original)
46+
assert.Equal(t, urlErr, sanitizedErr.urlError)
47+
assert.Equal(t, "https://storage.blob.core.windows.net/container/blob?sig=%5BREDACTED%5D&sv=%5BREDACTED%5D", sanitizedErr.sanitizedURL)
48+
49+
// Test Error() method - verifies ReplaceAll functionality
50+
expected := "Get \"https://storage.blob.core.windows.net/container/blob?sig=%5BREDACTED%5D&sv=%5BREDACTED%5D\": connection timeout"
51+
assert.Equal(t, expected, sanitized.Error())
52+
}
53+
54+
func TestSanitizeError_WrappedError(t *testing.T) {
55+
originalURL := "https://storage.blob.core.windows.net/blob?sig=SECRET&sv=2020"
56+
urlErr := &url.Error{
57+
Op: "Get",
58+
URL: originalURL,
59+
Err: fmt.Errorf("timeout"),
60+
}
61+
62+
wrappedErr := fmt.Errorf("image pull failed: %w", urlErr)
63+
64+
// Sanitize
65+
sanitized := SanitizeError(wrappedErr)
66+
67+
// Test Error() method with wrapped error - verifies ReplaceAll works in wrapped context
68+
sanitizedMsg := sanitized.Error()
69+
assert.NotContains(t, sanitizedMsg, "SECRET", "Secret should be sanitized")
70+
assert.Contains(t, sanitizedMsg, "image pull failed", "Wrapper message should be preserved")
71+
assert.Contains(t, sanitizedMsg, "%5BREDACTED%5D", "Should contain sanitized marker")
72+
73+
// Should still be able to unwrap to url.Error
74+
var targetURLErr *url.Error
75+
assert.True(t, errors.As(sanitized, &targetURLErr),
76+
"Should be able to find *url.Error in sanitized error chain")
77+
78+
// Verify url.Error properties are preserved
79+
assert.Equal(t, "Get", targetURLErr.Op)
80+
assert.Contains(t, targetURLErr.Err.Error(), "timeout")
81+
}
82+
83+
func TestSanitizeError_NonURLError(t *testing.T) {
84+
// Regular error without url.Error
85+
regularErr := fmt.Errorf("some error occurred")
86+
87+
sanitized := SanitizeError(regularErr)
88+
89+
// Should return the exact same error object
90+
assert.Equal(t, regularErr, sanitized,
91+
"Non-URL errors should pass through unchanged")
92+
}
93+
94+
func TestSanitizeError_NilError(t *testing.T) {
95+
sanitized := SanitizeError(nil)
96+
assert.Nil(t, sanitized, "nil error should return nil")
97+
}
98+
99+
func TestSanitizeError_NoQueryParams(t *testing.T) {
100+
// URL without any query parameters
101+
urlErr := &url.Error{
102+
Op: "Get",
103+
URL: "https://registry.example.com/v2/image/manifests/latest",
104+
Err: fmt.Errorf("not found"),
105+
}
106+
107+
sanitized := SanitizeError(urlErr)
108+
109+
// Should return the same error object (no sanitization needed)
110+
assert.Equal(t, urlErr, sanitized,
111+
"Errors without query params should pass through unchanged")
112+
}
113+
114+
func TestSanitizedError_Unwrap(t *testing.T) {
115+
originalURL := "https://storage.blob.core.windows.net/blob?sig=SECRET"
116+
urlErr := &url.Error{
117+
Op: "Get",
118+
URL: originalURL,
119+
Err: fmt.Errorf("timeout"),
120+
}
121+
122+
sanitized := SanitizeError(urlErr)
123+
124+
// Should be able to unwrap
125+
unwrapped := errors.Unwrap(sanitized)
126+
assert.NotNil(t, unwrapped, "Should be able to unwrap sanitized error")
127+
assert.Equal(t, urlErr, unwrapped, "Unwrapped should be the original error")
128+
}

0 commit comments

Comments
 (0)