Fix Insecure Default Configuration#1554
Conversation
Accidentally pushed with OED_PRODUCTION set to yes
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @Zachary-Squires & @Zach-O-Bates for this contribution. I've made some comments to consider. I'm happy to discuss them. I did some testing but not complete as I'm going to wait for any changes to do that.
Added: 1. Banners for the make sure to change this value notifications, also added a notification for the token secret. 2. Fixed spelling error. 3. Added spaces to comments. 4. A variable that checks what the installation type is at the start and is used for all checks.
Added comments in docker-compose.yml to alert users as to the fact passwords are only drawn from this file once. Added something to the .env file to show the code that the password has already been generated. Changed a redundant if statement to an else.
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zachary-Squires Thank you for the updated version. A number of comments are now resolved. I added a new comment on a couple of old ones and added a few new ones. Please let me know if you need anything.
Changes to address feedback on 1/19, added/edited comments, changed changePass.js to changePostgresPass.js
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zachary-Squires I'm trying to get all PRs up-to-date so I looked at this one even though you are still working on it. I resolved some comments and left one to think about along with any other open ones.
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
Thanks to @Zachary-Squires for addressing many previous comments. I've made a few more to consider and commented on a few older ones. I think this is getting close.
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zachary-Squires Thank you for the updated version. I'm sorry it took me a while to get back to this. I have gone through all the comments. I added a new comment on any one that is outstanding and these should be the only unresolved ones. Can you please comment on each one so I know what you are thinking. I also added one comment that relates to a previous one but only part of it so I thought this would be easier (new one not resolved but older one is resolved). I also hid the duplicate comments from before to make knowing what is active easier. It may be the case that the new ones get duplicated but I don't yet know. I am waiting on a version that is hopefully the final one to retest everything. Please let me know if anything is not clear or I can help.
There was a problem hiding this comment.
First, I want to thank @Zachary-Squires for the ongoing efforts to make this as good as possible. I tried a reasonable level of testing and it seems to work correctly. As a result, I resolved all the previous comments. I did make a few new comments to consider but I'm hopeful they are not anything of significance and finishing this up should not be difficult.
Here are some overall comments to consider:
- I think you did the merge of development to resolve the merge conflict which was nice. However, per the Discord developer channel message that I recently posted and pinned, this breaks OED. To get around this I reset head to the commit before that merge and it worked fine. The other issue needs to be resolved before merging this PR but whatever changes are made hopefully that should not impact this PR.
- For manual reset of password:
- The short-cut version to start OED without initializing the DB works. (Just for information to keep in the record).
- It allows any password including very short ones. Should OED require the provided password to be at least some number of characters (say 12)? This should not impact the automatically generated ones which are very long.
- If you stop OED, manually delete the postgres_data/ directory & remove the Docker oed database container then it fails (either with production yes or no). Maybe it is trying to use the new passwords but since it is initializing it starts with the default ones? Whatever the reason, I tried deleting the two Postgres lines in .env (POSTGRES_PASSWORD=... & OED_DB_PASSWORD=...) and starting OED and that did work. This is unusual but sometimes done by developers to fix issues or during testing. However, it is uncommon for developers to change the default passwords and it isn't required in your new system. Unless you have another idea, I say it is fine and we add to the developer docs that this extra step is needed if you changed the passwords.
- What is the status of the limitation listed in the PR description?
There was a problem hiding this comment.
@Zachary-Squires Overall, this looks good. I made one comment about my testing status that suggested a small change. Let me know if you want to do it or not (or have thoughts). Except for being able to fully test this looks except for the comment.
The only other open item was my question about this limitation in the PR description:
In order to fully remediate the insecure default configuration we will need to use a separate docker file when launching in development and production, this will be done in a separate PR since it will be intrusive to every OED developer.
Could you please comment on the status of this? I want to figure out if there is a plan or an issue needs to be opened. Update: This was addressed in PR #1615 so it is resolved for this PR.
Please let me know if anything is unclear, have thoughts or need anything.
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Zachary-Squires Thank you for the update. I believe the PR is down to this:
- I made one new comment to consider.
- There is still one comment (not to a line of code) that needs a response on the status.
- This will have to wait for final testing when the development install issue is resolved. That isn't something this PR needs to do but I am noting.
Please let me know if you have any thoughts or need anything.
| updateEnvFile(newPostgresPassword, newOedPassword); | ||
|
|
||
| console.log('********************************************************************************'); | ||
| console.log('Generated a secure PostgreSQL and OED password and applied them successfully.'); |
There was a problem hiding this comment.
I've managed to get the DB passwords reset so I could test some more. I noticed that this message is always displayed even if the user provided the password. Could it be changed so it says this when generated and, if possible, that it used the user provided ones if not? I do see that it is possible of for only one of the two passwords to be provided to the script so that may make it necessary to do a little tracking.
There was a problem hiding this comment.
I looked at it and thought that maybe it just telling the user what passwords were used would accomplish the same purpose, that way you can both verify which method of generation was used and for which password and it won't say "generated" if it was manual. Let me know if the change works.
There was a problem hiding this comment.
@Zachary-Squires What you did was fine but I am always a little nervous about dumping a password to a terminal. The person might not notice and leave it so others can see it. I don't think it is a big deal but putting it in a file seems a little better. Of course, if they use the args to set the new password then it is in the history but hopefully they are aware they did this. I'm open to other thoughts.
|
@Zachary-Squires I'm trying to be sure the status of comments. I think the only older one that I wanted your input is:
|
- This brings in a change in the open PR OpenEnergyDashboard#1554 to centrally set the install mode. The hope is that it will cleanly merge when that is added. - It adds a mechanism to know if the developer config is used and then tests in the install for probable misuse. - The OED install mode for the standard config file is set to production so the old install mechanism will still work for developers. The plan is to remove once the transition is done so there is no need for an OED site to change the value.
Tell to see .env as in other places.
- Enforces 12 character min for all postgres passwords. - Minor msg formatting update. - Remove msg with new postgres passwords.
This is a commit to get a working but not perfect version saved. It has the following changes: - Creates shared environment variables for the web and database containers in docker-compose.yml and contains the OED DB password. The goal was to use this so the DB startup could use that password. Otherwise, on initial startup, OED would use the default value and then the OED software would fail when used because it used a different password from the environment. Unfortunately, this is not working correctly so the value is currently duplicated in the DB setup. - The database build now passes args to send the desired password. The DB docker file now uses that value to edit the init.sql file to match. - Fixes that the install args comments were in the wrong place. - The syntax for setting environment variables was changed to use : (newer way). The ? could not be used with this value without quoting it. It may be okay to do others that way but not done. The leading - was removed as not longer needed/desired. This was done in both docker files. - Removes the version in docker-compose.yml. This gets rid of the warning on OED install and is no longer used. - A TODO was added since testing found the cypress UI testing is not working. It crashes after startup. It is unclear why and the current changes probably are not the cause.
- This is better but needs more testing/work. However, it is time to commit. - Make so knows if production install and acts accordingly. - The password check function now returns if the password should be changed rather than if it generated a new one so it works in all cases. - Put all three environment variables that can change in .env not just ones changed. This seemed better since you cannot change values in the docker config after the initial startup so now all are in that one file. - Fixed bug where did not change default password if it was not changed. Should not happen once everything works but safer to make it the first check. - Added the postgres password to the common environment variables in docker-compose.yml since needed by install during web process to get an password updates. - Update npm script call to change passwords so new name and directly calls the function. This avoids the default running since that can cause errors if improperly used and it may not be supported in the future. - Changed the password change file name to match script and reflect new usage. - There are debug statement left in for now.
|
I've been working on this for several days. I need some more time to get it to a great place given the changes I decided to make. I will push my changes at times to be sure they are not lost but it isn't ready/done until I comment about it. |
- Modify code so user can input either password when manually changing. - Fixed up so work in production and with 1 or 2 passwords set. - Don't change password if input is empty. No longer need to ask if want to change if console input. - Remove unused input function since replaced with usual OED one. - Add output if new password same so unchanged. - Fix issue that script no finish if no passwords changed and let user know.
- Added comment that if either password is empty in the docker file then install fails. - Clarified comment that 12 character min is now enforced for DB passwords. - Removed check for production of now since that cannot happen. - Added TODO comment that check for production switch does not properly working.
- Change so empty password always skips the update. - Fix a few small items. - Remove debug statements.
|
This is to note that this is probably close. I have tried to read carefully and test weird cases. The biggest remaining item is that the common environment variables in the docker-compose.yml file are not doing what is hoped so they are duplicated. It works now but you can cause issues if you don't change both the same way. @Zachary-Squires I have made non-trivial changes to get it to be how I thought was best (no reflection on you as I changed my mind while looking this over). I know your semester is over but would you be able to look my changes over? Also, line 223 and the following lines in changePostgresPasswords.js have two TODO comments about if it is needed. I was unsure if they were needed to update the values for other places or if it was a hold over from before the later lines set the value involved. Any insight from you would be appreciated. |
2 participants
Description
Changes installOED.sh and docker-compose.yml to automatically generate POSTGRES_PASSWORD and OED_TOKEN_SECRET when OED is launched in production mode. This should remediate an issue where OED_TOKEN_SECRET could be forged to allow a login as an admin. Will also make the postgres database more secure as the password will be much more robust by default. Additionally, installOED.sh now specifically checks if OED_PRODUCTION is no rather than just checking if it is not yes.
Developed and implemented by:
Zachary Squires - https://github.com/Zachary-Squires
Zachary Bates - https://github.com/Zach-O-Bates
Type of change
Checklist
Limitations
In order to fully remediate the insecure default configuration we will need to use a separate docker file when launching in development and production, this will be done in a separate PR since it will be intrusive to every OED developer.