Skip to content
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

nginx does not support bcrypt when using auth_basic #4332

Merged
merged 1 commit into from
Aug 29, 2017

Conversation

vidarl
Copy link
Contributor

@vidarl vidarl commented Aug 23, 2017

Registry only support bcrypt when using basic auth.
However, nginx does not support bcrypt, only md5....

Without this change you'll get error in the nginx log like
[crit] 5#5: *1 crypt_r() failed (22: Invalid argument)

Ref. https://stackoverflow.com/questions/31833583/nginx-gives-an-internal-server-error-500-after-i-have-configured-basic-auth

edit:
So to conclude, the example in registry/recipes/nginx.md creates the password hashes using bcrypt, while the libc in the nginx image being used doesn't support bcrypt.
This PR fixed the problem by changing hashes from bcrypt to md5, while the more correct way (as pointed out by Stephen ) would be to switch to a image with a libc which supports bcrypt.

Therefore I am reverting this fix and change to nginx:alpine image in #4489

@mdlinville
Copy link

@stevvooe PTAL

@mdlinville mdlinville merged commit 9ccf803 into docker:master Aug 29, 2017
@stevvooe
Copy link
Contributor

nginx actually supports whatever is supported by libc crypt_r (the error message even makes this clear :) ). Certain operating systems (debian, if I remember correctly) omit support, making it seem like nginx lacks support.

This change either needs to be rolled back or a note needs to be made on how to test for this. bcrypt (or similar strong hash) should be used on modern deployments.

@stevvooe
Copy link
Contributor

nginxinc/docker-nginx#29 explains the issue in more detail.

@vidarl
Copy link
Contributor Author

vidarl commented Aug 31, 2017

Thanx for the info @stevvooe. I was just trying to get the example described in the doc working...

So, based on your input I guess the correct fix is to use nginx:alphine instead of nginx:1.9.

I can make a new PR for that ( which also enables bcrypt again )

@mdlinville
Copy link

@vidarl that would be appreciated. Thanks for the review, @stevvooe and sorry I didn't wait for it before I merged.

@stevvooe
Copy link
Contributor

stevvooe commented Sep 1, 2017

@vidarl I've been through this same thing (I may have written this...). It is up to you. A simple note indicating that there are options might be enough. Include me on the PR and I can help out.

@mstanleyjones No worries!

vidarl added a commit to vidarl/docker.github.io that referenced this pull request Sep 1, 2017
@vidarl vidarl deleted the patch-1 branch September 1, 2017 12:49
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.

3 participants