fix(aws): handle ECR repositories in different regions

[knrc]

Description

This PR modified the ECR integration so that it obtains authorization tokens from the region hosting the ECR. Current behaviour would be to use the default, resulting in authentication errors such as

2024-02-26T18:56:03.738Z	ERROR	Error during vulnerabilities or misconfiguration scan: scan error: unable to initialize a scanner: unable to initialize an image scanner: 4 errors occurred:
	* docker error: unable to inspect the image (127647282379.dkr.ecr.us-east-1.amazonaws.com/undistro-test-image:1.25.3): Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
	* containerd error: containerd socket not found: /run/containerd/containerd.sock
	* podman error: unable to initialize Podman client: no podman socket found: stat podman/podman.sock: no such file or directory
	* remote error: GET https://127647282379.dkr.ecr.us-east-1.amazonaws.com/v2/undistro-test-image/manifests/1.25.3: unexpected status code 401 Unauthorized: Not Authorized

2024-02-26T18:56:03.738Z	ERROR	Error during vulnerabilities or misconfiguration scan: scan error: unable to initialize a scanner: unable to initialize an image scanner: 4 errors occurred:
	* docker error: unable to inspect the image (127647282379.dkr.ecr.sa-east-1.amazonaws.com/undistro-test-image:1.25.3): Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
	* containerd error: containerd socket not found: /run/containerd/containerd.sock
	* podman error: unable to initialize Podman client: no podman socket found: stat podman/podman.sock: no such file or directory
	* remote error: GET https://127647282379.dkr.ecr.sa-east-1.amazonaws.com/v2/undistro-test-image/manifests/1.25.3: unexpected status code 401 Unauthorized: Not Authorized

This was raised in #1026, which is now closed although the underlying issue doesn't appear to be addressed.

Related issues

• Close AWS ECR registry authentication only works in the same/default region as caller #1026

Checklist

• I've read the guidelines for contributing to this repository.
• I've followed the conventions in the PR title.
• I've added tests that prove my fix is effective or that my feature works.
• I've updated the documentation with the relevant information (if needed).
• I've added usage information (if the PR introduces new options)
• I've included a "before" and "after" example to the description (if the PR is a user interface change).

[CLAassistant]

All committers have signed the CLA.

[DmitriyLewen]

Hello @knrc
Thanks for your report!

Left comments. Take a look, when you have time, please.

Regards, Dmitriy

[DmitriyLewen]

@knrc Thanks for your work!

[DmitriyLewen]

@knrc I found 1 interesting case:
if AWS_REGION env != region from domain:
Should we use AWS_REGION (we are overwriting this value now)?

IIUC this case is user mistake (wrongAWS_REGION). But perhaps it make sense to show warning log message about this.

[knrc]

@DmitriyLewen The point of the PR is to override the AWS_REGION setting, if we don't do that then we end up with an authentication token for one region and have no visibility of containers hosted in other regions.

Our use case is multiple private repositories in multiple regions.

[DmitriyLewen]

I meant that we need to tell the user that region from AWS_REGION != region from domain.
Something like that:

func getSession(region string, option types.RegistryOptions) (aws.Config, error) {
	// create custom credential information if option is valid
	if option.AWSSecretKey != "" && option.AWSAccessKey != "" && option.AWSRegion != "" {
		if region != option.AWSRegion {
			log.Logger.Warnf("The region from AWS_REGION (%s) is incorrect. The region from domain (%s) was used.", option.AWSRegion, region)
		}
		return config.LoadDefaultConfig(
			context.TODO(),
			config.WithRegion(region),
			config.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(option.AWSAccessKey, option.AWSSecretKey, option.AWSSessionToken)),
		)
	}
	return config.LoadDefaultConfig(context.TODO(), config.WithRegion(region))
}

Also i am worried about asff template. We use AWS_REGION env for this template. Perhaps we need to set AWS_REGION env when we have overwritten the region.

[knrc]

@DmitriyLewen Ah gotcha. We can certainly add a message, although I'm not sure it would make much sense as it is likely to have been set by the webhook to match the EKS installation. If you consider our use case, with multiple private repositories in different regions, then it would be impossible for the user to set the region appropriately so it would be defaulted to the webhook's view.

I can take a look at the template today, I didn't consider that, and can certainly pass the parameter through to getSession as that seems cleaner than rewriting it.

[knrc]

I've pushed the changes for getSession and the warning, looking at the template.

[DmitriyLewen]

But it looks like when using asff template, users will have AWS_REGION set. In addition, we display a warning.
We can start with these changes.

If problems arise, we will think about fixing them (as another solution, we can add your regex to asff.tpl).

[knrc]

should Region match the image region?

AWS docs(https://docs.aws.amazon.com/securityhub/1.0/APIReference/API_AwsSecurityFinding.html#securityhub-Type-AwsSecurityFinding-ProductArn) say:

Region

    The Region from which the finding was generated.

+1, in my view Arn and Region are not related but the template assumes they are.

[knrc]

But it looks like when using asff template, users will have AWS_REGION set. In addition, we display a warning. We can start with these changes.

If problems arise, we will think about fixing them (as another solution, we can add your regex to asff.tpl).

Yes, since the output doesn't change with this PR we are no worse off. I do think there is change needed for the template but that should be a separate issue.

[DmitriyLewen]

AWS docs(https://docs.aws.amazon.com/securityhub/1.0/APIReference/API_AwsSecurityFinding.html#securityhub-Type-AwsSecurityFinding-ProductArn) say:

oh... i sent the wrong link.
You can see ARN formats here:
https://docs.aws.amazon.com/securityhub/latest/userguide/securityhub-custom-providers.html
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html

[knrc]

@DmitriyLewen I don't think those other links change anything for this PR.

[DmitriyLewen]

Thanks for your work @knrc

@knqyf263 i approved this PR.
If you agree with #6217 (comment) - we can merge it.

[knrc]

@DmitriyLewen I ran into another problem, it turns out the registry code (at least in 0.49.1) is broken. The three registries (google, azure and ECR) are invoked concurrently, which means their state gets overwritten each time while still being used.

I have some fixes for that, I'll check with 0.50.0 and push up another version of this PR some time next week.

[knrc]

@DmitriyLewen I pushed up the changes so you can see the difference, I'm just about to test them on 0.50.0. I'll rebase the PR on the latest once I've validated it.

[knrc]

@DmitriyLewen I've tested and rebased the PR, it's ready again

[DmitriyLewen]

Hello @knrc

The three registries (google, azure and ECR) are invoked concurrently, which means their state gets overwritten each time while still being used.

I'm a little confused

Trivy checks registries sequentially:

trivy/pkg/fanal/image/registry/token.go

Lines 36 to 52 in 5f69937

	for _, registry := range registries {
	err := registry.CheckOptions(domain, opt)
	if err != nil {
	continue
	}
	username, password, err := registry.GetCredential(ctx)
	if err != nil {
	// only skip check registry if error occurred
	log.Logger.Debug(err)
	break
	}
	return authn.Basic{
	Username: username,
	Password: password,
	}
	}
	return authn.Basic{}

Which field is overwritten?

[knrc]

I'm a little confused

Trivy checks registries sequentially:

It does, within GetToken, however GetToken is called concurrently.

Which field is overwritten?

Line 37 calls CheckOptions, which will create client resources in the singleton registry based on the domain. This client is then used later within GetCredentials. Since the singletons are being accessed concurrently the client is not guaranteed to be the one created within the previous call to CheckOptions in the loop.

[DmitriyLewen]

I think I understand your logic.

But I don't see any place where we use GetToken function (or upper function) using goroutines.
We also use 1 image. Therefore, if we overwrite ECRClient -> it will be a new ECRClient but with the same settings.

But i can missing something. Will be great if you can show some example.

Anyway, I think these changes should be made in another PR.
Can you undo the last changes and create a new PR with those changes and the example in the new PR?

[knrc]

But I don't see any place where we use GetToken function (or upper function) using goroutines. We also use 1 image. Therefore, if we overwrite ECRClient -> it will be a new ECRClient but with the same settings.

But i can missing something. Will be great if you can show some example.

The artifacts are scanned by different workers in parallel, see

trivy/pkg/k8s/scanner/scanner.go

Lines 141 to 142 in e98c873

	p := parallel.NewPipeline(s.opts.Parallel, !s.opts.Quiet, resourceArtifacts, onItem, onResult)
	err = p.Do(ctx)

[DmitriyLewen]

@knrc sorry for the wait for an answer.

The artifacts are scanned by different workers in parallel, see

Thank you for showing me this. I'm currently seeing this problem!

Your changes look correct for this case.

I left 1 comment about google test.

[knrc]

@knrc sorry for the wait for an answer.

No worries, we all have our day jobs.

The artifacts are scanned by different workers in parallel, see

Thank you for showing me this. I'm currently seeing this problem!

:)

Your changes look correct for this case.

I left 1 comment about google test.

Sounds good, I'll take a look. I'm at Open Source Summit this week, but will try to get to this as quickly as I can.

[knrc]

Sounds good, I'll take a look. I'm at Open Source Summit this week, but will try to get to this as quickly as I can.

@DmitriyLewen I rebased and updated the PR for your comment, can you take another look?

[DmitriyLewen]

@knrc can you fix linter error?

[knrc]

@knrc can you fix linter error?

Yes, I can add that to the list since I'm in those files anyway.

[knrc]

@DmitriyLewen try now

[DmitriyLewen]

@knrc Thanks for your work!

@knqyf263 take a look, when you have time, please.

[knqyf263]

Thanks for the contribution. This PR needs to resolve conflicts now.

[github-actions]

This PR is stale because it has been labeled with inactivity.

[knrc]

I'll take a look at this over the next few days

[knrc]

The integration tests appear to be hitting a limit

    library_test.go:165: 
        	Error Trace:	/home/runner/work/trivy/trivy/pkg/fanal/test/integration/library_test.go:165
        	Error:      	Received unexpected error:
        	            	unable to find the specified image "ghcr.io/aquasecurity/trivy-test-images:opensuse-tumbleweed" in ["remote"]:
        	            	    github.com/aquasecurity/trivy/pkg/fanal/image.NewContainerImage
        	            	        /home/runner/work/trivy/trivy/pkg/fanal/image/image.go:58
        	            	  - 1 error occurred:
        	            		* remote error: GET https://ghcr.io/v2/aquasecurity/trivy-test-images/manifests/opensuse-tumbleweed: TOOMANYREQUESTS: retry-after: 750.362µs, allowed: 44000/minute

[knrc]

The test does run successfully when executed locally

[DmitriyLewen]

Hello @knrc
Looks like it was bug in CI/CD.

PR has been merged.