-
Notifications
You must be signed in to change notification settings - Fork 879
Serve a TLS endpoint if REGISTRY_TLS_VERIFY is set and GUNICORN_OPTS is not #693
base: master
Are you sure you want to change the base?
Conversation
5215470
to
ce54641
Compare
@@ -12,6 +12,7 @@ FROM ubuntu:14.04 | |||
RUN apt-get update \ | |||
# Install pip | |||
&& apt-get install -y \ | |||
curl \ |
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.
lol indentation
|
@@ -19,6 +20,10 @@ RUN apt-get update \ | |||
libevent1-dev \ | |||
&& rm -rf /var/lib/apt/lists/* | |||
|
|||
# get generate_cert | |||
RUN curl -L -o $ROOTFS/usr/local/bin/generate_cert https://github.com/SvenDowideit/generate_cert/releases/download/0.1/generate_cert-0.1-linux-amd64/ && \ |
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.
$ROOTFS
? Someone copy-pasta'd too much. 😉
3848eb9
to
a7a1a04
Compare
@@ -37,4 +42,4 @@ ENV SETTINGS_FLAVOR dev | |||
|
|||
EXPOSE 5000 | |||
|
|||
CMD ["docker-registry"] | |||
CMD ["/docker-registry/run.sh"] |
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.
You mean:
ENTRYPOINT ["/docker-registry/run.sh"]
CMD ["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.
So it might be good to rename run.sh
now to be less misleading. 😉
8e1f742
to
5890418
Compare
@@ -19,6 +20,10 @@ RUN apt-get update \ | |||
libevent1-dev \ | |||
&& rm -rf /var/lib/apt/lists/* | |||
|
|||
# get generate_cert | |||
RUN curl -L -o /usr/local/bin/generate_cert https://github.com/SvenDowideit/generate_cert/releases/download/0.1/generate_cert-0.1-linux-amd64/ && \ |
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.
It's not a script; it's a compiled Go binary.
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.
@dmp42 we can rewrite it in bash if you want with openssl, will take some time though.
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.
wonder if we should keep the ENTRYPOINT
in python and push the extra logic about in-place cert generation to generate_certs
esac | ||
|
||
x=0 | ||
for f in /ssl/registry.{key,crt}; do |
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 wonder if we should assume that it's named registry.*
or just assumed they get passed as their own bind mount?
5890418
to
5ab86d7
Compare
@dmp42 @proppy @ewindisch |
8aa5c8d
to
eef7d1c
Compare
This is done by setting GUNICORN_OPTS to some default value, expecting the following files to be present: * /ssl/ca.crt * /ssl/registry.cert * /ssl/registry.key Signed-off-by: Tibor Vass <teabee89@gmail.com>
@wking @stevvooe @bacongobbler what do you think? |
On Mon, Nov 10, 2014 at 04:01:55PM -0800, Olivier Gambier wrote:
I'd just add this to the docs for: GUNICORN_OPTS='[--ssl-version, 3, --certfile, /ssl/registry.cert, --keyfile, /ssl/registry.keys, --ca-certs, /ssl/ca.crt]' but if folks want a shortcut environment variable for that, I'll go |
LGTM and +1 on separation of concerns, though users would probably like to have e.g. I usually like to have my certs located at |
On Mon, Nov 10, 2014 at 04:32:28PM -0800, Matthew Fisher wrote:
In this case I'd really rather they just used GUNICORN_OPTS directly. |
@bacongobbler @wking thanks for chiming in! This is a PR to facilitate TLS for registry users. I agree that we should document GUNICORN_OPTS, but mounting certs to /ssl seems to me a more user-friendly API. If people want to customize anything, they can do so with GUNICORN_OPTS. We could debate having Usage: |
On Mon, Nov 10, 2014 at 05:33:17PM -0800, Tibor Vass wrote:
That works for me too, as does encouraging folks to terminate their |
@wking The goal is to have simple TLS instructions on README.md. We could add a link saying |
On Mon, Nov 10, 2014 at 06:48:44PM -0800, Tibor Vass wrote:
I'd just say: The registry uses Gunicorn to manage workers. If you want to setup
That should be enough of a sketch for folks who are already familiar
I'm not sure its the recommended way, but it's certainly one way you |
if not gunicorn_opts and env.source('REGISTRY_TLS_VERIFY'): | ||
gunicorn_opts = ['--ssl-version', ssl.PROTOCOL_TLSv1] | ||
for k, v in { | ||
'--certfile': '/ssl/registry.cert', |
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 don't see the benefit of enforcing these non-standard, default directory paths when simply adding them to GUNICORN_OPTS
would suffice and actually provides more flexibility. Otherwise, I'd say make an environment variable for each path.
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.
the same discussion was posted above: #693 (comment)
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.
Apologies for the repetition. Let's figure out whether or not we want to break these out so this PR can get merged.
If
REGISTRY_TLS_VERIFY
is set, butGUNICORN_OPTS
is not, then serve via a TLS endpoint instead of plain HTTP.This is done by setting
GUNICORN_OPTS
to some default value, expecting the following files to be present:Signed-off-by: Tibor Vass teabee89@gmail.com