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

Require authentication for clients #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

geek
Copy link
Contributor

@geek geek commented Mar 7, 2017

  • Authentication mode is now enabled
  • Replica members now use a keyfile for authentication
  • Added script.sh file for easy _env creation
  • Update to latest MongoDB release

@geek geek requested a review from tgross March 7, 2017 20:52
Copy link
Contributor

@yosifkit yosifkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on the mongodb setup script.

#!/bin/bash

echo "Start MongoDB without access control."
mongod --port 27017 &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be mongod --fork --bind_ip 127.0.0.1 --logpath /dev/stdout --config /etc/mongod.conf to ensure that it runs with only local access, has the same config as the real instance, and when it returns will be ready for access and so we won't need the while ! nc .... The reason for the bind_ip is to prevent external services from getting a connection followed by a rejection (since auth is on and no user created), and instead just prevent connections altogether.

Not sure if a --port is necessary, since the default is 27017.

(Similar discussion on the official image docker-library/mongo#53)

chmod 400 /etc/mongod.key

echo "Overwrite setup_mongo.sh so that this is a one-time setup"
echo "#!/bin/bash" > ./setup_user.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this overwrote itself (setup_mongo.sh), this would only work on restarts of the current container, but would not prevent a second user creation when a user reuses an old data directory (like docker-compose when run on non-triton). In looking to add similar functionality to the mongo official image (docker-library/mongo#53 (comment)), we didn't yet have a way to detect that this was an already initialized database directory to prevent trying to create the user a second time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to solve this either.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkdir an empty dir into a well-known location in the data directory. If the mkdir fails (which it will if it already exists), we can just bail out there.

@@ -0,0 +1,21 @@
#!/bin/bash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to have a set -e here so that failures of individual lines will cause the script to stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@tianon
Copy link
Contributor

tianon commented Mar 15, 2017

FYI, I've filed docker-library/mongo#145 upstream to get a general solution to the initialization/user creation problem.

@sberryman sberryman mentioned this pull request Apr 7, 2017
Copy link

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no-op comment to get this old PR off my pulls page 😀 )

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.

4 participants