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

Bug in authorization code for private repos #90

Closed
lhernanz opened this issue Feb 3, 2020 · 3 comments
Closed

Bug in authorization code for private repos #90

lhernanz opened this issue Feb 3, 2020 · 3 comments
Labels

Comments

@lhernanz
Copy link

lhernanz commented Feb 3, 2020

This is related to #31. There is a bug in https://github.com/mcuadros/ofelia/blob/master/core/common.go#L242. The number 2 there should be 1.

The rationale is the following: when using docker hub, the containers are inside a folder of the owner (e.g. mcuadros/ofelia) but when using any other kind of registry, the first element is the name of the registry and the second could be the name of the container already (e.g. registry/container). Therefore, with the current code, this condition is missed and the authentication fails.

Please, notice that I am not entirely sure that the current code is going to work even for dockerhub as credentials from dockerhub should be prefixed with a repo URL too. The code is not including that repo URL in the search to retrieve the credentials.

@taraspos taraspos added the bug label Feb 4, 2020
@jgough
Copy link
Contributor

jgough commented Oct 7, 2020

Hi, I am working on a pull request to fix a bug with private repos that I have discovered. Do you have an example of an image name here so that I can verify works with my new code?

@taraspos
Copy link
Collaborator

taraspos commented Oct 7, 2020

Hey @jgough, can you explain the issue you found?

I've recently have been looking there as well regarding issue #118.
So what I've discovered that authorization with docker hub may not work because in config.json it is usually stored like:

    "https://index.docker.io/v1/": {
            "auth": "bas64decodeduserandpassowrd==",
            "email": "your email"
    }

however, ofelia tries to parse the registry from the image name. Which in case of docker hub will be mcuadros/ofelia and not https://index.docker.io/v1/mcuadros/ofelia, so lookup in map dockercfg.Configs[registry] fails, because registry is an empty string.

As a potential fix, I was thinking to do something like:

	if v, ok := dockercfg.Configs[registry]; ok {
		return v
	}

	// try to fetch configs from docker's default registry urls
	if registry == "" {
		if v, ok := dockercfg.Configs["https://index.docker.io/v2/"]; ok {
			return v
		}
		if v, ok := dockercfg.Configs["https://index.docker.io/v1/"]; ok {
			return v
		}
	}

	return auth
}

However, not sure how that would still work with fool registry URLs. Because in the config.json it still might be stored like:

  "auths" : {
    "https://account_id.dkr.ecr.us-east-1.amazonaws.com" : {
       ...
    }

While the image will be specified like: account_id.dkr.ecr.us-east-1.amazonaws.com/image:tag (without the https:// prefix), in this case, lookup in map will fail as well.

So I was thinking to check how other tools do that but didn't have time yet :(

@jgough
Copy link
Contributor

jgough commented Oct 7, 2020

My issue was around images of the form host:port/image:tag

buildPullOptions splits the string on ":" characters to get the tag in parts[1], but in this case it is in parts[2] and the parsing completely fails. My pull request addresses this issue.

I'm not that familiar with how the configs are usually stored, but in my case it's under the "auths" key in the config.

taraspos added a commit that referenced this issue Oct 8, 2020
* Support more types of image names

Now works with many cases of different image names including ports in registry or containers in folders.

Verified with the following image formats:
host:port/dir/image:tag
host:port/image:tag
host:port/image
host/image:tag
image
image:tag

fixes #31 #90 

* improve `parseRegistry`

add a check for `.` or `:` in the first part, if it is there, it must be registry URL
if not this can be image tag

Co-authored-by: Taras <9948629+Trane9991@users.noreply.github.com>
@taraspos taraspos closed this as completed Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants