Fix Insecure Default Configuration#1554
Fix Insecure Default Configuration#1554Zachary-Squires wants to merge 24 commits intoOpenEnergyDashboard:developmentfrom
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.
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.