Redact sensitive information in redirected URLs (#1408)

* Redact sensitive information in redirected URLs

This adds internal methods to redact potentially sensitive information
from URLs in error messages, especially when those URLs are the result
of server-side redirects.

We already redact potentially sensitive information like this as part of
transport.CheckError, but this isn't called when the error is the result
of an http.Client.Do (e.g., tcp dial error).

The specific use case where this can happen is a registry like GCR which
redirects blob requests to GCS with a sensitive access_token in the
query parameter. If the request to GCS fails due to tcp error, the error
message will include the sensitive access token.

This method of redaction relies on the original error being a
*url.Error, and redaction is accomplished by simply updating the error's
URL with a redacted equivalent.

* review feedback

Author: imjasonh

Diff for: 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
+}

Diff for: 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()
 

Diff for: 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())
+}

Diff for: 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"`