From 39e8b95ef549a4d6da3fee3290de921af079ea8d Mon Sep 17 00:00:00 2001 From: Boris Bera Date: Fri, 11 Mar 2022 07:50:50 -0500 Subject: [PATCH 1/5] Implement both the Add and Delete operations as nops These two operations are called when docker tries to store and delete credentials (docker login and docker logout) respectively. This change makes it so that both of these operations are implemented as nops instead of returning a "not implemented" error. The goal here is to ensure compatibilities with applications and tools that call docker login or docker logout for the user as part of their normal operations. --- ecr-login/ecr.go | 28 +++++++++++++++++++--------- ecr-login/ecr_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/ecr-login/ecr.go b/ecr-login/ecr.go index 461e3090..a374ca8b 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -14,7 +14,6 @@ package ecr import ( - "errors" "fmt" "io" @@ -24,8 +23,6 @@ import ( "github.com/docker/docker-credential-helpers/credentials" ) -var notImplemented = errors.New("not implemented") - type ECRHelper struct { clientFactory api.ClientFactory logger *logrus.Logger @@ -70,14 +67,27 @@ func NewECRHelper(opts ...Option) *ECRHelper { // ensure ECRHelper adheres to the credentials.Helper interface var _ credentials.Helper = (*ECRHelper)(nil) -func (ECRHelper) Add(creds *credentials.Credentials) error { - // This does not seem to get called - return notImplemented +// Called when docker tries to store credentials. This usually happens during `docker login` calls. In our context, +// storing arbitrary user given credentials makes no sense. To keep interoperability with applications that call `docker +// login` during their execution, this is implemented as a nop. +func (self ECRHelper) Add(creds *credentials.Credentials) error { + self.logger.Warningf( + "Ignoring request to store credentials for %s@%s. "+ + "This is not supported in the context of the docker ecr-login helper."+ + creds.Username, + creds.ServerURL) + return nil } -func (ECRHelper) Delete(serverURL string) error { - // This does not seem to get called - return notImplemented +// Called when docker tries to delete credentials. This usually happens during `docker logout` calls. In our context, we +// don't store arbitrary user given credentials so deleting them makes no sense. To keep interoperability with +// applications that call `docker logout` during their execution, this is implemented as a nop. +func (self ECRHelper) Delete(serverURL string) error { + self.logger.Warningf( + "Ignoring request to delete credentials for %s. "+ + "This is not supported in the context of the docker ecr-login helper.", + serverURL) + return nil } func (self ECRHelper) Get(serverURL string) (string, string, error) { diff --git a/ecr-login/ecr_test.go b/ecr-login/ecr_test.go index e7365065..bcd7f8e1 100644 --- a/ecr-login/ecr_test.go +++ b/ecr-login/ecr_test.go @@ -118,3 +118,27 @@ func TestListFailure(t *testing.T) { assert.Error(t, err) assert.Len(t, serverList, 0) } + +func TestAddNop(t *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + err := helper.Add(&credentials.Credentials{ + ServerURL: "123456789.dkr.ecr.us-east-1.amazonaws.com", + Username: "AWS", + Secret: "supersecret", + }) + + assert.Nil(t, err) +} + +func TestDeleteNop(t *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + err := helper.Delete("123456789.dkr.ecr.us-east-1.amazonaws.com") + + assert.Nil(t, err) +} From 6eb364e04b85c0b7597cd27b8f63321c10a15225 Mon Sep 17 00:00:00 2001 From: Boris Bera Date: Fri, 11 Mar 2022 07:57:51 -0500 Subject: [PATCH 2/5] Fix typo in add's nop log --- ecr-login/ecr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ecr-login/ecr.go b/ecr-login/ecr.go index a374ca8b..32026a93 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -73,8 +73,8 @@ var _ credentials.Helper = (*ECRHelper)(nil) func (self ECRHelper) Add(creds *credentials.Credentials) error { self.logger.Warningf( "Ignoring request to store credentials for %s@%s. "+ - "This is not supported in the context of the docker ecr-login helper."+ - creds.Username, + "This is not supported in the context of the docker ecr-login helper.", + creds.Username, creds.ServerURL) return nil } From 2540a1caf20e959351767611ecf0e40fb6a937c7 Mon Sep 17 00:00:00 2001 From: Boris Bera Date: Mon, 14 Mar 2022 08:43:49 -0400 Subject: [PATCH 3/5] Use WithField when logging warning --- ecr-login/ecr.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ecr-login/ecr.go b/ecr-login/ecr.go index 32026a93..0522d32d 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -71,11 +71,11 @@ var _ credentials.Helper = (*ECRHelper)(nil) // storing arbitrary user given credentials makes no sense. To keep interoperability with applications that call `docker // login` during their execution, this is implemented as a nop. func (self ECRHelper) Add(creds *credentials.Credentials) error { - self.logger.Warningf( - "Ignoring request to store credentials for %s@%s. "+ - "This is not supported in the context of the docker ecr-login helper.", - creds.Username, - creds.ServerURL) + self.logger. + WithField("username", creds.Username). + WithField("serverURL", creds.ServerURL). + Warning("Ignoring request to store credentials. " + + "This is not supported in the context of the docker ecr-login helper.") return nil } @@ -83,10 +83,10 @@ func (self ECRHelper) Add(creds *credentials.Credentials) error { // don't store arbitrary user given credentials so deleting them makes no sense. To keep interoperability with // applications that call `docker logout` during their execution, this is implemented as a nop. func (self ECRHelper) Delete(serverURL string) error { - self.logger.Warningf( - "Ignoring request to delete credentials for %s. "+ - "This is not supported in the context of the docker ecr-login helper.", - serverURL) + self.logger. + WithField("serverURL", serverURL). + Warning("Ignoring request to delete credentials. " + + "This is not supported in the context of the docker ecr-login helper.") return nil } From 2c6ec845ce2687ad527fccb186607934d8a424cc Mon Sep 17 00:00:00 2001 From: Boris Bera Date: Mon, 14 Mar 2022 09:29:23 -0400 Subject: [PATCH 4/5] Gate docker creds ignore feature behind env var --- README.md | 15 +++++----- ecr-login/ecr.go | 44 +++++++++++++++++++---------- ecr-login/ecr_test.go | 65 ++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index c8548cb3..d6d4df58 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ Docker to work with the helper. To build and install the Amazon ECR Docker Credential Helper, we suggest Go 1.19 or later, `git` and `make` installed on your system. -If you just installed Go, make sure you also have added it to your PATH or +If you just installed Go, make sure you also have added it to your PATH or Environment Vars (Windows). For example: ``` @@ -213,7 +213,7 @@ include: * An [IAM role for Amazon EC2](http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html) To use credentials associated with a different named profile in the shared credentials file (`~/.aws/credentials`), you -may set the `AWS_PROFILE` environment variable. +may set the `AWS_PROFILE` environment variable. The Amazon ECR Docker Credential Helper reads and supports some configuration options specified in the AWS shared configuration file (`~/.aws/config`). To disable these options, you must set the `AWS_SDK_LOAD_CONFIG` environment @@ -236,12 +236,13 @@ in the *AWS Command Line Interface User Guide*. The credentials must have a policy applied that [allows access to Amazon ECR](http://docs.aws.amazon.com/AmazonECR/latest/userguide/ecr_managed_policies.html). -### Amazon ECR Docker Credential Helper +### Amazon ECR Docker Credential Helper -| Environment Variable | Sample Value | Description | -| --------------------- | ------------- | ------------------------------------------------------------------ | -| AWS_ECR_DISABLE_CACHE | true | Disables the local file auth cache if set to a non-empty value | -| AWS_ECR_CACHE_DIR | ~/.ecr | Specifies the local file auth cache directory location | +| Environment Variable | Sample Value | Description | +| ---------------------------- | ------------- | ------------------------------------------------------------------ | +| AWS_ECR_DISABLE_CACHE | true | Disables the local file auth cache if set to a non-empty value | +| AWS_ECR_CACHE_DIR | ~/.ecr | Specifies the local file auth cache directory location | +| AWS_ECR_IGNORE_CREDS_STORAGE | true | Ignore calls to docker login or logout and pretend they succeeded | ## Usage diff --git a/ecr-login/ecr.go b/ecr-login/ecr.go index 0522d32d..5ae1aef4 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -14,8 +14,10 @@ package ecr import ( + "errors" "fmt" "io" + "os" "github.com/sirupsen/logrus" @@ -23,6 +25,8 @@ import ( "github.com/docker/docker-credential-helpers/credentials" ) +var notImplemented = errors.New("not implemented") + type ECRHelper struct { clientFactory api.ClientFactory logger *logrus.Logger @@ -67,27 +71,37 @@ func NewECRHelper(opts ...Option) *ECRHelper { // ensure ECRHelper adheres to the credentials.Helper interface var _ credentials.Helper = (*ECRHelper)(nil) +func shouldIgnoreCredsStorage() bool { + return os.Getenv("AWS_ECR_IGNORE_CREDS_STORAGE") == "true" +} + // Called when docker tries to store credentials. This usually happens during `docker login` calls. In our context, -// storing arbitrary user given credentials makes no sense. To keep interoperability with applications that call `docker -// login` during their execution, this is implemented as a nop. +// storing arbitrary user given credentials makes no sense. func (self ECRHelper) Add(creds *credentials.Credentials) error { - self.logger. - WithField("username", creds.Username). - WithField("serverURL", creds.ServerURL). - Warning("Ignoring request to store credentials. " + - "This is not supported in the context of the docker ecr-login helper.") - return nil + if shouldIgnoreCredsStorage() { + self.logger. + WithField("username", creds.Username). + WithField("serverURL", creds.ServerURL). + Warning("Ignoring request to store credentials. " + + "This is not supported in the context of the docker ecr-login helper.") + return nil + } else { + return notImplemented + } } // Called when docker tries to delete credentials. This usually happens during `docker logout` calls. In our context, we -// don't store arbitrary user given credentials so deleting them makes no sense. To keep interoperability with -// applications that call `docker logout` during their execution, this is implemented as a nop. +// don't store arbitrary user given credentials so deleting them makes no sense. func (self ECRHelper) Delete(serverURL string) error { - self.logger. - WithField("serverURL", serverURL). - Warning("Ignoring request to delete credentials. " + - "This is not supported in the context of the docker ecr-login helper.") - return nil + if shouldIgnoreCredsStorage() { + self.logger. + WithField("serverURL", serverURL). + Warning("Ignoring request to delete credentials. " + + "This is not supported in the context of the docker ecr-login helper.") + return nil + } else { + return notImplemented + } } func (self ECRHelper) Get(serverURL string) (string, string, error) { diff --git a/ecr-login/ecr_test.go b/ecr-login/ecr_test.go index bcd7f8e1..9003ceb1 100644 --- a/ecr-login/ecr_test.go +++ b/ecr-login/ecr_test.go @@ -16,6 +16,7 @@ package ecr import ( "errors" "fmt" + "os" "testing" ecr "github.com/awslabs/amazon-ecr-credential-helper/ecr-login/api" @@ -119,13 +120,14 @@ func TestListFailure(t *testing.T) { assert.Len(t, serverList, 0) } -func TestAddNop(t *testing.T) { +func TestAddIgnored(t *testing.T) { factory := &mock_api.MockClientFactory{} helper := NewECRHelper(WithClientFactory(factory)) + os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "true") err := helper.Add(&credentials.Credentials{ - ServerURL: "123456789.dkr.ecr.us-east-1.amazonaws.com", + ServerURL: proxyEndpoint, Username: "AWS", Secret: "supersecret", }) @@ -133,12 +135,67 @@ func TestAddNop(t *testing.T) { assert.Nil(t, err) } -func TestDeleteNop(t *testing.T) { +func TestAddNotImplemented(t *testing.T) { + tests := []struct { + name string + setEnv func() + }{ + {"unset", func() { os.Unsetenv("AWS_ECR_IGNORE_CREDS_STORAGE") }}, + {"false", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }}, + {"0", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }}, + {"empty string", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "") }}, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + test.setEnv() + err := helper.Add(&credentials.Credentials{ + ServerURL: proxyEndpoint, + Username: "AWS", + Secret: "supersecret", + }) + + assert.Error(t, err, "not implemented") + }) + } +} + +func TestDeleteIgnored(t *testing.T) { factory := &mock_api.MockClientFactory{} helper := NewECRHelper(WithClientFactory(factory)) - err := helper.Delete("123456789.dkr.ecr.us-east-1.amazonaws.com") + os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "true") + err := helper.Delete(proxyEndpoint) assert.Nil(t, err) } + +func TestDeleteNotImplemented(t *testing.T) { + tests := []struct { + name string + setEnv func() + }{ + {"unset", func() { os.Unsetenv("AWS_ECR_IGNORE_CREDS_STORAGE") }}, + {"false", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }}, + {"0", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }}, + {"empty string", func() { os.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "") }}, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + test.setEnv() + err := helper.Delete(proxyEndpoint) + + assert.Error(t, err, "not implemented") + }) + } +} From c2915f706cf86d454b301645885ff26c88daa23a Mon Sep 17 00:00:00 2001 From: Boris Bera Date: Tue, 25 Jul 2023 17:14:24 -0400 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Gavin Inglis <43075615+ginglis13@users.noreply.github.com> --- ecr-login/ecr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ecr-login/ecr.go b/ecr-login/ecr.go index 5ae1aef4..1a49eddd 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -75,7 +75,7 @@ func shouldIgnoreCredsStorage() bool { return os.Getenv("AWS_ECR_IGNORE_CREDS_STORAGE") == "true" } -// Called when docker tries to store credentials. This usually happens during `docker login` calls. In our context, +// Add tries to store credentials when docker requests it. This usually happens during `docker login` calls. In our context, // storing arbitrary user given credentials makes no sense. func (self ECRHelper) Add(creds *credentials.Credentials) error { if shouldIgnoreCredsStorage() { @@ -90,7 +90,7 @@ func (self ECRHelper) Add(creds *credentials.Credentials) error { } } -// Called when docker tries to delete credentials. This usually happens during `docker logout` calls. In our context, we +// Delete tries to delete credentials when docker requests it. This usually happens during `docker logout` calls. In our context, we // don't store arbitrary user given credentials so deleting them makes no sense. func (self ECRHelper) Delete(serverURL string) error { if shouldIgnoreCredsStorage() {