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

Valid oci reference names disallowed #294

Closed
owtaylor opened this issue Jun 21, 2017 · 5 comments
Closed

Valid oci reference names disallowed #294

owtaylor opened this issue Jun 21, 2017 · 5 comments

Comments

@owtaylor
Copy link
Contributor

The rules for a valid org.opencontainers.image.ref.name at:

 https://github.com/opencontainers/image-spec/blob/master/annotations.md

Are quite different from the checks in the code, which seem to be (in oci_transport.go)

 var refRegexp = regexp.MustCompile(`^([A-Za-z0-9._-]+)+$`)

For example, app/org.gnome.eog/x86_64/master is valid by the former set of rules and not by the later. (See containers/skopeo#369)

@runcom
Copy link
Member

runcom commented Jun 21, 2017

Nice catch

@owtaylor
Copy link
Contributor Author

owtaylor commented Jul 7, 2017

In containers/skopeo#369 mtrmac said:

(We may have to break the transport syntax now that an arbitrary number of : is allowed in the .tag :( )

Looking at the code in oci/layout/oci_transport.go I'm wondering if that is necessary - the code disallows ":" in the path component, so that (as I understand it) you can set a policy on /a/b:c and have that be unambiguously refer to path=/a/b, tag=c. Since ':' is not allowed in the path, then I think it should be possible to simply change:

+ sep := strings.LastIndex(reference, ":")
- sep := strings.Index(reference, ":")

If that doesn't work out for some reason, I think "::" would make sense as a separator since the ref-name syntax doesn't allow consecutive separator characters, and most single characters that aren't allowed in the ref-name are problematical in some way (|, $, etc.).

owtaylor added a commit to owtaylor/image that referenced this issue Aug 11, 2017
org.opencontainers.image.ref.name values are allowed to be much more complex
than the code allowed for. Change the validation to match the spec. Since
ref-names can contain a :, split from the path on the first :, not the
last :. (Path containing a : were already considered invalid.)

fixes containers#294
owtaylor added a commit to owtaylor/image that referenced this issue Aug 11, 2017
org.opencontainers.image.ref.name values are allowed to be much more complex
than the code allowed for. Change the validation to match the spec. Since
ref-names can contain a :, split from the path on the first :, not the
last :. (Path containing a : were already considered invalid.)

fixes containers#294

Signed-off-by: Owen W. Taylor <[email protected]>
owtaylor added a commit to owtaylor/image that referenced this issue Aug 11, 2017
org.opencontainers.image.ref.name values are allowed to be much more complex
than the code allowed for. Change the validation to match the spec. Since
ref-names can contain a :, split from the path on the first :, not the
last :. (Path containing a : were already considered invalid.)

fixes containers#294

Signed-off-by: Owen W. Taylor <[email protected]>
owtaylor added a commit to owtaylor/image that referenced this issue Aug 11, 2017
org.opencontainers.image.ref.name values are allowed to be much more complex
than the code allowed for. Change the validation to match the spec. Since
ref-names can contain a :, split from the path on the first :, not the
last :. (Path containing a : were already considered invalid.)

fixes containers#294

Signed-off-by: Owen W. Taylor <[email protected]>
owtaylor added a commit to owtaylor/image that referenced this issue Aug 24, 2017
org.opencontainers.image.ref.name values are allowed to be much more complex
than the code allowed for. Change the validation to match the spec. Since
ref-names can contain a :, split from the path on the first :, not the
last :. (Path containing a : were already considered invalid.)

fixes containers#294

Signed-off-by: Owen W. Taylor <[email protected]>
@mtrmac
Copy link
Collaborator

mtrmac commented Oct 26, 2017

This was fixed in #318.

@rhatdan
Copy link
Member

rhatdan commented Oct 26, 2017

@mtrmac If it is fixed, close the issue.

@mtrmac
Copy link
Collaborator

mtrmac commented Oct 27, 2017

Right, I meant to do that and somehow didn’t.

@mtrmac mtrmac closed this as completed Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants