Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redact ECR layer URLs from container pull errors #3885

Merged
merged 2 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions agent/dockerclient/dockerapi/docker_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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}
}

Expand Down Expand Up @@ -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)
Expand All @@ -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}
}

Expand Down Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions agent/dockerclient/dockerapi/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
package dockerapi

import (
"fmt"
"regexp"
"time"

"github.com/aws/amazon-ecs-agent/agent/dockerclient"
Expand Down Expand Up @@ -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(overrideStr string, err error) error {
if err == nil {
return nil
}
urlRegex := regexp.MustCompile(`\"?https[^\s]+starport-layer-bucket[^\s]+`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're being very prescriptive in this redact function (ie this applies only to ECR). This is fine for now. I can see this being applicable to more cases in future.

Also nit maybe we call this not str, but image so it's clear that the error returns the image in place of the redacted URL?

Copy link
Contributor

@mythri-garaga mythri-garaga Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synced up offline with @prateekchaudhry, for now we are restricting redacting only ECR's URLs containing starport-layer-bucket bucket name and cannot make this regex expression more generic as there is no evidence of other URLs containing security token and might need them for debugging purpose.

Copy link
Contributor Author

@prateekchaudhry prateekchaudhry Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is right, being prescriptive is the intention - we can make it more generic if we have to in future.

Ack, I'll replace str with overrideStr to keep the function usage generic and more clear, if that is fine.

redactedStr := urlRegex.ReplaceAllString(err.Error(), fmt.Sprintf("REDACTED ECR URL related to %s", overrideStr))
return fmt.Errorf(redactedStr)
}
62 changes: 62 additions & 0 deletions agent/dockerclient/dockerapi/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package dockerapi

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -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 {
overrideStr string
err error
expectedErr error
}{
// NO REDACT CASES
{
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"),
},
{
overrideStr: "busybox:latest",
err: fmt.Errorf("invalid reference format"),
expectedErr: fmt.Errorf("invalid reference format"),
},
{
overrideStr: "busybox:latest",
err: fmt.Errorf(""),
expectedErr: fmt.Errorf(""),
},
{
overrideStr: "busybox:latest",
err: fmt.Errorf("busybox:latest"),
expectedErr: fmt.Errorf("busybox:latest"),
},
{
overrideStr: "ubuntu",
err: fmt.Errorf("busybox:latest"),
expectedErr: fmt.Errorf("busybox:latest"),
},
{
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
{
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"),
},
{
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"),
},
{
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.overrideStr, tc.err)
assert.Equal(t, redactedErr.Error(), tc.expectedErr.Error(), "ECR URL redaction output mismatch")
}
}