-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
@proppy If possible i'd like to agree on the API before this is merged. At the time of writing, we don't use SECURE=ssl but check for the /ssl directory, we don't use volumes only bind-mounts, and you're not doing the entrypoint and cmd the same way. I hope to get it all sorted out today. |
Hey @tiborvass, didn't meant to cause you more trouble. I'm fine to switching to the upstream convention once yours get merged. |
@tiborvass changed the A few comments about the rational behind the other changes:
Sorry I didn't mention those earlier, most of them came up while I was playing with implementing a PoC w/ |
@mmdriley @ewindisch, crypto noob here: is that "cool" to use a self-signed CA cert as the server cert and a CA key as the server key? |
If you ever need to rev the certificate then you'd have to install a new CA on all clients. Seems poor? |
So you'd recommend to maintain a separate server and CA cert? Note that the CA cert is mount bound in |
@mmdriley generating separate server certs/key, PTAL |
@dlorenc can I get a review? |
So the registry certs need to be, not in One way to get around that is to bind mount /etc/docker/certs.d, but that would expose other registries's certs to the container. |
@tiborvass the upstream "spec" only define the default for So I could imagine it's up to who consume |
@proppy oh right, i was just reading the documentation that needs to be fixed then :) EDIT: or rather, docker has to have escaping for |
@tiborvass, yes the current docs assume |
@@ -53,5 +54,37 @@ else | |||
fi | |||
fi | |||
|
|||
export GCS_BUCKET BOTO_PATH | |||
exec docker-registry $* | |||
if [ -n "${REGISTRY_TLS_VERIFY}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only generate if GUNICORN_OPTS is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
PTAL |
FYI built: |
lgtm, thank you! |
@mmdriley PTAL simplified a lot based on your offline feedback. No more conditional check if the certs exist, and I don't document |
EOF | ||
echo 01 > /ssl/ca.srl | ||
openssl req -subj "/CN=local docker registry CA" -config /ssl/ssl.conf -extensions v3_ca -new -x509 -days 365 -newkey rsa:2048 -nodes -keyout /ssl/ca.key -out /ssl/ca.crt | ||
openssl req -subj "/CN=local docker server cert" -config /ssl/ssl.conf -reqexts v3_req -new -newkey rsa:2048 -nodes -keyout /ssl/registry.key -out /ssl/registry.csr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with "Local CA" and "Local Docker registry".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Seems okay to me modulo above comments. |
@tiborvass can you LGTM before I merge? |
# assuming CA is already in /etc/docker/certs.d | ||
docker run -e REGISTRY_TLS_VERIFY=1 \ | ||
-v /mycerts:/ssl \ | ||
-e GUNICORN_OPTS="['--certfile','/ssl/myserver.cert','--keyfile','/ssl/myserver.key','--ca-certs','/ssl/myca.crt']" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@proppy don't you wanna make sure users are using tlsv1 ? (would need '--ssl-version', 3
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
nsCertType = server | ||
subjectAltName = @alt_names | ||
[alt_names] | ||
DNS.1 = localhost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@proppy this is hardcoded isnt it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, because when we generate cert we really only care about localhost/boot2docker.
If you want to support arbitrary hostname you should bring your own cert for now.
See also #33 for later
@proppy EDIT: works if I pass |
Fixed, PTAL |
LGTM |
generate certs if REGISTRY_TLS_VERIFY
Usage:
while waiting for docker-archive#693 to be merged.
/cc @tiborvass @dmp42 @dlorenc @ktintc @jbeda