From 5ee6ab242c11add1eca3bda57a571e88e718fcbf Mon Sep 17 00:00:00 2001 From: therealvio <41095688+therealvio@users.noreply.github.com> Date: Thu, 8 Aug 2024 00:45:07 +1000 Subject: [PATCH] Implement both the Add and Delete operations as no-ops (#847) * 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. * Fix typo in add's nop log * Use WithField when logging warning * Gate docker creds ignore feature behind env var * Apply suggestions from code review Co-authored-by: Gavin Inglis <43075615+ginglis13@users.noreply.github.com> * fix: avoid logging username * fix: log warning only when AWS_ECR_IGNORE_CREDS_STORAGE is set * test: use testing-native methods to set and unset env vars * fix: guide users to setting AWS_ECR_IGNORE_CREDS_STORAGE var for ecr-login With the new capability to opt-out of saving credentials, we can guide users to not to save ecr credentials by setting the AWS_ECR_IGNORE_CREDS_STORAGE environment variable when encountering the `notImplemented` error. Ref: https://docs.aws.amazon.com/cli/latest/reference/ecr/get-login-password.html * fix: warning should be more explicit on nature of saving creds * test: use the current test scope, not parent This change fixes an incorrect use of the test's scope. The parent was used for assertions and setting the env. This now should be using the correct instance of the `*testing.T` for the test. --------- Co-authored-by: Boris Bera Co-authored-by: Boris Bera Co-authored-by: Gavin Inglis <43075615+ginglis13@users.noreply.github.com> --- README.md | 15 +++--- ecr-login/ecr.go | 37 ++++++++++++--- ecr-login/ecr_test.go | 103 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 246124e2..8ee7f8b0 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,7 @@ Windows executables are available via [GitHub releases](https://github.com/awsla 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: ``` @@ -263,7 +263,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 @@ -286,12 +286,13 @@ in the *AWS Command Line Interface User Guide*. The credentials must have a policy applied that [allows access to Amazon ECR](https://docs.aws.amazon.com/AmazonECR/latest/userguide/security-iam-awsmanpol.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 987713e9..5fbbd0ec 100644 --- a/ecr-login/ecr.go +++ b/ecr-login/ecr.go @@ -17,6 +17,7 @@ import ( "errors" "fmt" "io" + "os" "github.com/sirupsen/logrus" @@ -70,14 +71,38 @@ 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 +func shouldIgnoreCredsStorage() bool { + return os.Getenv("AWS_ECR_IGNORE_CREDS_STORAGE") == "true" } -func (ECRHelper) Delete(serverURL string) error { - // This does not seem to get called - return notImplemented +// 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() { + self.logger. + WithField("serverURL", creds.ServerURL). + Warning("Ignoring request to store credentials since AWS_ECR_IGNORE_CREDS_STORAGE env variable is set." + + "ecr-login does not require 'docker login', and does not support persisting temporary ECR-issued credentials.") + return nil + } else { + self.logger.Warning("Add() is not supported by the ecr-login credentials helper as all issued credentials are temporary. Consider setting the AWS_ECR_IGNORE_CREDS_STORAGE env variable (see documentation for details).") + return notImplemented + } +} + +// 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() { + self.logger. + WithField("serverURL", serverURL). + Warning("Ignoring request to store credentials since AWS_ECR_IGNORE_CREDS_STORAGE env variable is set." + + "ecr-login does not require 'docker login', and does not support persisting temporary ECR-issued credentials.") + return nil + } else { + self.logger.Warning("Delete() credentials is not supported by the ecr-login credentials helper as all issued credentials are temporary. Consider setting the AWS_ECR_IGNORE_CREDS_STORAGE env variable (see documentation for details).") + 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 d9bb9223..d74189d8 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" @@ -32,6 +33,28 @@ const ( expectedPassword = "password" ) +// unsetEnv unsets an environment variable and registers a cleanup function that +// restores it after the test is complete. Note: tests that modify environment +// variables may not be run in parallel. +// +// See also [testing.T.Setenv] +func unsetEnv(t *testing.T, key string) { + t.Helper() + + // save original value + originalValue, wasSet := os.LookupEnv(key) + + // unset the environment variable + os.Unsetenv(key) + + // restore the original value if necessary + if wasSet { + t.Cleanup(func() { + os.Setenv(key, originalValue) + }) + } +} + func TestGetSuccess(t *testing.T) { factory := &mock_api.MockClientFactory{} client := &mock_api.MockClient{} @@ -118,3 +141,83 @@ func TestListFailure(t *testing.T) { assert.Error(t, err) assert.Len(t, serverList, 0) } + +func TestAddIgnored(t *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + t.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "true") + err := helper.Add(&credentials.Credentials{ + ServerURL: proxyEndpoint, + Username: "AWS", + Secret: "supersecret", + }) + + assert.Nil(t, err) +} + +func TestAddNotImplemented(t *testing.T) { + tests := []struct { + name string + setEnv func(*testing.T) + }{ + {"unset", func(*testing.T) { unsetEnv(t, "AWS_ECR_IGNORE_CREDS_STORAGE") }}, + {"false", func(*testing.T) { t.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }}, + {"0", func(*testing.T) { t.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }}, + {"empty string", func(*testing.T) { t.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(tt) + err := helper.Add(&credentials.Credentials{ + ServerURL: proxyEndpoint, + Username: "AWS", + Secret: "supersecret", + }) + + assert.Error(tt, err, "not implemented") + }) + } +} + +func TestDeleteIgnored(t *testing.T) { + factory := &mock_api.MockClientFactory{} + + helper := NewECRHelper(WithClientFactory(factory)) + + t.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(*testing.T) + }{ + {"unset", func(*testing.T) { unsetEnv(t, "AWS_ECR_IGNORE_CREDS_STORAGE") }}, + {"false", func(*testing.T) { t.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "false") }}, + {"0", func(*testing.T) { t.Setenv("AWS_ECR_IGNORE_CREDS_STORAGE", "0") }}, + {"empty string", func(*testing.T) { t.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(tt) + err := helper.Delete(proxyEndpoint) + + assert.Error(tt, err, "not implemented") + }) + } +}