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 username and password for mongodb #1813

Merged
merged 8 commits into from
May 23, 2024

Conversation

rmunn
Copy link
Collaborator

@rmunn rmunn commented May 20, 2024

Fixes #1787

Description

Add env vars MONGODB_USER and MONGODB_PASS to optionally specify authentication when connecting to MongoDB. Some manual setup on the MongoDB server(s) should be done before this is deployed; see #1787 (comment) for details. Once the appropriate admin user has been created and Kubernetes secrets set, this PR can then be deployed to staging and production.

Note that sillsdev/LfMerge#336 will need to be merged first, and a new LfMerge version built. The version number in lfmerge/Dockerfile can then be bumped as part of this PR.

Screenshots

None

Checklist

  • I have labeled my PR with: bug, feature, engineering, security fix or testing
  • I have performed a self-review of my own code
  • I have reviewed the title & description of this PR which I will use as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have enabled auto-merge (optional)

Testing

Testers, use the following instructions against our staging environment. Post your findings as a comment and include any meaningful screenshots, etc.

  • Log in
  • Send/Receive a test project if you already have one
  • Delete the test project if it exists
  • Clone it afresh from Language Depot

If those all continue to pass, then the necessary setup steps (creating an admin user and giving it a password, and creating Kubernetes secrets for that username and password) were correctly completed.

@rmunn rmunn added the engineering Tasks which do not directly relate to a user-facing feature or fix label May 20, 2024
@rmunn rmunn self-assigned this May 20, 2024
Copy link

github-actions bot commented May 20, 2024

Unit Test Results

362 tests   362 ✅  12s ⏱️
 37 suites    0 💤
  1 files      0 ❌

Results for commit 15e532f.

♻️ This comment has been updated with latest results.

@rmunn
Copy link
Collaborator Author

rmunn commented May 20, 2024

Once sillsdev/LfMerge#336 is merged, I'll update this PR with the new version of the LfMerge docker image and then mark it ready for review. I don't expect many changes between now and then, so if you want to give this an early review, @megahirt, go ahead.

@rmunn rmunn requested a review from megahirt May 20, 2024 09:11
@rmunn
Copy link
Collaborator Author

rmunn commented May 20, 2024

LfMerge has now completed a build with auth built in, so I've pushed a commit bumping the LfMerge version. It's currently just an alpha build of LfMerge so we'll need another version bump later on once LfMerge releases a non-alpha build, but this is enough ot test the PR with. So it's ready for review now.

@rmunn rmunn marked this pull request as ready for review May 20, 2024 09:34
rmunn added 5 commits May 21, 2024 10:16
If MONGODB_USER and MONGODB_PASS are set, they will be used to
authenticate to the MongoDB server. If they are not set, then a
connection request with no authentication will be sent (as per the
existing behavior). This allows us to deploy this change, then set
MONGODB_USER and MONGODB_PASS later and have those changes picked up
without a redeployment.

Note that a corresponding change to LfMerge will also be needed.
LfMerge env vars won't be used until we deploy a new build of LfMerge
that looks for them, which will be in a PR on the LfMerge repo.
LfMerge now has an alpha build that handles auth. Once we've proved that
it works, we'll release a full build of LfMerge and bump this version
number again.
Now the check for the `MONGODB_USER` and `MONGODB_PASS` env vars works
correctly because it has the right syntax.
Uncommenting these two lines will enable MongoDB auth on local dev. Do
not do so until you have created the `admin` user or you may end up
locked out of your local MongoDB.
@rmunn rmunn force-pushed the chore/require-password-for-mongodb branch from 1d30bca to 01fcccb Compare May 21, 2024 03:16
@rmunn
Copy link
Collaborator Author

rmunn commented May 21, 2024

Rebased on top of develop to pull in the base-php changes from #1812.

rmunn added 2 commits May 21, 2024 10:34
This will allow us to change the name of the Mongo database we store our
auth in, if in the future we decide not to go with the default name.
This does not create the actual secrets, which will need to be done at
the Rancher admin UI. This commit simply adds the Kubernetes config to
reference the secrets.
@rmunn rmunn changed the title Chore/require password for mongodb Require username and password for mongodb May 21, 2024
Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'd like to get some clarification on a few things:

  • what is the developer experience? It looks like it works fine out of the box and local dev environment continues without a password?
  • how is this intended to work in the GHA testing environment? Currently tests are failing
  • we need to outline a process to test this change in staging and then again in production. How about we outline those steps in this PR description?

Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

Looks like it's working to me! My jetbrains IDE gave a false positive that it was still able to connect to mongo without a password, but it wasn't actually able to list any databases or anything. When I used mongo compass it gave an error that auth was required to access the db, so it seems good.

running the provided script worked for setup. Maybe we should put that script in the dev docs for future reference?

@megahirt
Copy link
Collaborator

running the provided script worked for setup. Maybe we should put that script in the dev docs for future reference?

Great! I am hoping that new devs won't need it. It's only required if you have an existing mongo database and don't want to drop the mongo volume. A fresh start should work fine.

@rmunn rmunn merged commit 93255b5 into develop May 23, 2024
17 checks passed
@rmunn rmunn deleted the chore/require-password-for-mongodb branch May 23, 2024 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engineering Tasks which do not directly relate to a user-facing feature or fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

secure mongodb with a password
3 participants