Redact sensitive information in redirected URLs

internal/redact/redact.go

@@ -17,6 +17,8 @@ package redact
 
 import (
 	"context"
+	"errors"
+	"net/url"
 )
 
 type contextKey string
@@ -33,3 +35,55 @@ func FromContext(ctx context.Context) (bool, string) {
 	reason, ok := ctx.Value(redactKey).(string)
 	return ok, reason
 }
+
+// Error redacts potentially sensitive query parameter values in the URL from the error's message.
+//
+// If the error is a *url.Error, this returns a *url.Error with the URL redacted.
+// Any other error type, or nil, is returned unchanged.
+func Error(err error) error {
+	// If the error is a url.Error, we can redact the URL.
+	// Otherwise (including if err is nil), we can't redact.
+	var uerr *url.Error
+	if ok := errors.As(err, &uerr); !ok {
+		return err
+	}
+	u, perr := url.Parse(uerr.URL)
+	if perr != nil {
+		return err // If the URL can't be parsed, just return the original error.
+	}
+	uerr.URL = URL(u).String() // Update the URL to the redacted URL.
+	return uerr
+}
+
+// The set of query string keys that we expect to send as part of the registry
+// protocol. Anything else is potentially dangerous to leak, as it's probably
+// from a redirect. These redirects often included tokens or signed URLs.
+var paramAllowlist = map[string]struct{}{
+	// Token exchange
+	"scope":   {},
+	"service": {},
+	// Cross-repo mounting
+	"mount": {},
+	"from":  {},
+	// Layer PUT
+	"digest": {},
+	// Listing tags and catalog
+	"n":    {},
+	"last": {},
+}
+
+// URL redacts potentially sensitive query parameter values from the URL's query string.
+func URL(u *url.URL) *url.URL {
+	qs := u.Query()
+	for k, v := range qs {
+		for i := range v {
+			if _, ok := paramAllowlist[k]; !ok {
+				// key is not in the Allowlist
+				v[i] = "REDACTED"
+			}
+		}
+	}
+	r := *u
+	r.RawQuery = qs.Encode()
+	return &r
+}

pkg/v1/remote/descriptor.go

@@ -24,6 +24,7 @@ import (
 	"net/url"
 	"strings"
 
+	"github.com/google/go-containerregistry/internal/redact"
 	"github.com/google/go-containerregistry/internal/verify"
 	"github.com/google/go-containerregistry/pkg/logs"
 	"github.com/google/go-containerregistry/pkg/name"
@@ -367,7 +368,7 @@ func (f *fetcher) fetchBlob(ctx context.Context, size int64, h v1.Hash) (io.Read
 
 	resp, err := f.Client.Do(req.WithContext(ctx))
 	if err != nil {
-		return nil, err
+		return nil, redact.Error(err)
 	}
 
 	if err := transport.CheckError(resp, http.StatusOK); err != nil {
@@ -398,7 +399,7 @@ func (f *fetcher) headBlob(h v1.Hash) (*http.Response, error) {
 
 	resp, err := f.Client.Do(req.WithContext(f.context))
 	if err != nil {
-		return nil, err
+		return nil, redact.Error(err)
 	}
 
 	if err := transport.CheckError(resp, http.StatusOK); err != nil {
@@ -418,7 +419,7 @@ func (f *fetcher) blobExists(h v1.Hash) (bool, error) {
 
 	resp, err := f.Client.Do(req.WithContext(f.context))
 	if err != nil {
-		return false, err
+		return false, redact.Error(err)
 	}
 	defer resp.Body.Close()
 

pkg/v1/remote/descriptor_test.go

@@ -15,6 +15,7 @@
 package remote
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -25,6 +26,7 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 )
 
@@ -216,3 +218,42 @@ func TestHead_MissingHeaders(t *testing.T) {
 		}
 	}
 }
+
+// TestRedactFetchBlob tests that a request to fetchBlob that gets redirected
+// to a URL that contains sensitive information has that information redacted
+// if the subsequent request fails.
+func TestRedactFetchBlob(t *testing.T) {
+	ctx := context.Background()
+	f := fetcher{
+		Ref: mustNewTag(t, "original.com/repo:latest"),
+		Client: &http.Client{
+			Transport: errTransport{},
+		},
+		context: ctx,
+	}
+	h, err := v1.NewHash("sha256:0000000000000000000000000000000000000000000000000000000000000000")
+	if err != nil {
+		t.Fatal("NewHash:", err)
+	}
+	if _, err := f.fetchBlob(ctx, 0, h); err == nil {
+		t.Fatalf("fetchBlob: expected error, got nil")
+	} else if !strings.Contains(err.Error(), "access_token=REDACTED") {
+		t.Fatalf("fetchBlob: expected error to contain redacted access token, got %v", err)
+	}
+}
+
+type errTransport struct{}
+
+func (errTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	// This simulates a registry that returns a redirect upon the first
+	// request, and then returns an error upon subsequent requests. This helps
+	// test whether error redaction takes into account URLs in error messasges
+	// that are not the original request URL.
+	if req.URL.Host == "original.com" {
+		return &http.Response{
+			StatusCode: http.StatusSeeOther,
+			Header:     http.Header{"Location": []string{"https://redirected.com?access_token=SECRET"}},
+		}, nil
+	}
+	return nil, fmt.Errorf("error reaching %s", req.URL.String())
+}

pkg/v1/remote/transport/error.go

@@ -19,26 +19,10 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
 	"strings"
-)
 
-// The set of query string keys that we expect to send as part of the registry
-// protocol. Anything else is potentially dangerous to leak, as it's probably
-// from a redirect. These redirects often included tokens or signed URLs.
-var paramAllowlist = map[string]struct{}{
-	// Token exchange
-	"scope":   {},
-	"service": {},
-	// Cross-repo mounting
-	"mount": {},
-	"from":  {},
-	// Layer PUT
-	"digest": {},
-	// Listing tags and catalog
-	"n":    {},
-	"last": {},
-}
+	"github.com/google/go-containerregistry/internal/redact"
+)
 
 // Error implements error to support the following error specification:
 // https://github.com/docker/distribution/blob/master/docs/spec/api.md#errors
@@ -59,7 +43,7 @@ var _ error = (*Error)(nil)
 func (e *Error) Error() string {
 	prefix := ""
 	if e.Request != nil {
-		prefix = fmt.Sprintf("%s %s: ", e.Request.Method, redactURL(e.Request.URL))
+		prefix = fmt.Sprintf("%s %s: ", e.Request.Method, redact.URL(e.Request.URL))
 	}
 	return prefix + e.responseErr()
 }
@@ -100,22 +84,6 @@ func (e *Error) Temporary() bool {
 	return true
 }
 
-// TODO(jonjohnsonjr): Consider moving to internal/redact.
-func redactURL(original *url.URL) *url.URL {
-	qs := original.Query()
-	for k, v := range qs {
-		for i := range v {
-			if _, ok := paramAllowlist[k]; !ok {
-				// key is not in the Allowlist
-				v[i] = "REDACTED"
-			}
-		}
-	}
-	redacted := *original
-	redacted.RawQuery = qs.Encode()
-	return &redacted
-}
-
 // Diagnostic represents a single error returned by a Docker registry interaction.
 type Diagnostic struct {
 	Code    ErrorCode   `json:"code"`