Prevent fallback to v1 registry for digest pulls#12881
Conversation
nakedible
force-pushed
the
patch-1
branch
4 times, most recently
from
e1142a6 to
e83796d
Compare
|
The change made here is the simplest one - there would be room for refactoring and improvement altogether, but that might better be done separately? Also, should I write tests which fail if digest pulls ever fall back to v1 registry? |
There was a problem hiding this comment.
This may not be a strong enough guarantee. Its probably better to use digest.ParseDigest and catch the error or digest.DigestRegexp.Match. Arguably, utils.DigestReference should not exist. The most reliable way to tell if something is a reference is to test whether it came after the @.
There was a problem hiding this comment.
While utils.DigestReference is a fragile function, it is strong enough in this case. It is only intended to test for digest given a reference (digest or tag). Reference should be made a stronger type with explicit digest or ref, but that is a different refactor which is not necessary to implement this fix.
There was a problem hiding this comment.
I used utils.DigestReference because that exact function is used inside the v2 pull functions to check if loadManifest should attempt to verify that the manifest digest matches or not - the check should be identical. This should be safe as the tag has already been parsed, so the only way it can contain a colon is if it has been specified by the digest format.
I dislike mixing tags and digests in the tag argument to Pull, but fixing that is a bigger refactoring effort and I do not know enough Go.
There was a problem hiding this comment.
@nakedible Discussed with @dmcgowan and we think this is okay for this PR.
@ncdc We'll clean this up in the future.
|
@nakedible This is looking good although I am a little worried about how we are detecting pull by digest. I commented above. This approach should work but we may want to make this more explicit. Its up to you if you'd like to explore fixing this up.
Yes, please do. |
|
@nakedible if you are going to add a test, you can add one to the integration cli tests which attempts to pull a non-existent digest and checks the error output. Use a locally running v2 registry though, we are trying to avoid tests which rely on hitting the central repository. |
nakedible
force-pushed
the
patch-1
branch
2 times, most recently
from
f90d22b to
cce31ff
Compare
|
Added a simple test case, which only checks that the correct error message is shown. Ideally the test case should set up a working v1 registry and ensure that no v1 uris are accessed, but this is probably good enough for now. |
nakedible
force-pushed
the
patch-1
branch
2 times, most recently
from
05a9e92 to
82986c5
Compare
|
Rebased as single commit and cleaned up commit message. |
|
LGMT. @dmcgowan PTAL. |
|
LGTM |
|
Is this ready to merge? |
|
I'm new here so I'm not sure of the process here (considering this is still labeled as needs design review), but this is ready to merge from my side. No documentation changes as not falling back to an insecure download for a digest pull is kinda obvious. |
|
Just saw it was tagged as design review, updated. It is ready for merge after one more LGTM. |
|
Again, not sure of the policy for this sort of thing, but is it a possibility that this could get baked in to 1.6.1 or similar (once merged to master)? This is a security vulnerability, but it is a security vulnerability in a part that is explicitly marked as "do not trust this yet". |
|
@nakedible-p not sure if there will be a 1.6.1 release, or directly 1.7.0 I'll include @ewindisch for the security part, and @jfrazelle to check if there are any plans to release a 1.6.1 |
|
LGTM |
graph/pull.go
Outdated
There was a problem hiding this comment.
nit: Seems odd to tell users we're not falling back to v1. I don't think users care and it's confusing.
There was a problem hiding this comment.
I agree in general, but this is surrounded by log messages such as:
logrus.Errorf("Error from V2 registry: %s", err)
logrus.Debug("image does not exist on v2 registry, falling back to v1")
logrus.Debugf("pulling v1 repository with local name %q", repoInfo.LocalName)
However, would this be a reasonable error message:
secure pull with digest reference failed
Or should I just remove everything after the comma in the original message?
There was a problem hiding this comment.
@nakedible-p Just remove the post-comma content. No reason to hold this PR up on a minor nit.
|
LGTM; left a nit on an error message. |
|
@nakedible-p I'd say that because fallback exists in the first place and because this isn't a baked feature, we will hold off until the next scheduled release. This is not severe enough to warrant tagging a new release. |
|
@nakedible-p please update the error message then we can merge |
|
Added a comment about the error message 7 days ago, nothing has happened since. |
|
@nakedible-p Reiterating my LGTM. This can merge with or without the error message fix. |
|
@stevvooe Thanks. I hope someone give feedback on the comment I made about the error message though - I'd love to fix it if I knew which would be the right direction. I'm also fine with merging without any changes. |
|
Fixed error message, no other changes. |
The intention of the user is to download a verified image if explicitly pulling with a digest and falling back to v1 registry circumvents that protection. Signed-off-by: Nuutti Kotivuori <[email protected]>
|
@jfrazelle Not sure if I need to ping you separately... |
|
LGTM |
Prevent fallback to v1 registry for digest pulls
12 participants
Prevent fallback to v1 registry when pull is for a digest. The intention of the user is to download a verified image if explicitly pulling with a digest and falling back to v1 registry circumvents that protection.
Fixes #12879.