-
-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Docker run option to inlcude credentials for private remote reg… #87
Conversation
…istry to remove necessity to login from browser
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.
Hi, thank you for your contribution.
I'm glad that my project is interesting you 😊
Some changes need to be done. I think it's the user who should use the openssl command and not this image
I want this image to be the simplest as possible without extra packages and all dockerfiles should be updated as well
I think I can generalise your idea. We will be able to add many headers instead of only authorisations.
…th to credentials file
I've updated the PR to remove the installation of In addition, I've also added logic such that the user can pass in the path to a credentials file that will be stored on the container. The credentials file should be one line with the format I hope that you find these changes acceptable. Please let me know if you have any questions or concerns! |
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 need to think a bit more if I use your PR, I like the way you do with the file, but this can be a bit more complicated to use for newbies
@@ -25,6 +25,9 @@ server { | |||
#! return 404; | |||
#! } | |||
#! proxy_pass ${REGISTRY_URL}; | |||
#! proxy_set_header Host ${HOST}; |
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.
This is out of the scope and has been fixed with 1321d9b
|
||
if [ -f ${CREDS_FILE} ] ; then | ||
CREDENTIALS=$(cat ${CREDS_FILE} | tr -d \\n) | ||
BASIC_AUTH=$(echo -n "${CREDENTIALS}" | base64 | tr -d \\n) |
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.
This looks better yes 😄 but I found a more generic way, a feature to add any header in the proxy pass
sed -i "s,#!,," /etc/nginx/conf.d/default.conf | ||
|
||
if [ -f ${CREDS_FILE} ] ; then | ||
CREDENTIALS=$(cat ${CREDS_FILE} | tr -d \\n) |
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 a brilliant idea to improve the security!
Supports custom headers when the ui is used as proxy ## Background Headers can be useful in some cases such as avoid sending credentials when you are on the UI (like #87). Or give to the registry server other properties such as `X-Forward-For` or `Server` headers for monitoring. ## How to use ? This is compatible only with static version of the UI and used with `REGISTRY_URL` variable. When you want to add a custom header, add to the registry ui a environment variable or entry in `/etc/nginx/.env` which looks like `NGINX_PROXY_HEADER_Custom_Header`. All underscores (`_`) will be replaced by hyphens (`-`). Some example of custom headers as variable: - `NGINX_PROXY_HEADER_Authorization` for Basic auth credentials - `NGINX_PROXY_HEADER_X_Forwarded_For` for identifying the originating IP address of a client An example is bundled with this PR closes: #87
…istry to remove necessity to login from browser
This could be useful in removing the necessity to login to a private remote registry in browser when UI acts as a proxy. Done for Alpine based containers.