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 <[email protected]> * 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 <[email protected]> Co-authored-by: Boris Bera <[email protected]> Co-authored-by: Gavin Inglis <[email protected]>
awslabs · Aug 7, 2024 · 5ee6ab2 · 5ee6ab2
1 parent a8d7d3c
commit 5ee6ab2
Show file tree

Hide file tree

Showing 3 changed files with 142 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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")
+		})
+	}
+}