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

Allow a role to be assumed before checking Docker images in AWS ECR #9103

Closed
wants to merge 6 commits into from

Conversation

dgholz
Copy link

@dgholz dgholz commented Feb 22, 2024

This is to resolve #6152

Very much a work-in-progress. I've tested this code with:

  • An ECR repository that gives read permissions to the user via a resource policy
  • A user with an identity policy to read the ECR repository
    (both of the existing use-cases)

And a new use case:

  • A user with permission to assume a role which has an identity policy to read the ECR repository

I still have a few challenges:

  • Writing the tests to verify this behaviour
  • Documenting how to use this feature
    • Mostly "which repo has the user-facing docs that need updating?"
  • Understanding how the Credentials Proxy gets used from the Docker package updater
    • And if this change affects it

Any advice or guidance on these would be greatly appreciated!

I spotted a bug with bin/dry-run when credentials get set: they're not converted to Dependabot::Credentials. A follow-up to #8967?
I also saw that regctl assumes it's already logged in, which isn't always true.

The session token is only useful for session credentials that needed an MFA.
Dependabot will never use session credentials. Devs who want to test locally may want to use them.
@dgholz dgholz requested a review from a team as a code owner February 22, 2024 16:09
@github-actions github-actions bot added the L: docker Docker containers label Feb 22, 2024
If `LOCAL_ECR_REGISTRY` is set, add the standard AWS environment variables as credentials.
Add ECR role to assume to ECR credentials, if set.
@dgholz dgholz force-pushed the allow_role_assumption_for_ecr branch from 8a455d9 to a38271d Compare February 22, 2024 16:11
@jurre
Copy link
Member

jurre commented Feb 22, 2024

Unfortunately, I don't think this is going to work, in production the VM that runs dependabot does not have access to credential data. We run a short-lived job specific proxy that intercepts requests and attaches any credentials that are configured for requests that match the configured registry. So while this might work in dry-runs, in production it would fail.

Unfortunately said proxy is not publicly available today

@dgholz
Copy link
Author

dgholz commented Feb 22, 2024

Thanks for the explanation, I was worried about that. I see that the Dependabot CLI can set up a local environment with the credential proxy, but I guess that it's aimed at internal team members?

@jurre
Copy link
Member

jurre commented Feb 23, 2024

Thanks for the explanation, I was worried about that. I see that the Dependabot CLI can set up a local environment with the credential proxy, but I guess that it's aimed at internal team members?

Yeah the proxy container can be pulled publicly and contains an executable version of the proxy, but the source code is not available today. We've talked about opening this up as well to allow contributions such as yours but it's not something we have concrete plans for right now, as there's a bit of red tape involved and so many competing priorities. I wish I had a more satisfying answer to be honest.

@dgholz
Copy link
Author

dgholz commented Feb 23, 2024

No worries, thanks for confirming that. I'll drop this PR: the actual AWS SDK code change is pretty trivial, so no big deal to recreate later when/if the proxy gets opened up.

@dgholz dgholz closed this Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: docker Docker containers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support IAM roles for ECR
2 participants