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.

Sign up for GitHub

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

Jump to bottom

Improve container credentials retrieval matching container repository names #224

Open
pditommaso opened this issue Mar 1, 2023 · 12 comments · May be fixed by #278
Open

Improve container credentials retrieval matching container repository names #224

pditommaso opened this issue Mar 1, 2023 · 12 comments · May be fixed by #278
Assignees
@munishchouhan

Comments

@pditommaso
Copy link
Contributor

Wave retrieves the container credentials to be used, querying Tower credentials for container-reg provider and looking for a matching registry name for the given repository e.g. docker.io given the repository docker.io/library/ubuntu.

However, this approach limits the possibility of authenticating repositories in the same registry owned by different users or organisations, requiring different credentials.

This issue aims to extend the credentials retrieval mechanism so that the entire container repository name is used to discover the matching credentials to be used, not just the registry name.

The implementation of this feature also requires that Tower allows, optionally, storing the full container name,
@pditommaso
Copy link
Contributor Author

This issue requires changes both on Tower and Wave side

Tower

  • The field "Registry server" in the container crendentials should changed to "Registry or repository name"

  • The user can enter both values, such as docker.io or docker.io/username/repositoryname. Note: the tag should not be allowed, e.g docker.io/username/repositoryname:xyz is not valid. When omitted it falls back to docker.io (like it's doing now)

  • This value is continued to be stored in the field registry

Wave side

The credentials matching logic on Wave should be extend so that it tries to match the keys:

  1. a given repository name
  2. fallback to the registry name if there's no match for the repository (already implemented)

The relevant code is this

wave/src/main/groovy/io/seqera/wave/service/CredentialServiceImpl.groovy

Lines 49 to 74 in 1546fe5

// find credentials with a matching registry
// TODO @t0randr
// for the time being we take the first matching credentials.
// A better approach would be to match the credentials
// based on user repository, but this is not supported by tower:
// For instance if we try to pull from docker.io/seqera/tower:v22
// then we should match credentials for docker.io/seqera instead of
// the ones associated with docker.io.
// This cannot be implemented at the moment since, in tower, container registry
// credentials are associated to the whole registry
final matchingRegistryName = registryName ?: DOCKER_IO
final creds = all.find {
it.provider == 'container-reg' && (it.registry ?: DOCKER_IO) == matchingRegistryName
}
if (!creds) {
log.debug "No credentials matching criteria registryName=$registryName; userId=$userId; workspaceId=$workspaceId; endpoint=$towerEndpoint"
return null
}
// log for debugging purposes
log.debug "Credentials matching criteria registryName=$registryName; userId=$userId; workspaceId=$workspaceId; endpoint=$towerEndpoint => $creds"
// now fetch the encrypted key
final encryptedCredentials = towerClient.fetchEncryptedCredentials(towerEndpoint, towerToken, creds.id, pairing.pairingId, workspaceId).get()
final privateKey = pairing.privateKey
final credentials = decryptCredentials(privateKey, encryptedCredentials.keys)
return parsePayload(credentials)

Caveats

When entering container credentials in Tower the keys are validated using this code, and invoking this service on Wave

The service accepts a registry name i.e. docker.io not sure if it's possible to validate the credentials against a specific repository name e.g. docker.io/orgname/reponame

@pditommaso
Copy link
Contributor Author

Tagging @jimmypoms that can help on Tower side

@munishchouhan
Copy link
Member

@pditommaso so this will allow collaborates to access private repos as described
here https://docs.docker.com/docker-hub/repos/access/

I can not add a collaborator to my docker account,
Can you please add me to one of your private repo?
my username=hrma017
Screenshot 2023-08-07 at 13 34 34

@pditommaso
Copy link
Contributor Author

I've added you to the pditommaso/wave-tests repository

@munishchouhan
Copy link
Member

munishchouhan commented Aug 8, 2023 •
edited
Loading

I've added you to the pditommaso/wave-tests repository

please make this repository private

@pditommaso
Copy link
Contributor Author

I'd suggest using another repo for testing. Also, a bit more details about the expected behaviour.

Given a container repository e.g. host.com/foo/bar, and a list of possible choices, the algorithm should pick the best match. For example, having the following list:

  1. host.com
  2. host.com/foo
  3. host.com/foo/bar
  4. host.com/foo/bar/baz

The best match is clearly 3. When having partial matches, it should be taken the longest path having a partial match, for example, having those choices

  1. host.com
  2. host.com/foo
  3. host.com/fooo
  4. host.com/foo/bar/baz

it should be taken 2. Does it make sense?

@munishchouhan
Copy link
Member

Do we get this list of possible choices from tower credentials?

@pditommaso
Copy link
Contributor Author

Yes, once the registry is modified to return (optionally) the repository name

@munishchouhan munishchouhan linked a pull request Aug 8, 2023 that will close this issue
improve container credentials retrieval matching container repository names #278
Open
@munishchouhan
Copy link
Member

@pditommaso I have added functionality to check credentials against a repository
but i am still not clear, where this algorithm to find best match for a container registry will go?
should i add a List possible matches in ValidateRegistryCredsRequest.groovy?

wave/src/main/groovy/io/seqera/wave/controller/ValidateRegistryCredsRequest.groovy

Lines 9 to 16 in d15f1c3

class ValidateRegistryCredsRequest {
@NotBlank
String userName
@NotBlank
String password
@NotBlank
String registry
}

@munishchouhan munishchouhan added the WIP Work In Progress label Aug 9, 2023
@pditommaso
Copy link
Contributor Author

pditommaso commented Aug 10, 2023 •
edited
Loading

where this algorithm to find best match for a container registry will go?

Here. Now it's fetches all container cedentials, and then check for first one having a mahcing registry name, see here.

Instead, it should be assumed the field registry in the tower credential can be a simple registry name e.g. docker.io, and partial repository eg. docker.io/username or specific repo eg. docker.io/username/reponame. the goal is to pick the best match giving the current repository name.

@munishchouhan munishchouhan removed the WIP Work In Progress label Aug 25, 2023
@canny Canny
Copy link

canny bot commented May 30, 2024

This issue has been linked to a Canny post: Authenticate to ECR using AWS roles 🎉

@canny Canny
Copy link

canny bot commented May 30, 2024

This issue has been unlinked from a Canny post: Authenticate to ECR using AWS roles 😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@munishchouhan munishchouhan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

improve container credentials retrieval matching container repository names seqeralabs/wave
2 participants
@pditommaso @munishchouhan