Merge pull request #1057 from replicatedhq/registry-stack-errors

No stack trace in registry auth errors

kotsadm/go.mod

@@ -10,9 +10,11 @@ require (
 	github.com/aws/aws-sdk-go v1.28.2
 	github.com/bitnami-labs/sealed-secrets v0.12.5
 	github.com/containerd/containerd v1.3.2
+	github.com/containers/image/v5 v5.2.0
 	github.com/coreos/etcd v3.3.13+incompatible
 	github.com/deislabs/oras v0.8.1
 	github.com/dgrijalva/jwt-go v3.2.0+incompatible
+	github.com/docker/distribution v2.7.1+incompatible
 	github.com/docker/go-units v0.4.0
 	github.com/go-logfmt/logfmt v0.4.0
 	github.com/gorilla/mux v1.7.4

kotsadm/go.sum

@@ -229,6 +229,9 @@ github.com/containerd/ttrpc v0.0.0-20190828154514-0e0f228740de/go.mod h1:PvCDdDG
 github.com/containerd/typeurl v0.0.0-20180627222232-a93fcdb778cd/go.mod h1:Cm3kwCdlkCfMSHURc+r6fwoGH6/F1hH3S4sg0rLFWPc=
 github.com/containerd/typeurl v0.0.0-20190228175220-2a93cfde8c20/go.mod h1:Cm3kwCdlkCfMSHURc+r6fwoGH6/F1hH3S4sg0rLFWPc=
 github.com/containernetworking/cni v0.7.1/go.mod h1:LGwApLUm2FpoOfxTDEeq8T9ipbpZ61X79hmU3w8FmsY=
+github.com/containers/image v1.5.1 h1:ssEuj1c24uJvdMkUa2IrawuEFZBP12p6WzrjNBTQxE0=
+github.com/containers/image v3.0.2+incompatible h1:B1lqAE8MUPCrsBLE86J0gnXleeRq8zJnQryhiiGQNyE=
+github.com/containers/image v3.0.2+incompatible/go.mod h1:8Vtij257IWSanUQKe1tAeNOm2sRVkSqQTVQ1IlwI3+M=
 github.com/containers/image/v5 v5.2.0 h1:DowY5OII5x9Pb6Pt76vnHU79BgG4/jdwhZjeAj2R+t8=
 github.com/containers/image/v5 v5.2.0/go.mod h1:IAub4gDGvXoxaIAdNy4e3FbVTDPVNMv9F0UfVVFbYCU=
 github.com/containers/libtrust v0.0.0-20190913040956-14b96171aa3b h1:Q8ePgVfHDplZ7U33NwHZkrVELsZP5fYj9pM5WBZB2GE=

kotsadm/pkg/handlers/errors.go

@@ -1,6 +1,17 @@
 package handlers
 
+import "github.com/pkg/errors"
+
 type ErrorResponse struct {
 	Error   string `json:"error"`
 	Success bool   `json:"success"` // NOTE: the frontend relies on this for some routes
+	Err     error  `json:"-"`
+}
+
+func NewErrorResponse(err error) ErrorResponse {
+	return ErrorResponse{
+		Error:   errors.Cause(err).Error(),
+		Success: false,
+		Err:     err,
+	}
 }

kotsadm/pkg/handlers/registry.go

@@ -3,7 +3,11 @@ package handlers
 import (
 	"encoding/json"
 	"net/http"
+	"net/url"
 
+	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/containers/image/v5/docker"
+	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
 	"github.com/replicatedhq/kots/kotsadm/pkg/logger"
@@ -137,9 +141,24 @@ func UpdateAppRegistry(w http.ResponseWriter, r *http.Request) {
 	// in a goroutine, start pushing the images to the remote registry
 	// we will let this function return while this happens
 	go func() {
-		if err := registry.RewriteImages(foundApp.ID, foundApp.CurrentSequence, updateAppRegistryRequest.Hostname, updateAppRegistryRequest.Username, updateAppRegistryRequest.Password,
-			updateAppRegistryRequest.Namespace, nil); err != nil {
-			logger.Error(err)
+		if err := registry.RewriteImages(
+			foundApp.ID, foundApp.CurrentSequence, updateAppRegistryRequest.Hostname,
+			updateAppRegistryRequest.Username, updateAppRegistryRequest.Password,
+			updateAppRegistryRequest.Namespace, nil,
+		); err != nil {
+			// log credential errors at info level
+			causeErr := errors.Cause(err)
+			switch causeErr.(type) {
+			case docker.ErrUnauthorizedForCredentials, errcode.Errors, errcode.Error, awserr.Error, *url.Error:
+				logger.Infof(
+					"Failed to rewrite images for host %q and username %q: %v",
+					updateAppRegistryRequest.Hostname,
+					updateAppRegistryRequest.Username,
+					causeErr,
+				)
+			default:
+				logger.Error(err)
+			}
 			return
 		}
 
@@ -256,27 +275,23 @@ func ValidateAppRegistry(w http.ResponseWriter, r *http.Request) {
 
 			if kotsadmSettings.Hostname != validateAppRegistryRequest.Hostname || kotsadmSettings.Password == "" {
 				err := errors.Errorf("no password found for %s", validateAppRegistryRequest.Hostname)
-				logger.Error(err)
-				validateAppRegistryResponse.Error = err.Error()
-				JSON(w, 400, validateAppRegistryResponse)
+				JSON(w, 400, NewErrorResponse(err))
 				return
 			}
 			password = kotsadmSettings.Password
 		}
 	}
 	if password == "" || password == registrytypes.PasswordMask {
 		err := errors.Errorf("no password found for %s", validateAppRegistryRequest.Hostname)
-		logger.Error(err)
-		validateAppRegistryResponse.Error = err.Error()
-		JSON(w, 400, validateAppRegistryResponse)
+		JSON(w, 400, NewErrorResponse(err))
 		return
 	}
 
 	err = dockerregistry.TestPushAccess(validateAppRegistryRequest.Hostname, validateAppRegistryRequest.Username, password, validateAppRegistryRequest.Namespace)
 	if err != nil {
-		logger.Error(err)
-		validateAppRegistryResponse.Error = err.Error()
-		JSON(w, 500, validateAppRegistryResponse)
+		// NOTE: it is possible this is a 500 sometimes
+		logger.Infof("Failed to test push access: %v", err)
+		JSON(w, 400, NewErrorResponse(err))
 		return
 	}
 

kotsadm/pkg/registry/registry.go

@@ -55,7 +55,9 @@ func RewriteImages(appID string, sequence int64, hostname string, username strin
 				logger.Error(err)
 			}
 		} else {
-			if err := store.GetStore().SetTaskStatus("image-rewrite", finalError.Error(), "failed"); err != nil {
+			// do not show the stack trace to the user
+			causeErr := errors.Cause(finalError)
+			if err := store.GetStore().SetTaskStatus("image-rewrite", causeErr.Error(), "failed"); err != nil {
 				logger.Error(err)
 			}
 		}

pkg/docker/registry/auth.go

@@ -113,7 +113,7 @@ func TestPushAccess(endpoint, username, password, org string) error {
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		return errors.New(errorResponseToString(authBody))
+		return errors.New(errorResponseToString(resp.StatusCode, authBody))
 	}
 
 	bearerToken, err := newBearerTokenFromJSONBlob(authBody)
@@ -166,5 +166,8 @@ func sanitizeEndpoint(endpoint string) string {
 	endpoint = strings.TrimSuffix(endpoint, "/v2")
 	endpoint = strings.TrimSuffix(endpoint, "/v1/")
 	endpoint = strings.TrimSuffix(endpoint, "/v1")
+	if endpoint == "docker.io" {
+		endpoint = "index.docker.io"
+	}
 	return endpoint
 }

pkg/docker/registry/errors.go

@@ -12,17 +12,22 @@ type registryError struct {
 	Detail  string `json:"detail"`
 }
 
+type dockerIOError struct {
+	Details string `json:"details,omitempty"`
+}
+
 type registryErrors struct {
 	Errors []registryError `json:"errors"`
 }
 
-func errorResponseToString(response []byte) string {
-	e := &registryErrors{}
-	err := json.Unmarshal(response, e)
-	if err != nil {
-		return string(response)
+func errorResponseToString(statusCode int, response []byte) string {
+	if len(response) == 0 {
+		return fmt.Sprintf("unexpected status code %d", statusCode)
 	}
 
+	e := &registryErrors{}
+	_ = json.Unmarshal(response, e)
+
 	messages := make([]string, 0)
 	for _, err := range e.Errors {
 		if err.Message != "" && err.Detail != "" {
@@ -36,5 +41,16 @@ func errorResponseToString(response []byte) string {
 		}
 	}
 
-	return strings.Join(messages, "\n")
+	ee := &dockerIOError{}
+	_ = json.Unmarshal(response, ee)
+	if ee.Details != "" {
+		messages = append(messages, ee.Details)
+	}
+
+	errResponse := strings.Join(messages, "\n")
+	if errResponse == "" {
+		return string(response)
+	}
+
+	return errResponse
 }

pkg/docker/registry/errors_test.go

@@ -0,0 +1,30 @@
+package registry
+
+import "testing"
+
+func Test_errorResponseToString(t *testing.T) {
+	tests := []struct {
+		name       string
+		statusCode int
+		response   string
+		want       string
+	}{
+		{
+			name:     "quay.io",
+			response: `{"errors":[{"code":"UNAUTHORIZED","detail":{},"message":"Invalid Username or Password"}]}`,
+			want:     "Invalid Username or Password",
+		},
+		{
+			name:     "docker.io",
+			response: `{"details":"incorrect username or password"}`,
+			want:     "incorrect username or password",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := errorResponseToString(tt.statusCode, []byte(tt.response)); got != tt.want {
+				t.Errorf("errorResponseToString() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

pkg/image/builder.go

@@ -314,17 +314,33 @@ func copyOneImage(srcRegistry, destRegistry registry.RegistryOptions, image stri
 		return nil, errors.Wrapf(err, "failed to parse source image name %s", sourceImage)
 	}
 
+	destStr := fmt.Sprintf("docker://%s", DestRef(destRegistry, image))
+	destRef, err := alltransports.ParseImageName(destStr)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to parse dest image name %s", destStr)
+	}
+
 	destCtx := &types.SystemContext{
 		DockerInsecureSkipTLSVerify: types.OptionalBoolTrue,
 	}
-	destCtx.DockerAuthConfig = &types.DockerAuthConfig{
-		Username: destRegistry.Username,
-		Password: destRegistry.Password,
-	}
 
-	destRef, err := alltransports.ParseImageName(fmt.Sprintf("docker://%s", DestRef(destRegistry, image)))
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to parse dest image name %s", DestRef(destRegistry, image))
+	if destRegistry.Username != "" && destRegistry.Password != "" {
+		username, password := destRegistry.Username, destRegistry.Password
+
+		registryHost := reference.Domain(destRef.DockerReference())
+		if registry.IsECREndpoint(registryHost) {
+			login, err := registry.GetECRLogin(registryHost, username, password)
+			if err != nil {
+				return nil, errors.Wrap(err, "failed to get ECR login")
+			}
+			username = login.Username
+			password = login.Password
+		}
+
+		destCtx.DockerAuthConfig = &types.DockerAuthConfig{
+			Username: username,
+			Password: password,
+		}
 	}
 
 	if dryRun {
@@ -460,27 +476,29 @@ func CopyFromFileToRegistry(path string, name string, tag string, digest string,
 	destStr := fmt.Sprintf("docker://%s:%s", name, tag)
 	destRef, err := alltransports.ParseImageName(destStr)
 	if err != nil {
-		return errors.Wrapf(err, "failed to parse dest image name: %s", destStr)
+		return errors.Wrapf(err, "failed to parse dest image name %s", destStr)
 	}
 
 	destCtx := &types.SystemContext{
 		DockerInsecureSkipTLSVerify: types.OptionalBoolTrue,
 	}
 
 	if auth.Username != "" && auth.Password != "" {
+		username, password := auth.Username, auth.Password
+
 		registryHost := reference.Domain(destRef.DockerReference())
 		if registry.IsECREndpoint(registryHost) {
-			login, err := registry.GetECRLogin(registryHost, auth.Username, auth.Password)
+			login, err := registry.GetECRLogin(registryHost, username, password)
 			if err != nil {
 				return errors.Wrap(err, "failed to get ECR login")
 			}
-			auth.Username = login.Username
-			auth.Password = login.Password
+			username = login.Username
+			password = login.Password
 		}
 
 		destCtx.DockerAuthConfig = &types.DockerAuthConfig{
-			Username: auth.Username,
-			Password: auth.Password,
+			Username: username,
+			Password: password,
 		}
 	}