aws auth use default creds when none are supplied

[caleblloyd]

Comprehensive Summary of your change

Harbor should not assume that if AWS Credentials are not defined that the user wants to use ec2rolecreds. Rather, it should allow the AWS SDK to resolve credentials by passing nil Credentials:

https://docs.aws.amazon.com/sdk-for-go/v1/developer-guide/configuring-sdk.html#specifying-credentials

This should be backwards compatible because the defualt provider chain does look for: 4. If your application is running on an Amazon EC2 instance, IAM role for Amazon EC2.

Issue being fixed

Fixes #15007

Related to #17962

Please indicate you've done the following:

• Well Written Title and Summary of the PR
• Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
• Accepted the DCO. Commits without the DCO will delay acceptance.
• Made sure tests are passing and test coverage is added if needed.
• Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

[Vad1mo]

This looks good to me, any idea or guide how to easily test this on AWS?

[caleblloyd]

A little tough to unit test because you need actual creds to run through the SDK. But I purpose-built a couple test binaries in branches on my fork. This run demonstrates that Before, the Shared credentials file is not loaded per AWS' default credential chain, but After it is.

I also tested Good/Bad static creds after the change and there are no regressions there.

Before Change:

Test source: https://github.com/caleblloyd/harbor/tree/aws-test-before-change/src/awssecrettest

Auto Detect

$ export AWS_PROFILE=my-profile-name
$ export AWS_SDK_LOAD_CONFIG=true
$ ./run.sh https://123456.dkr.ecr.us-east-2.amazonaws.com
2022-12-08T13:21:24-05:00 [INFO] [/pkg/reg/adapter/native/adapter.go:36]: the factory for adapter docker-registry registered
2022-12-08T13:21:24-05:00 [INFO] [/pkg/reg/adapter/awsecr/adapter.go:44]: the factory for adapter aws-ecr registered
2022-12-08T13:21:30-05:00 [FATAL] [/awssecrettest/awssecrettest.go:38]: EC2RoleRequestError: no EC2 instance role found
caused by: RequestError: send request failed
caused by: Get "http://169.254.169.254/latest/meta-data/iam/security-credentials/": dial tcp 169.254.169.254:80: connect: no route to host

After Change:

Test source: https://github.com/caleblloyd/harbor/tree/aws-test-after-change/src/awssecrettest

Auto Detect

$ export AWS_PROFILE=my-profile-name
$ export AWS_SDK_LOAD_CONFIG=true
$ ./run.sh https://123456.dkr.ecr.us-east-2.amazonaws.com
2022-12-08T13:18:08-05:00 [INFO] [/pkg/reg/adapter/native/adapter.go:36]: the factory for adapter docker-registry registered
2022-12-08T13:18:08-05:00 [INFO] [/pkg/reg/adapter/awsecr/adapter.go:44]: the factory for adapter aws-ecr registered
<valid token>

With Bad Static Token:

$ ./run.sh https://123456.dkr.ecr.us-east-2.amazonaws.com "bad" "bad"
2022-12-08T13:32:40-05:00 [INFO] [/pkg/reg/adapter/native/adapter.go:36]: the factory for adapter docker-registry registered
2022-12-08T13:32:40-05:00 [INFO] [/pkg/reg/adapter/awsecr/adapter.go:44]: the factory for adapter aws-ecr registered
2022-12-08T13:32:41-05:00 [FATAL] [/awssecrettest/awssecrettest.go:38]: UnrecognizedClientException: The security token included in the request is invalid.
        status code: 400, request id: 7c3a3f19-95c8-4cdd-9b39-a1cb07edab2b
exit status 1

With Good Static Token:

$ ./run.sh https://123456.dkr.ecr.us-east-2.amazonaws.com "xxxxxx" "xxxxxx"
2022-12-08T13:35:15-05:00 [INFO] [/pkg/reg/adapter/native/adapter.go:36]: the factory for adapter docker-registry registered
2022-12-08T13:35:15-05:00 [INFO] [/pkg/reg/adapter/awsecr/adapter.go:44]: the factory for adapter aws-ecr registered
<valid token>

[caleblloyd]

After Change - EC2 Instance Metadata Allows getting token:

After Change - EC2 Instance Metadata Does Not Allow getting token:

[stonezdj]

LGTM

[stonezdj]

LGTM

[slacki123]

Hey @Vad1mo, are you the other reviewer on this PR? We're really excited to get this released

[slacki123]

Hi @caleblloyd @Vad1mo, what is the status on this PR?

[OrlinVasilev]

Will close and re-open the PR so we can try to unblock GH actions!

[codecov]

Codecov Report

Merging #17932 (95350b3) into main (bfe4362) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #17932      +/-   ##
==========================================
- Coverage   66.41%   66.41%   -0.01%     
==========================================
  Files        1012     1012              
  Lines      108684   108682       -2     
  Branches     2676     2676              
==========================================
- Hits        72186    72184       -2     
+ Misses      32534    32531       -3     
- Partials     3964     3967       +3     

Flag	Coverage Δ
unittests	66.41% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pkg/reg/adapter/awsecr/auth.go	71.00% <ø> (-0.57%)	⬇️
...es/vulnerability/vulnerability-config.component.ts	58.51% <0.00%> (-4.45%)	⬇️
...ortal/src/app/shared/pipes/harbor-datetime.pipe.ts	32.00% <0.00%> (-4.00%)	⬇️
src/jobservice/sync/schedule.go	54.54% <0.00%> (-2.38%)	⬇️
src/server/v2.0/handler/user.go	13.43% <0.00%> (+1.55%)	⬆️
src/common/utils/passports.go	90.19% <0.00%> (+3.92%)	⬆️
...-job/gc-page/gc/gc-history/gc-history.component.ts	46.51% <0.00%> (+5.81%)	⬆️

[Vad1mo]

@caleblloyd, seems the test fail, can you rebase and we try again. We merge if it becomes green.

[caleblloyd]

Yes I will rebase, I also had a change in here to upgrade aws-sdk-go, but now I am seeing this in CONTRIBUTING.md:

The official maintainers will take the responsibility for managing the code in vendor directory.

So I will remove the go.mod change also and open another issue for that

[caleblloyd]

Opened #18134 requesting the dependency update

[slacki123]

@caleblloyd looks like test coverage decreased by 0.01% and failed the checks. Can we do anything about it?

[caleblloyd]

All the checks were green, but it was complaining that the commit was not signed, so I have re-pushed the same code except with a signed commit now. I think the workflows may need to be manually approved again because I am a first-time contributor?