-
Notifications
You must be signed in to change notification settings - Fork 378
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
Match a signature for any tag when pulling/coping by digest #99
Comments
Excerpt from an IRC conversation so we don't forget:
|
The loosest match would be the best for default. Users will expect the TAG==Digest behaviour. Which I guess is matchSignatureForAnyTag |
This is a blocking issue for the The only way right now to relax this is to use:
but as noted by @mtrmac - this is an administrator thing |
agree here |
I'm working on this, we can bikeshed on naming on the PR when I'll submit it - let's go the way Miloslav outlined in the chat above |
@mtrmac this isn't working anymore (but used to! testing with projectatomic/docker#200):
|
@mtrmac has something changed which makes the above failing? |
|
(I can’t remember anything changing in this area FWIW.) |
sorry, the point was that the above used to work - runcom/busybox2:latest get translated to docker.io/runcom/busybox2:latest and the policy was working correctly 2 weeks ago..... |
Could this be because, within docker/docker + RH patches, docker/docker/reference is patched with a different semantics? Just guessing. |
it could - but projectatomic/docker#200 was working fine with containers/image 2 weeks ago so something happened in between :( |
No idea. Can you perhaps bisect that? (Note that containers/image does not vendor anything, so docker/docker/reference comes from whatever your build environment is.) |
@mtrmac you were right, I placed some logs around and:
So, we're signing with skopeo with the upstream |
None of that should be a dealbreaker; upstream docker/docker/reference, in Apparently the RH patch somehow breaks or removes the normalization logic? (Note that both removing it, and using a different one than upstream, would be very bad for signatures.) Note that the signature match code does not really care what the normalization is, but there must be one. We can, easily enough, use a |
@mtrmac yeah, figured that. BTW, we may probably need an actual fork just for containers/image which uses upstream docker/docker reference? |
@mtrmac wdyt? I know this can be bad as well but at least it ensures us we use the same docker/docker/reference when checking signatures. |
We didn’t have that, then we had that, now we don’t have that again. If re-adding that again is the simplest solution for signatures, sure, why not. I am, a bit, worried, though, that this lack of normalization will manifest elsewhere in docker as well. (See e.g. the “ |
I'm worried as well about this - that patch is almost 2 years old at least and apart from that bug discussed in the email I didn't spot anything significant till now. OTOH I'm not confident enough to change that code for the upcoming release :( but we definitely need an audit of that code wrt normalization. |
The scenario is the following (requires interacting with Docker):
runcom/busybox:signed
runcom/busybox:signed
viacontainers/image
containers/image
provides the trusted digest forruncom/busybox:signed
docker pull runcom/busybox@sha256:%HEX%
trust-plugin
on the docker API side ...trust-plugin
check that a valid signature exists forruncom/busybox@sha256:%HEX%
trust-plugin
fails because we don't have signature forruncom/busybox@sha256:%HEX%
but forruncom/busybox:signed
(which is one of the tag for that digest, which should be already trusted)I'm proposing something to relax this since if you're pulling by digest at some time it's likely that you have already checked for signatures from a tag.
/cc @mtrmac @aweiteka @rhatdan
The text was updated successfully, but these errors were encountered: