[WIP] Add SSL Certificate Support to Fast Pull and Push #39
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
@mattmoor Does this approach seem reasonable? And should it be expanded to all uses of the transport pool? Also, I'll open up a pr against docker_rules if/when this guy gets merged. I've got a branch that threads through the arguments. |
| @@ -45,6 +45,13 @@ | |||
| parser.add_argument('--directory', action='store', | |||
| help='Where to save the image\'s files.') | |||
|
|
|||
| parser.add_argument('--certificates', nargs='*', help='A comma separated ' + | |||
There was a problem hiding this comment.
Maybe this parser argument can get pulled into a common helper since it's duplicated at the moment.
There was a problem hiding this comment.
Look at what jmillikin did here for logging: https://github.com/google/containerregistry/blob/master/tools/logging_setup_.py#L31
|
tl;dr I'm worried this is going to end up making rules very verbose. What does a An alternative would be to make the client support something like this. I think my bias is generally towards adopting compatibility with the solutions folks are using with conventional clients, unless there are strong reasons against (maybe there are). Are you guys using anything like this for certs? |
|
There's also precedent for defining certs using flags on the Docker client: |
|
@KingEmet thanks, I was looking for that. :) |
|
I'm happy with either solution. Is there a clean way to depend on certs in |
|
@ahirreddy my biggest concern is the drwx------ 2 root root 4096 Jan 20 2017 dockerSince this is daemon-side auth and the daemon runs as root, it can read this ACL'd this way. We don't (want to) require I think we basically want what you have, but instead of passing the certs it would enable reading them from this standard directory (WDYT?). @KingEmet I don't think there are environment variables controlling this because it's daemon-side. I think the environment variables control certs for talking to the daemon. Please correct me if I'm mistaken. :) |
|
@ahirreddy In summary, if you can also require making that directory user-readable, then the flag option is viable and less verbose. |
|
@mattmoor Sounds good! I'll give it a try this weekend |
|
@mattmoor looks like it, yeah. Seems like |
| for item in args.certificates: | ||
| logging.info('Adding certificate %s', item) | ||
| key, cert, domain = item.split(',') | ||
| transport.add_certificate(key, cert, domain) |
There was a problem hiding this comment.
Can you please add a test to ensure, this actually works. Looking at the code I have some doubts.
| @@ -61,6 +61,13 @@ | |||
| parser.add_argument('--oci', action='store_true', | |||
| help='Push the image with an OCI Manifest.') | |||
|
|
|||
| parser.add_argument('--certificates', nargs='*', help='A comma separated ' + | |||
There was a problem hiding this comment.
This seems to disallow paths, which contain a comma, right? Any plans for an encoding?
|
Um...the merge-base version for this is a bit old cough. I am trying to rebase and fix the problems. |
|
Ok, rebased and pushed to https://github.com/abergmeier/containerregistry/tree/container-push-pull-cert. It now semi-depends on #51. |
|
Sorry, I've been swamped with other things. I'm planning to take a look next week! |
|
@mattmoor It occurred to me, it's unclear where to look for the certificates on OS X. Docker for Mac doesn't seem to support client certs, and docker-machine stores them inside the VM. |
|
@ahirreddy: Any progress on this? |
This PR allows specifying SSL certificates (client key and cert chain) for a specific domain (or set of domains). The transport pool wrapper now exposes a
add_certificatemethod that mimics the underlying httplib2.Http transports that it's wrapping.