-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
POSTGRES_PASSWORD changes authentication settings unexpectedly #580
Comments
I was just about to report this same bug! It's rather surprising that when you don't set a password for the |
I think that at minimum the image needs a big strong warning at the start of the readme, warning that the default configuration is:
then links to the sections that discuss how to set each up. Plus clearly and explicitly documenting that not passing a |
I've just had my db (luckily a backed-up side-project db) targeted by ransomware. Imagine my surprise when, in trying to find out how they were able to access the db, I managed to connect to the db on my server with a simple Obviously my fault for not checking deployment and default options, I'm not trying to shift blame which obviously belongs to me and noone else. But I incorrectly assumed that, being a super popular docker image of the best relational database, it had a sensible pseudo-secure default, something like "trust all connections coming from localhost, and from anywhere if using the correct password". "Trust anything from anywhere" is in my opinion an option that shouldn't even exist, much less be the default when some tangentially-related environment variable is unset! At the very least, if it really needs to exist, it should be under some very uncomfortable and scary-looking parameter, along the lines of |
The problem is that One idea we've had to help combat this problem is to enforce an explicit Operationally as a side note, I would say there are very few good reasons for exposing the PostgreSQL port externally from the cluster ( |
Hurray! Thanks for your response, @tianon. In the past 20 days I’ve experimented with postgres and... For some strange reason you can’t supply empty string as a password. You can easily set empty sting as a password, but then you just can’t login with it:
There's no problems with My problem is that insanely permissive
Yes there are. But if your image is intended only for usage in trusted networks, then please, let’s reflect this in the documentation My proposals, updated given the weird empty-password behavior:
I'm not talking about making postgres image secure by default, this will hurt usability and as we all know, usability is much more important than security when it comes to databases |
@tianon ^^^^^ Is somebody playing Curb Your Enthusiasm theme or is it in my head?
This misconfiguration vulnerability (or, as you put it "Request") is open for 8 months with no actionable feedback. Try searching github for Maintainers, please. Choose 1, 2, or 3 and I'll implement it. Propose 4th alternative and I'll implement that. Say "we don't care" and close the issue. Do something concrete. |
This really needs to change. It's easy for users to expose their whole DB to the world by default, silently, but that doesn't make it user friendly. It just sets them up with a time bomb that will explode on them later. Yes, it'd be better to use proper service mapping and discovery, but even then you should preferably define some model of authentication beyond "0.0.0.0/0 trust". To mitigate the issues around the lack of a true "localhost" you can expose a variable for allowed subnets, for example. |
Opened #658. Let me know if that is sufficient or if you have any suggestions for improvement. |
@yosifkit Thanks. Per comment on PR that looks sensible; making the user make an explicit decision is good. Better, it's BC so long as they're already doing the sensible thing and setting |
Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <tylercloke@gmail.com>
Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <tylercloke@gmail.com>
Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <tylercloke@gmail.com>
Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <tylercloke@gmail.com>
Recently, the docker image's default security config was changed because it defaulted to super insecure settings: docker-library/postgres#580 We didn't care about that since we were only using it locally for testing. Updating the Makefiles for auth related services to work with newer versions of the docker postgres image. Also cleaned up some of authn's use of PG_URL so there was less inconsistency and hard-codedness. Signed-off-by: Tyler Cloke <tylercloke@gmail.com>
This is in response to a recent change in the PostgreSQL Docker image. Either the database must be configured to not check passwords, i.e. `POSTGRESQL_HOST_AUTH_METHOD=trust`, or a password must now be set. For explanation and context, see: - docker-library/postgres#658 - docker-library/postgres#681 - docker-library/postgres#580
This is in response to a recent change in the PostgreSQL Docker image. Either the database must be configured to not check passwords, i.e. `POSTGRESQL_HOST_AUTH_METHOD=trust`, or a password must now be set. For explanation and context, see: - docker-library/postgres#658 - docker-library/postgres#681 - docker-library/postgres#580 - https://discuss.circleci.com/t/postgresql-image-password-not-specified-issue/34555
* Dockerfile: Use better PhantomJS URL GitHub's CDN is more reliable than BitBucket's. (This is the URL we originally used as of PR #435, which was the initial implementation of our Docker setup.) * docker-compose.yml: Add password for PostgreSQL db This is in response to a recent change in the PostgreSQL Docker image. Either the database must be configured to not check passwords, i.e. `POSTGRESQL_HOST_AUTH_METHOD=trust`, or a password must now be set. For explanation and context, see: - docker-library/postgres#658 - docker-library/postgres#681 - docker-library/postgres#580 - https://discuss.circleci.com/t/postgresql-image-password-not-specified-issue/34555
Squashed commit of the following: commit a3ba4b7 Author: DeeDeeG <DeeDeeG@users.noreply.github.com> Date: Fri Apr 3 22:10:09 2020 -0400 Update Node.JS and Ruby Dependencies (#617) * Gemfile[.lock]: Update rails to 5.2.4.2 Also update its dependencies, as required. * Gemfile[.lock]: Update grape and grape-swagger Also update their dependencies, as needed. * Gemfile[.lock]: Update activeadmin * Gemfile: Pin sprockets to "< 4" The 4.x major version upgrade requires some configuration changes. Pinning keeps the app from breaking when doing `bundle update`. * Gemfile.lock: Update all packages * yarn.lock: Update all packages commit 15fe9f7 Author: DeeDeeG <DeeDeeG@users.noreply.github.com> Date: Thu Apr 2 16:02:39 2020 -0400 Ruby: Update from 2.5.7 to 2.5.8 (#618) commit cc9f2a7 Author: DeeDeeG <DeeDeeG@users.noreply.github.com> Date: Tue Mar 17 15:02:57 2020 -0400 Update docker config (#616) * Dockerfile: Use better PhantomJS URL GitHub's CDN is more reliable than BitBucket's. (This is the URL we originally used as of PR #435, which was the initial implementation of our Docker setup.) * docker-compose.yml: Add password for PostgreSQL db This is in response to a recent change in the PostgreSQL Docker image. Either the database must be configured to not check passwords, i.e. `POSTGRESQL_HOST_AUTH_METHOD=trust`, or a password must now be set. For explanation and context, see: - docker-library/postgres#658 - docker-library/postgres#681 - docker-library/postgres#580 - https://discuss.circleci.com/t/postgresql-image-password-not-specified-issue/34555
* db/seeds.rb: Give restroom entries an edit_id (#567) Only applies during development and testing when we use the "db/export.csv" data. Doesn't affect production, which uses the real data in its db. * Explain how to run individual tests and access psql (#570) Also, clarify that there are two containers, web and db, rather than just one; these can be viewed using docker ps * Make filter with "focus" class more readable * Remove unused li * Allow dropdown menu text to wrap and fit within the dropdown Add border bottom to give each menu item more separation * yarn.lock: Update jquery (#587) * Dockerfile: Update and streamline steps (#586) Does effectively the same things as before, but now in a simpler/faster way. Some of the changes take inspiration from @btyy77c's dockerAlpine branch: https://github.com/btyy77c/refugerestrooms/blob/dockerAlpine/Dockerfile The PhantomJS install is based on (mostly copy-pasted from) @nkovacs' phantomjs image from Docker Hub: https://github.com/nkovacs/selenium-standalone-phantomjs/blob/c5f6bba218472270/Dockerfile#L19-L22 * Dockerfile: Get latest Node.js in a major version (#589) (Also installs Node.js in /usr/local/ instead of installing Node.js in the root directory.) With this updated script, we specify just a major version and the script picks the latest minor/patch version within that. -- Nodejs.org does most of the work by maintaining the "latest-v[MAJOR]" folders; We only need to parse the "SHASUMS256.txt" file from there, and pick the "linux-x64" variant, which works with our Docker setup. At this point we can use the known directory URL, plus the filename extracted from "SHASUMS256.txt", and download with curl, or wget, etc. (e.g. "curl -L https://nodejs.org/dist/latest-v10.x/node-v10.16.0-linux-x64.tar.xz -o nodejs.tar.xz") -- There is no "latest-LTS" folder or similar, so automatically getting the latest LTS version would be more difficult. We could search "nodejs.org/dist/" for folders with the name "latest-[LETTERS-ONLY-STRING]", which would be the folders of all the LTS codenames. Among these, the one with the alphabetically last name is the latest LTS. This would work at least until around 2040, when they may have to loop around and re-use some earlier letters (a, b, c, etc.) * Update Node.js and Ruby dependencies (#590) * yarn.lock: Update Node.js packages * Gemfile.lock: Update gems * Add Bugsnag to readme To fufill the open source agreement, we have to link to bugsnag now in our readme. * .travis.yml: Use minimal base image for Travis CI We do all the setup/build steps inside a Docker container, so we don't need ruby tools outside of Docker (on the Travis CI virtual machine instances). Should save about 20 seconds of Travis CI build time. -- Inspired by @btyy77c who did this first at their dockerAlpine branch: - btyy77c@393cf46 Documentation at Travis re: minimal/generic images: - https://docs.travis-ci.com/user/languages/minimal-and-generic/ * docker-compose.yml: Use postgresql:alpine image This (the Alpine Linux-based postgresql image) is a smaller image than the debian-based postgresql image, so it should be marginally faster to download. Seems like a good idea in general, to speed up build times (even outside of Travis CI). Also should save some disk space for developers. -- Inspired by the general concept of @btyy77c's dockerAlpine branch: - https://github.com/btyy77c/refugerestrooms/commits/dockerAlpine Docker Hub documentation on the alpine vs debian postgres images: - https://hub.docker.com/_/postgres#image-variants * layouts/_footer.html.haml: Remove tumblr link * stylesheets/.../common: Remove tumblr icon stlye * en/footer.en.yml: Remove string for tumblr blog This isn't needed anymore, since we have removed the tumblr link from the footer. * removed tumblr from about page (#593) I noticed we were removing tumblr, here's one more instance * Create about.fil.yml (#465) * Filipino Translation devise.fil.yml (#454) * Create devise.fil.yml * Update devise.fil.yml * Update for devise.fil.yml @100% Translation * Update and rename devise.fil.yml to devise.fl.yml * Update and rename devise.fl.yml to devise.fil.yml * 100% completed for restroom.fil.yml file (#467) * Create restroom.fil.yml * Translations for EN to FIL Issue 451 (#556) Translated files from EN to FIL #451 * config/locales/fil/: Remove tumblr See #592 and #593 * Updated filipino translations * switched sass-rails gem (#595) * switched sass-rails gem (sass-rails --> sassc-rails) * Changed the word `restroom` to `banyo` to be mroe understandable to most Filipinos * Fixed some unnoticed words that needed some changes in translation * additional translation changes * config/application.rb: Add 'fil' locale (Filipino) Enables translations as merged in #596 * .travis.yml: Set "dist" to "trusty" (#600) Should allow our CI tests to pass while we investigate test failures on xenial and newer. * production.rb: Fix i18n.fallbacks deprecation warn * config/application.rb: Add Tagalog (:tl) locale * production.rb: I18n fallbacks for :tl --> :fil We don't maintain separate translations for "Tagalog" and Filipino, since they are arguably the same language. However, Firefox only allows users to set "Tagalog" as preferred, and Chrome only allows users to set "Filipino" as preferred. To support both browsers, we must support both the "Tagalog" and the "Filipino" locales. (These locales use the "tl" and "fil" locale codes, respectively.) * config/application.rb: Set default locale to "en" * package.json: Update swagger to master with patch * yarn.lock: Commit updated (indirect) dependencies * yarn.lock: Update all packages * Gemfile[.lock]: Update devise, simple_form * yarn.lock: Upgrade swagger-ui's dependencies * Ruby: Upgrade from 2.5.3 to 2.5.7 * Dockerfile: Work around an issue with phantomjs When running the tests, cliver tries to check that PhantomJS's version is within a certain range, by running "phantomjs --version". The "phantomjs --version" command fails for some reason on the new ruby:2.5.7-slim Docker base image. Perhaps because the new Docker image is based on Debian 10 "Buster," whereas the old Docker image was based on Debian 9 "Stretch"? This commit's workaround allows "phantomjs --version" to work again. * Fix Travis tests failing on distributions other than trusty (#606) * Revert ".travis.yml: Set "dist" to "trusty" (#600)" This reverts commit ac8f6ab. Doing this to run tests on Travis to investigate why they aren't passing. * Explicitly require locations.rb in rspec.rb Tentative fix for tests not passing in xenial but passing in trusty. This might be because different distributions load files in a different order. In xenial, `rspec.rb` might get loaded before `locations.rb`, making `Locations` uninitialized. Explicit require fixes this. * Dockerfile: Upgrade Node from v10.x to v12.x (#603) Node 12 "Erbium" is the newest Long Term Service release. We should either pin a version of Node in our package.json file, or stay on the latest LTS version of Node; Heroku will use the latest LTS version of Node in production if we don't have any versions pinned in our package.json * Webpack Upgrade (#607) * Updated webpacker gem * Upgraded yarn packages * Ran webpack:install process. Working without rails-erb-loader * Added rails-erb-loader to webpack * Fixed include PgSearch warning * Added .dockerignore * PR #607: Minor tweaks/cleanup - Adjust Gemfile[.lock] to specify webpacker within the 4.x series, rather than any version 4.0 or greater. - Delete some duplicate entries in the .gitignore file * CONTRIBUTING.md: Remove the reference to "Cmd + C" The "Cmd + C" keyboard shortcut is for copying text, not quitting programs in the terminal. The proper way to quit programs in the terminal under macOS is "Ctrl + C", the same as Linux. Referring to "Cmd + C" here was based on a mistaken assumption that "Ctrl" on Windows or Linux always gets translated to "Cmd" on macOS. (In fact, some uses of "Ctrl" on Windows/Linux are preserved as-is on macOS. It's a mixed bag.) Deleting the reference to "Ctrl + C", to make the guidance clearer. * db/schema.rb: Commit with underscores in date The date gets underscores added automatically when running migrations on the database. Committing with the underscores so the change isn't flagged by git when no code has been changed. * restrooms_spec.rb: Fix a test (#608) Background: The Mission Creek Cafe in San Francisco has been closed for some time. Google Maps API now resolves "Mission Creek Cafe" to a coffee shop in Washington state. Washington is too far away from our stub restroom entries; No stub restrooms are located near Washington, so no restroom results are shown on our results page for this search. The test expects to see a stub restroom entry on the reults page, but does not see it, and so the test fails. --- Fix: search the Maps API for "San Francisco," not "Mission Creek Cafe" (This returns a lat/long associated with San Francisco not Washington) * Update some dependencies, fix some deprecation warnings (#609) * Gemfile[.lock]: Update simplecov Fixes a deprecation warning * restrooms_spec.rb: Use 'successful' not 'success' Rspec's `be_success` and `.success?` are deprecated. Rspec's `be_successful` and `.successful?` are the non-deprecated versions of this check. (This fixes the associated deprecation warning) * Gemfile.lock: Bump some dependencies Upgraded loofah, puma, rack, and rack-cors, plus their dependencies. * Tweak CSS a bit for narrow screens (e.g. mobile phones) (#610) * CSS: Add some styles for narrow screens For screen widths ~340px or narrower. (Such a narrow screen is found, for example, on the original iPhone through to the iPhone 5S and iPhone SE.) - Makes the "+" icon on the "Add A Restroom" button appear in a more correct-looking position. - Fixes the overlap of the "Refuge Restrooms" text with the "hamburger" drop-down menu button in the header/nav section. - Adds a class via the haml source (.nav-column) to make applying one of the style rules easier. * CSS: No double-padding on nested `.container`s Eliminate double-padding in cases of an [element].container immediately inside another [element].container. (Doing this only directly under the header div, just to be conservative.) The 15px + 15px = 30px of padding on both sides seemed unintentionally wide. Also, I think this looks nicer. Helps with the tight fit on mobile devices, too. (Should affect the header/nav on all pages other than the home page, aka the splash page, due to the way the pages are coded.) * CSS: Center logo and brand name on narrow screens (#611) * _mobile.scss: Lower logo/brand on narrow screens Adjust the CSS "top" property to set the logo and "brand name" ("Refuge Restrooms") slightly lower within the navbar on narrow screens. This is to adjust for the navbar being responsively taller on narrower screens. "767px screen width" happens to be the responsive threshold for that height change for the navbar. * _mobile.scss: Move 342px rules, adjust whitespace Moved the "max 342px" rules to the bottom, so all screen-width-related style rules are in descending order of the sizes that they apply to. (For consistency). Adjusted the use of newlines in this stylesheet to be more consitent. * Update docker config (#616) * Dockerfile: Use better PhantomJS URL GitHub's CDN is more reliable than BitBucket's. (This is the URL we originally used as of PR #435, which was the initial implementation of our Docker setup.) * docker-compose.yml: Add password for PostgreSQL db This is in response to a recent change in the PostgreSQL Docker image. Either the database must be configured to not check passwords, i.e. `POSTGRESQL_HOST_AUTH_METHOD=trust`, or a password must now be set. For explanation and context, see: - docker-library/postgres#658 - docker-library/postgres#681 - docker-library/postgres#580 - https://discuss.circleci.com/t/postgresql-image-password-not-specified-issue/34555 * Ruby: Update from 2.5.7 to 2.5.8 (#618) * Update Node.JS and Ruby Dependencies (#617) * Gemfile[.lock]: Update rails to 5.2.4.2 Also update its dependencies, as required. * Gemfile[.lock]: Update grape and grape-swagger Also update their dependencies, as needed. * Gemfile[.lock]: Update activeadmin * Gemfile: Pin sprockets to "< 4" The 4.x major version upgrade requires some configuration changes. Pinning keeps the app from breaking when doing `bundle update`. * Gemfile.lock: Update all packages * yarn.lock: Update all packages * Implement Google's reCAPTCHA (#566) * Add server reCAPTCHA verification for contacts Added a temporary secret key for testing in .env, which is loaded by the dotenv gem. In production, just put another key in the Heroku env variable settings. * Add reCAPTCHA to contacts submission page * Enable browser form validation by default This gets form input validated on the client side, which gives faster feedback to the user, without the need for a custom solution. This feature is supported in all modern browsers. * Add reCAPTCHA to restrooms page * Make stub for reCAPTCHA verification during tests Co-authored-by: Mikena Wood <mi-wood@users.noreply.github.com> Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com> Co-authored-by: Kai Middleton <kai.middleton@hingehealth.com> Co-authored-by: hkly <hannah.k.yiu@gmail.com> Co-authored-by: Teagan <tkwidmer@gmail.com> Co-authored-by: Joe Wadcan <joe.wadcan@github.com> Co-authored-by: vinzruzell <35182720+vinzruzell@users.noreply.github.com> Co-authored-by: hnarasaki <hnarasaki@indeed.com> Co-authored-by: Bryan Mark Fajutag <fbryanmark@gmail.com> Co-authored-by: Emily Ring <emily_ring@ymail.com> Co-authored-by: Jason Chen <kbtpodifo@gmail.com>
I believe that current authentication configuration in
docker-entrypoint.sh
is less secure than it should be. I have a problem with$POSTGRES_PASSWORD
changing the authentication settings and not adequately warning user about it.Currently if the password is not supplied
host all all all trust
is added topg_hba.conf
, disabling the password checks for all users. I think this goes way too far:host all all all trust
to appear in configuration unless user explicitly setsACCEPT_ANY_PASSWORD_FOR_ANY_USER=true
POSTGRES_PASSWORD
) results in disabled authenticationPOSTGRES_PASSWORD
wasn't set does not convey that password checks were disabled altogether.Sounds like you can fix it by setting the password, but the password would be just ignored.
I had an unpleasant experience of discovering my development DB for a personal project consuming 100% of server's CPU because someone was mining some kind of crypto coin on it.
Initially I was using
POSTGRES_PASSWORD
, but later I decided to move password configuration for "postgres" user together with creating additional, less privileged users into an init SQL file in/docker-entrypoint-initdb.d/
(amazing feature, btw). As you might imagine, I did not believe that this would disable all password checks altogether, and since my server continued to grant access with my password I had no idea that I was completely insecure.I did not expect (and didn't need) high security from DB with an exposed port and without SSL, but having literally no security is a really strange decision.
I've read issues about security of this image (#3 #29 #31), and it seems like "security is annoying and not our problem" argument is winning. I'd much rather have official images be more secure by default, but I feel like this fight is futile. So here are my suggestions, from less bothersome to more secure:
POSTGRES_PASSWORD
is not set (better yet, ifALLOW_EMPTY_PASSWORD
is set), then set it to an empty string and keep themd5
authentication always enabled. That way developers can still easily connect to a DB with empty password, but changing the password would never be silently ignored, like it was in my case.POSTGRES_PASSWORD
is not set (better yet, ifGENERATE_RANDOM_PASSWORD
is set), then set password to a random string and print it on initial launch.Official MySQL image allows to get second or third behavior by setting
MYSQL_ALLOW_EMPTY_PASSWORD
orMYSQL_RANDOM_ROOT_PASSWORD
environment variables. Alternatively, you can setMYSQL_ROOT_PASSWORD
directly. One of the three variables must be set explicitly, or MySQL container fails to launch.For backwards compatibility
ACCEPT_ANY_PASSWORD_FOR_ANY_USER
or similarly explicitly named environment variable could be added. If it is set to true thenhost all all all trust
is added to the config.I'm willing to write the necessary code, but I'd like to know beforehand which approach would be accepted. IMHO MySQL's approach is the correct one, it is both flexible and explicit, but it's up to the maintainers.
The text was updated successfully, but these errors were encountered: