Skip to content

Conversation

@mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Aug 7, 2018

We need "pull" to be able to read the manifest, and "delete" to delete it.

Note that until docker/distribution commit ccb839e0e30c3b6992fb4084dfd6550d0ddd4d1a (Jan 3 2017), the action checked for deleting manifests was "*"; this does not add it.

(Also, neither of these actions are documented at https://github.com/docker/distribution/blob/master/docs/spec/auth/scope.md . At least https://github.com/docker/distribution/blob/master/docs/spec/auth/jwt.md says that requesting permissions that the server does not grant should not be an error, so this kind of non-compliance is hopefully safe enough.)

This should not affect OpenShift, where the token is an API token and does not actually include any scopes or permissions; that is also probably why noone has noticed before.

@jwhonce @runcom PTAL

@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Aug 7, 2018

Maybe I messed something up in my compilation, but the change doesn't seem to fix the problem for me, it only changes the error.

(In Skopeo)
# make binary-local
# make install
# skopeo --tls-verify=true delete docker://docker.io/tomsweeneyredhat/testing:first
WARN[0000] '--tls-verify' is deprecated, please set this on the specific subcommand 
FATA[0000] Failed to delete /v2/tomsweeneyredhat/testing/manifests/sha256:184a1460ad7ff5a861e572da10087c32a60adc97fdb2a6df5694df483e24daec: {"errors":[{"code":"UNSUPPORTED","message":"The operation is unsupported."}]}
 (405 Method Not Allowed) 

@TomSweeneyRedHat
Copy link
Member

and without tls-verify

# skopeo delete docker://docker.io/tomsweeneyredhat/testing:first
FATA[0001] Failed to delete /v2/tomsweeneyredhat/testing/manifests/sha256:184a1460ad7ff5a861e572da10087c32a60adc97fdb2a6df5694df483e24daec: {"errors":[{"code":"UNSUPPORTED","message":"The operation is unsupported."}]}
 (405 Method Not Allowed) 

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 7, 2018

Yes, this is known, as noted in containers/skopeo#531 . I’m sorry, I forgot to mention it here. When the server does not implement deletes at all, there’s not much we can do; the operation can succeed only against a registry which implements deleting images.

Running skopeo --debug delete should show that with this PR the code at least gets to the point of actually making a DELETE request, instead of failing on a GET.

We do at least one skopeo delete as part of skopeo’s integration tests (with deletion enabled), so this PR should hopefully not be completely broken; OTOH that registry does not require authentication, so it is not useful for verifying this PR is an improvement. Maybe I should add a real test.

@rhatdan
Copy link
Member

rhatdan commented Aug 8, 2018

LGTM

@TomSweeneyRedHat
Copy link
Member

Thanks for the info @mtrmac , I threw --debug on the command line and did indeed see the DELETE command get attempted when deleting from dockerhub. It still failed though as you suspected. I tried Quay.io too and got a 400 failure there and did not see the DELETE command. Thought I'd throw it by you in case it's of concern for skopeo and/or something we should try adjusting on quay.io. If I replace delete with inspect I see the info on the image fwiw.

skopeo --debug delete --creds tomsweeneyredhat:{mypassword} docker://quay.io/tomsweeneyredhat/myalpine:latest
DEBU[0000] Using registries.d directory /etc/containers/registries.d for sigstore configuration 
DEBU[0000]  Using "default-docker" configuration        
DEBU[0000]   Using file:///var/lib/atomic/sigstore      
DEBU[0000] Looking for TLS certificates and private keys in /etc/docker/certs.d/quay.io 
DEBU[0000] GET https://quay.io/v2/                      
DEBU[0000] Ping https://quay.io/v2/ err <nil>           
DEBU[0000] Ping https://quay.io/v2/ status 401          
FATA[0000] unexpected http code: 400, URL: https://quay.io/v2/auth?account=tomsweeneyredhat&scope=repository%3Atomsweeneyredhat%2Fmyalpine%3Apull%2Cdelete&service=quay.io 

@runcom
Copy link
Member

runcom commented Aug 8, 2018

LGTM

Approved with PullApprove

@rhatdan
Copy link
Member

rhatdan commented Aug 8, 2018

@runcom how do I get on the approved list for merging?

@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 8, 2018

I tried Quay.io too and got a 400 failure there and did not see the DELETE command.

Yes, that does not look too good. I’ll look into it.

@mtrmac mtrmac force-pushed the delete-permissions branch from 9b62cb3 to 620400e Compare August 10, 2018 00:40
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 10, 2018

I tried Quay.io too and got a 400 failure there and did not see the DELETE command.

So (keep in mind that we are in undocumented territory in all of this), it turns out that Quay requires the push action to be in the token, and it chokes on delete. Meanwhile, #211 forces us to hard-code the actions in the code, because gcr.io does not return an useful WWW-Authenticate challenge.

Luckily, it seems that both new docker/distribution and quay.io support * to mean “all actions”; and old docker/distribution explicitly required *. So, let’s try that.

@mtrmac mtrmac force-pushed the delete-permissions branch from 620400e to 82b74a5 Compare August 27, 2018 12:48
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 27, 2018

@runcom PTAL

@runcom
Copy link
Member

runcom commented Aug 27, 2018

@mtrmac LGTM, up to you to merge now

https://github.com/docker/distribution/blob/master/docs/spec/auth/scope.md
does not document the action necessary for deleting images.

For docker/distribution, we need "pull" to be able to read the manifest,
and "delete" to delete it.  Until docker/distribution commit
ccb839e0e30c3b6992fb4084dfd6550d0ddd4d1a (Jan 3 2017), the action checked
for deleting manifests was "*".

For Quay.io, only "push" is sufficient for both; and although
https://github.com/docker/distribution/blob/master/docs/spec/auth/jwt.md
says that requesting permissions that the server does not grant should
not be an error, quay.io refuses to even parse a request which contains
a "delete" action and does not grant any token.

This should not affect OpenShift, where the token is an API
token and does not actually include any scopes or permissions;
(that is also probably why noone has noticed before).

Overall, "*" seems to be the only common action specification;
luckily both docker/distribution and Quay.io seem to treat it
as "all actions allowed" (which is _not_ documented in the spec,
but then again, nothing about actions for deleting images is
documented).

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac mtrmac force-pushed the delete-permissions branch from 82b74a5 to e43b271 Compare August 27, 2018 14:03
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 27, 2018

👍

Approved with PullApprove

@mtrmac mtrmac merged commit 5d57275 into containers:master Aug 27, 2018
@mtrmac mtrmac deleted the delete-permissions branch August 27, 2018 15:20
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 this pull request may close these issues.

4 participants