From fd29a7adaa4c181e8b95860378bc48e7cf9b3f2a Mon Sep 17 00:00:00 2001 From: Prateek Chaudhry Date: Fri, 1 Sep 2023 09:09:35 -0700 Subject: [PATCH 1/2] redact ecr layer urls --- agent/dockerclient/dockerapi/docker_client.go | 12 +++- agent/dockerclient/dockerapi/errors.go | 16 +++++ agent/dockerclient/dockerapi/errors_test.go | 62 +++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) diff --git a/agent/dockerclient/dockerapi/docker_client.go b/agent/dockerclient/dockerapi/docker_client.go index 35fa5588f92..571533578f5 100644 --- a/agent/dockerclient/dockerapi/docker_client.go +++ b/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 diff --git a/agent/dockerclient/dockerapi/errors.go b/agent/dockerclient/dockerapi/errors.go index ae4261b5a43..9fbc1f0deec 100644 --- a/agent/dockerclient/dockerapi/errors.go +++ b/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) +} diff --git a/agent/dockerclient/dockerapi/errors_test.go b/agent/dockerclient/dockerapi/errors_test.go index 1bdd65a15fb..d5af42e9ecc 100644 --- a/agent/dockerclient/dockerapi/errors_test.go +++ b/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") + } +} From 99a1787590cbb9043b04717776e3e1f31c0a9b17 Mon Sep 17 00:00:00 2001 From: Prateek Chaudhry Date: Fri, 1 Sep 2023 14:43:07 -0700 Subject: [PATCH 2/2] rename str as overrideStr in redactEcrUrls --- agent/dockerclient/dockerapi/errors.go | 4 ++-- agent/dockerclient/dockerapi/errors_test.go | 22 ++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/agent/dockerclient/dockerapi/errors.go b/agent/dockerclient/dockerapi/errors.go index 9fbc1f0deec..29824f7e620 100644 --- a/agent/dockerclient/dockerapi/errors.go +++ b/agent/dockerclient/dockerapi/errors.go @@ -414,11 +414,11 @@ func (err CannotInspectContainerExecError) ErrorName() 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 { +func redactEcrUrls(overrideStr 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)) + redactedStr := urlRegex.ReplaceAllString(err.Error(), fmt.Sprintf("REDACTED ECR URL related to %s", overrideStr)) return fmt.Errorf(redactedStr) } diff --git a/agent/dockerclient/dockerapi/errors_test.go b/agent/dockerclient/dockerapi/errors_test.go index d5af42e9ecc..15bf6389dfd 100644 --- a/agent/dockerclient/dockerapi/errors_test.go +++ b/agent/dockerclient/dockerapi/errors_test.go @@ -36,61 +36,61 @@ func TestRetriableErrorReturnsTrue(t *testing.T) { func TestRedactEcrUrls(t *testing.T) { testCases := []struct { - str string + overrideStr string err error expectedErr error }{ // NO REDACT CASES { - str: "111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp", + overrideStr: "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", + overrideStr: "busybox:latest", err: fmt.Errorf("invalid reference format"), expectedErr: fmt.Errorf("invalid reference format"), }, { - str: "busybox:latest", + overrideStr: "busybox:latest", err: fmt.Errorf(""), expectedErr: fmt.Errorf(""), }, { - str: "busybox:latest", + overrideStr: "busybox:latest", err: fmt.Errorf("busybox:latest"), expectedErr: fmt.Errorf("busybox:latest"), }, { - str: "ubuntu", + overrideStr: "ubuntu", err: fmt.Errorf("busybox:latest"), expectedErr: fmt.Errorf("busybox:latest"), }, { - str: "111111111111.dkr.ecr.us-west-2.amazonaws.com/myapp", + overrideStr: "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", + overrideStr: "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", + overrideStr: "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", + overrideStr: "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) + redactedErr := redactEcrUrls(tc.overrideStr, tc.err) assert.Equal(t, redactedErr.Error(), tc.expectedErr.Error(), "ECR URL redaction output mismatch") } }