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

Add POSTGRES_PASSWORD and USER, with warning on no password #36

Merged
merged 2 commits into from
Nov 20, 2014
Merged

Add POSTGRES_PASSWORD and USER, with warning on no password #36

merged 2 commits into from
Nov 20, 2014

Conversation

yosifkit
Copy link
Member

Fixes #31

@@ -9,8 +9,29 @@ if [ "$1" = 'postgres' ]; then

sed -ri "s/^#(listen_addresses\s*=\s*)\S+/\1'*'/" "$PGDATA"/postgresql.conf

{ echo; echo 'host all all 0.0.0.0/0 trust'; } >> "$PGDATA"/pg_hba.conf

TYPE='CREATE'
Copy link
Member

Choose a reason for hiding this comment

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

Lowercase/camel for "local" vars please. 👍 (and in that case, type is already an instruction so you'll need to choose a new word like op or something 😄)

@tianon
Copy link
Member

tianon commented Nov 20, 2014

Approach seems sane to me, just mostly minor style nits! 👍

@md5
Copy link
Contributor

md5 commented Nov 20, 2014

Won't this fail in the case that POSTGRES_USER != 'postgres' and POSTGRES_PASSWORD is blank? It will add a line to pg_hba.conf for a user that doesn't exist.

@tianon
Copy link
Member

tianon commented Nov 20, 2014

Ahahaha, good catch @md5.

@md5
Copy link
Contributor

md5 commented Nov 20, 2014

👐


TYPE='CREATE'
: ${POSTGRES_USER:=postgres}
if [ "$POSTGRES_USER" = postgres ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add quotes here? It's more symmetrical
if [ "$POSTGRES_USER" = "postgres" ]; then

@yosifkit
Copy link
Member Author

Fixed many nits; fixed specifying user and no pass.


# check password first so we can ouptut the warning before postgres
# messes it up
pass=
Copy link
Member

Choose a reason for hiding this comment

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

To be consistent, shouldn't this be down in the else with authMethod=trust ?

@tianon
Copy link
Member

tianon commented Nov 20, 2014

LGTM other than that one minor consistency nit

@tianon
Copy link
Member

tianon commented Nov 20, 2014

LGTM

tianon added a commit that referenced this pull request Nov 20, 2014
Add POSTGRES_PASSWORD and USER, with warning on no password
@tianon tianon merged commit 40cfe26 into docker-library:master Nov 20, 2014
@tianon tianon deleted the password branch November 20, 2014 18:18
@yosifkit
Copy link
Member Author

Fixed! (and a sneak version bump to 9.4 for rc1)

@md5
Copy link
Contributor

md5 commented Nov 20, 2014

Nice. Do you think this will be up in the Registry today? I'll build a new version of my postgis image once it is to pick up the changes.

@yosifkit
Copy link
Member Author

👍, I am working on the docs for it and I'll poke @tianon to make the PR for official-images.

@jcf
Copy link

jcf commented Dec 1, 2014

This caused a small issue for us. We were using a specific user to connect to the database, and that user was blocked by this change.

The solution we opted for (as this is a development environment) was to use a single user for everything, but in production we of course do things differently.

@mmarzantowicz
Copy link
Contributor

You can also create additional users and databases during container initialization (in custom script).

@dcosson
Copy link

dcosson commented Dec 3, 2014

This also broke our dev environment, we have a couple different users in our setup to mirror prod and now it seems to be not possible to use this postgres container with multiple remote users. Or is there a way I'm not thinking of?

Can you make it an option to use host all all 0.0.0.0/0 md5 as the line in pg_hba.conf?

@mmarzantowicz
Copy link
Contributor

@dcosson what about creating user accounts with passwords for that users? Optionally you can add your own custom line to pg_hba.conf (please see #23).

@tianon
Copy link
Member

tianon commented Dec 3, 2014

Indeed. Also, pg_hba.conf lives in the data directory, so you can even provide an entirely custom pg_hba.conf after the database is initialized.

@dcosson
Copy link

dcosson commented Dec 3, 2014

@mmarzantowicz sorry not sure I understand, we do create user accounts with passwords but we have multiple user accounts and it looks like the entrypoint script will only allow access by 1 user via the POSTGRES_USER env variable.

Otherwise we can do as suggested using the hook in PR #23

@yosifkit
Copy link
Member Author

yosifkit commented Dec 3, 2014

@dcosson, if you need more than the one account created by POSTGRES_USER then you will need to add the other users in init scripts as you noted, but you will also need to add lines to pg_hba.conf or replace it with your own file like @tianon suggested.

@dcosson
Copy link

dcosson commented Dec 3, 2014

Makes sense, thanks for the replies.

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.

Default pg_hba is extremely insecure
7 participants