Error when POSTGRES_PASSWORD is unset like mysql

[yosifkit]

Add POSTGRES_HOST_AUTH_METHOD to bring back old behavior and be similar to MYSQL_ALLOW_EMPTY_PASSWORD, but add warning when "trust" is used since it disables all passwords

Fixes #580.

[tianon]

This is a breaking change, but I think it's one that makes sense overall (and that the benefit outweighs the pain in this instance).

[Andrew-Morozko]

Thank you for considering my request seriously. I'm much happier with this behaviour.

One little nitpick is the result when both POSTGRES_PASSWORD and POSTGRES_DISABLE_PASSWORDS are set. Currently it ignores the supplied POSTGRES_PASSWORD. If we're going for clarity this should error-out, otherwise POSTGRES_PASSWORD should overrule the POSTGRES_DISABLE_PASSWORDS.

Ether way, I'm much less sympathetic if someone forgets to remove POSTGRES_DISABLE_PASSWORDS from production config, but still, somebody will make that mistake given time...

Proposed patches

Add to docker_verify_minimum_env, before if [ -n "$POSTGRES_DISABLE_PASSWORDS" ]

Error-out on incompatible options:

	if [ -n "$POSTGRES_PASSWORD" ] && [ -n "$POSTGRES_DISABLE_PASSWORDS" ]; then
		cat >&2 <<-'EOE'
			Error: Both POSTGRES_PASSWORD and POSTGRES_DISABLE_PASSWORDS are specified.
			       You must specify one and only one.
		EOE
		exit 1
	fi

OR

Ignore POSTGRES_DISABLE_PASSWORDS on incompatible options:

	if [ -n "$POSTGRES_PASSWORD" ] && [ -n "$POSTGRES_DISABLE_PASSWORDS" ]; then
		cat >&2 <<-'EOE'
			WARNING: Both POSTGRES_PASSWORD and POSTGRES_DISABLE_PASSWORDS are specified.
			         Using POSTGRES_PASSWORD, ignoring POSTGRES_DISABLE_PASSWORDS.
		EOE
		unset POSTGRES_DISABLE_PASSWORDS
	fi

Tested on postgres/12 and postgres/12/alpine, works as intended.

P.S.: Sorry for slow feedback, end of the year crazyness left me with full inbox...

[ringerc]

Erroring on incompatible options probably makes more sense.

The error wording looks sensible.

I'm surprised you had to change so many places. No include files or snippets? Seems like plenty of room to miss one and leave things inconsistent. But that's not really the issue at hand.

Thankyou for this. I hope you're able to get it merged, it'd help reduce the rate at which potentially wormable wide-open postgres instances are created by people who don't realise what they're doing.

[tianon]

Erroring on incompatible options probably makes more sense.

Sure, but in this case they aren't actually incompatible -- they're orthogonal. When one sets POSTGRES_DISABLE_PASSWORDS, the database is configured to allow all connections, regardless of their authentication parameters. Separately, POSTGRES_PASSWORD configures the password which is set on the default user, and that gets set/stored regardless of the authentication method.

In other words, if the user asks us to disable password authentication, we do so (including spitting out a large warning).

Rethinking in this context, I think the name of the variable is misleading and should more directly correspond to what it does instead of what the user intends -- we're not actually disabling "passwords" in PostgreSQL but rather asking PostgreSQL not to prompt for them by setting the authentication method to be trust.

So, perhaps we should instead call it something more explicit like POSTGRES_AUTH_METHOD and warn when the value is set to trust. This would have the added benefit of allowing users to opt into newer authentication methods like scram-sha-256 (https://www.postgresql.org/docs/11/auth-password.html), increasing security further.

I'm surprised you had to change so many places. No include files or snippets? Seems like plenty of room to miss one and leave things inconsistent. But that's not really the issue at hand.

They're all templated -- we just have to commit the templating result so that Travis tests the right thing.

[yosifkit]

Swapped POSTGRES_DISABLE_PASSWORDS to POSTGRES_HOST_AUTH_METHOD so that users configure the value of the auth-method rather than an opt-in to "disable passwords".

I'll try to get a docs PR going soon that will link to https://www.postgresql.org/docs/current/auth-pg-hba-conf.html

[tianon]

LGTM (pending a corresponding docs PR)

[yosifkit]

Docs PR created: docker-library/docs#1653

[BYK]

This is a breaking change, but I think it's one that makes sense overall (and that the benefit outweighs the pain in this instance).

Burnt by this @tianon - now our setup requires POSTGRES_HOST_AUTH_METHOD=trust to be passed. Not sure how this passed the bar for a "patch" release.

[tianon]

We don't have an "image version" -- our published image version is the version of PostgreSQL itself. The fact that this went out with a patch release is entirely coincidental, and it would've gone out as an update to the existing images regardless. See #681 (comment) for further explanation.

The workaround for folks who want to stay insecure is to set POSTGRES_HOST_AUTH_METHOD=trust or if you know you cannot tolerate image updates, pin to an explicit content-addressable digest (which is a good practice to follow regardless).

[BYK]

The fact that this went out with a patch release is entirely coincidental, and it would've gone out as an update to the existing images regardless.

This is even scarier from a versioning perspective and a shortcoming of the official images approach I guess?

The workaround for folks who want to stay insecure is to set POSTGRES_HOST_AUTH_METHOD=trust

If the network is limited properly this is not insecure. Having good and more secure defaults is great but if you had a bad default you don't "fix" it in a minor version bump.

or if you know you cannot tolerate image updates, pin to an explicit content-addressable digest (which is a good practice to follow regardless).

See above. There's a reason why semver exists and people rely on those semantics. We can tolerate changes in images as long as they are patch releases which was what we have set and here we are.