redact ecr layer urls

agent/dockerclient/dockerapi/docker_client.go

@@ -341,7 +341,7 @@ func (dg *dockerGoClient) PullImage(ctx context.Context, image string,
 				}
 				return err
 			})
-		response <- DockerContainerMetadata{Error: wrapPullErrorAsNamedError(err)}
+		response <- DockerContainerMetadata{Error: wrapPullErrorAsNamedError(image, err)}
 	}()
 
 	select {
@@ -356,15 +356,17 @@ func (dg *dockerGoClient) PullImage(ctx context.Context, image string,
 		}
 		// Context was canceled even though there was no timeout. Send
 		// back an error.
+		err = redactEcrUrls(image, err)
 		return DockerContainerMetadata{Error: &CannotPullContainerError{err}}
 	}
 }
 
-func wrapPullErrorAsNamedError(err error) apierrors.NamedError {
+func wrapPullErrorAsNamedError(image string, err error) apierrors.NamedError {
 	var retErr apierrors.NamedError
 	if err != nil {
 		engErr, ok := err.(apierrors.NamedError)
 		if !ok {
+			err = redactEcrUrls(image, err)
 			engErr = CannotPullContainerError{err}
 		}
 		retErr = engErr
@@ -382,11 +384,12 @@ func (dg *dockerGoClient) pullImage(ctx context.Context, image string,
 
 	sdkAuthConfig, err := dg.getAuthdata(image, authData)
 	if err != nil {
-		return wrapPullErrorAsNamedError(err)
+		return wrapPullErrorAsNamedError(image, err)
 	}
 	// encode auth data
 	var buf bytes.Buffer
 	if err := json.NewEncoder(&buf).Encode(sdkAuthConfig); err != nil {
+		err = redactEcrUrls(image, err)
 		return CannotPullECRContainerError{err}
 	}
 
@@ -457,6 +460,7 @@ func (dg *dockerGoClient) pullImage(ctx context.Context, image string,
 		break
 	case pullErr := <-pullFinished:
 		if pullErr != nil {
+			pullErr = redactEcrUrls(image, pullErr)
 			return CannotPullContainerError{pullErr}
 		}
 		seelog.Debugf("DockerGoClient: pulling image complete: %s", image)
@@ -468,6 +472,7 @@ func (dg *dockerGoClient) pullImage(ctx context.Context, image string,
 
 	err = <-pullFinished
 	if err != nil {
+		err = redactEcrUrls(image, err)
 		return CannotPullContainerError{err}
 	}
 
@@ -522,6 +527,7 @@ func (dg *dockerGoClient) getAuthdata(image string, authData *apicontainer.Regis
 		provider := dockerauth.NewECRAuthProvider(dg.ecrClientFactory, dg.ecrTokenCache)
 		authConfig, err := provider.GetAuthconfig(image, authData)
 		if err != nil {
+			err = redactEcrUrls(image, err)
 			return authConfig, CannotPullECRContainerError{err}
 		}
 		return authConfig, nil

agent/dockerclient/dockerapi/errors.go

@@ -14,6 +14,8 @@
 package dockerapi
 
 import (
+	"fmt"
+	"regexp"
 	"time"
 
 	"github.com/aws/amazon-ecs-agent/agent/dockerclient"
@@ -406,3 +408,17 @@ func (err CannotInspectContainerExecError) Error() string {
 func (err CannotInspectContainerExecError) ErrorName() string {
 	return "CannotInspectContainerExecError"
 }
+
+// Redact ECR bucket urls from error string
+// Return a new error with redacted string - replacing ECR bucket (*starport-layer-bucket*) urls with a string.
+// This is done because container runtime's request may sometimes contain security tokens when accessing ECR buckets for image layers.
+// When these requests error out, the URLs with secrets may get bubbled up to Agent logs.
+// So we redact the otherwise hidden URLs for security.
+func redactEcrUrls(str string, err error) error {
+	if err == nil {
+		return nil
+	}
+	urlRegex := regexp.MustCompile(`\"?https[^\s]+starport-layer-bucket[^\s]+`)
+	redactedStr := urlRegex.ReplaceAllString(err.Error(), fmt.Sprintf("REDACTED ECR URL related to %s", str))
+	return fmt.Errorf(redactedStr)
+}

agent/dockerclient/dockerapi/errors_test.go

@@ -18,6 +18,7 @@ package dockerapi
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -32,3 +33,64 @@ func TestRetriableErrorReturnsTrue(t *testing.T) {
 	err := CannotStopContainerError{errors.New("error")}
 	assert.True(t, err.IsRetriableError(), "Non unretriable error treated as unretriable docker error")
 }
+
+func TestRedactEcrUrls(t *testing.T) {
+	testCases := []struct {
+		str         string
+		err         error
+		expectedErr error
+	}{
+		// NO REDACT CASES
+		{
+			str:         "111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp",
+			err:         fmt.Errorf("Error response from daemon: pull access denied for 111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::111111111111:assumed-role/ecsInstanceRole/i-01a01a01a01a01a01a is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:us-west-2:111111111111:repository/myapp because no resource-based policy allows the ecr:BatchGetImage action"),
+			expectedErr: fmt.Errorf("Error response from daemon: pull access denied for 111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp, repository does not exist or may require 'docker login': denied: User: arn:aws:sts::111111111111:assumed-role/ecsInstanceRole/i-01a01a01a01a01a01a is not authorized to perform: ecr:BatchGetImage on resource: arn:aws:ecr:us-west-2:111111111111:repository/myapp because no resource-based policy allows the ecr:BatchGetImage action"),
+		},
+		{
+			str:         "busybox:latest",
+			err:         fmt.Errorf("invalid reference format"),
+			expectedErr: fmt.Errorf("invalid reference format"),
+		},
+		{
+			str:         "busybox:latest",
+			err:         fmt.Errorf(""),
+			expectedErr: fmt.Errorf(""),
+		},
+		{
+			str:         "busybox:latest",
+			err:         fmt.Errorf("busybox:latest"),
+			expectedErr: fmt.Errorf("busybox:latest"),
+		},
+		{
+			str:         "ubuntu",
+			err:         fmt.Errorf("busybox:latest"),
+			expectedErr: fmt.Errorf("busybox:latest"),
+		},
+		{
+			str:         "111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp",
+			err:         fmt.Errorf("String with https://111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp"),
+			expectedErr: fmt.Errorf("String with https://111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp"),
+		},
+		// REDACT CASES
+		{
+			str:         "111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp",
+			err:         fmt.Errorf("ref pull has been retried 5 time(s): failed to copy: httpReadSeeker: failed open: failed to do request: Get \"https://prod-us-east-1-starport-layer-bucket.s3.us-east-1.amazonaws.com/d1d1d1-222222222222-11d11d1d-4444-aaaa-bbbb-e4444gg4g4g4/9s9s9s9s9s-2b2b-a2a2-dddd-gg99gg99gg99?X-Amz-Security-Token=mysup3rs3cr3ttok3n\""),
+			expectedErr: fmt.Errorf("ref pull has been retried 5 time(s): failed to copy: httpReadSeeker: failed open: failed to do request: Get REDACTED ECR URL related to 111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp"),
+		},
+		{
+			str:         "111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp",
+			err:         fmt.Errorf("https://prod-us-east-1-starport-layer-bucket.s3.us-east-1.amazonaws.com/d1d1d1-222222222222-11d11d1d-4444-aaaa-bbbb-e4444gg4g4g4/9s9s9s9s9s-2b2b-a2a2-dddd-gg99gg99gg99?X-Amz-Security-Token=mysup3rs3cr3tt0k3n"),
+			expectedErr: fmt.Errorf("REDACTED ECR URL related to 111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp"),
+		},
+		{
+			str:         "111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp",
+			err:         fmt.Errorf("failed to do request: Get \"https://prod-us-east-1-starport-layer-bucket.s3.us-east-1.amazonaws.com/d1d1d1-222222222222-11d11d1d-4444-aaaa-bbbb-e4444gg4g4g4/9s9s9s9s9s-2b2b-a2a2-dddd-gg99gg99gg99?X-Amz-Security-Token=mysup3rs3cr3ttok3n\" and another request \"https://prod-us-east-1-starport-layer-bucket.s3.us-east-1.amazonaws.com/d1d1d1-222222222222-11d11d1d-4444-aaaa-bbbb-e4444gg4g4g4/9s9s9s9s9s-2b2b-a2a2-dddd-gg99gg99gg99?X-Amz-Security-Token=mysup3rs3cr3ttok3n\""),
+			expectedErr: fmt.Errorf("failed to do request: Get REDACTED ECR URL related to 111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp and another request REDACTED ECR URL related to 111111111111.dkr.ecr.us-east-1.amazonaws.com/myapp"),
+		},
+	}
+
+	for _, tc := range testCases {
+		redactedErr := redactEcrUrls(tc.str, tc.err)
+		assert.Equal(t, redactedErr.Error(), tc.expectedErr.Error(), "ECR URL redaction output mismatch")
+	}
+}