From 7bb946ff8e81f97bcfe96922b7e6a716377e984a Mon Sep 17 00:00:00 2001 From: Ethan Mosbaugh Date: Thu, 3 Sep 2020 14:00:16 +0000 Subject: [PATCH 1/3] No stack trace in registry auth errors --- kotsadm/go.mod | 2 ++ kotsadm/go.sum | 3 +++ kotsadm/pkg/handlers/errors.go | 11 +++++++++ kotsadm/pkg/handlers/registry.go | 39 +++++++++++++++++++++--------- kotsadm/pkg/registry/registry.go | 4 ++- pkg/docker/registry/auth.go | 2 +- pkg/docker/registry/errors.go | 28 ++++++++++++++++----- pkg/docker/registry/errors_test.go | 30 +++++++++++++++++++++++ 8 files changed, 99 insertions(+), 20 deletions(-) create mode 100644 pkg/docker/registry/errors_test.go diff --git a/kotsadm/go.mod b/kotsadm/go.mod index d864b148d8..2c0c32924e 100644 --- a/kotsadm/go.mod +++ b/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 diff --git a/kotsadm/go.sum b/kotsadm/go.sum index 0fe64b4eb1..8ec2bc3b73 100644 --- a/kotsadm/go.sum +++ b/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= diff --git a/kotsadm/pkg/handlers/errors.go b/kotsadm/pkg/handlers/errors.go index 045a516917..e95a669d7e 100644 --- a/kotsadm/pkg/handlers/errors.go +++ b/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, + } } diff --git a/kotsadm/pkg/handlers/registry.go b/kotsadm/pkg/handlers/registry.go index 94cca34b63..4596022241 100644 --- a/kotsadm/pkg/handlers/registry.go +++ b/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,9 +275,7 @@ 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 @@ -266,17 +283,15 @@ func ValidateAppRegistry(w http.ResponseWriter, r *http.Request) { } 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 } diff --git a/kotsadm/pkg/registry/registry.go b/kotsadm/pkg/registry/registry.go index bda65320b7..fd9dacaca7 100644 --- a/kotsadm/pkg/registry/registry.go +++ b/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) } } diff --git a/pkg/docker/registry/auth.go b/pkg/docker/registry/auth.go index 8e1aa33448..f5183cb829 100644 --- a/pkg/docker/registry/auth.go +++ b/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) diff --git a/pkg/docker/registry/errors.go b/pkg/docker/registry/errors.go index b56c86d991..1035efb63b 100644 --- a/pkg/docker/registry/errors.go +++ b/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 := ®istryErrors{} - 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 := ®istryErrors{} + _ = 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 } diff --git a/pkg/docker/registry/errors_test.go b/pkg/docker/registry/errors_test.go new file mode 100644 index 0000000000..1c7c1ecf2c --- /dev/null +++ b/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) + } + }) + } +} From 461fac0314fb59045d6532b81ff15d4dad8ea68c Mon Sep 17 00:00:00 2001 From: Ethan Mosbaugh Date: Thu, 3 Sep 2020 14:00:30 +0000 Subject: [PATCH 2/3] Fix docker auth test --- pkg/docker/registry/auth.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/docker/registry/auth.go b/pkg/docker/registry/auth.go index f5183cb829..157b4a0816 100644 --- a/pkg/docker/registry/auth.go +++ b/pkg/docker/registry/auth.go @@ -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 } From e44f70238d8f4395df01caa65e699180857620f7 Mon Sep 17 00:00:00 2001 From: Ethan Mosbaugh Date: Thu, 3 Sep 2020 16:25:39 +0000 Subject: [PATCH 3/3] Handle ecr image rewrite --- pkg/image/builder.go | 44 +++++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/pkg/image/builder.go b/pkg/image/builder.go index d78abc38dd..d7b7a27815 100644 --- a/pkg/image/builder.go +++ b/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,7 +476,7 @@ 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{ @@ -468,19 +484,21 @@ func CopyFromFileToRegistry(path string, name string, tag string, digest string, } 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, } }