Skip to content

Change default shell for user postgres to sh #669

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

Merged
merged 2 commits into from
Feb 14, 2020
Merged

Change default shell for user postgres to sh #669

merged 2 commits into from
Feb 14, 2020

Conversation

yaroze
Copy link
Contributor

@yaroze yaroze commented Feb 4, 2020

The adduser command creates the postgres user without specifying a default shell.
alpine-3.11 creates the user with /bin/nologin by default, which differs from the way the user was created when alpine-3.10 was used.
Also, the creation of the postgres user in aports (https://git.alpinelinux.org/aports/tree/main/postgresql/postgresql.pre-install?h=3.11-stable) suggests the usage of /bin/sh.

@tianon
Copy link
Member

tianon commented Feb 7, 2020

In the future, please re-push to your existing PR's branch instead of opening a new one -- GitHub is smart enough to do the right thing and update the relevant pull request (instead of filling our inboxes with multiple threads of PRs being closed and opened anew).

As to the change, I'm not necessarily opposed to it, but I would love to understand more about what's breaking due to it not being set?

Also, this template change will need to be propagated to all variants, but I think we should figure out the "why" behind the change better before we make more changes. 👍

@yaroze
Copy link
Contributor Author

yaroze commented Feb 10, 2020

Sorry for spamming your inbox -- newbie mistakes! 😅

About the suggested change, nothing but "su - postgres" got broken, as far as we noticed.

We just noticed the upgrade to alpine-3.11 after we pulled the postgres image again from the public registry. We had some monitoring scripts that were relying on su to work.

So the suggestion of putting back a default shell is merely to keep things working the way they were before.

It could have been worse - the user id could have been changed 😅

@tianon
Copy link
Member

tianon commented Feb 12, 2020

Ok, that makes sense, although I would recommend switching from su to su-exec since it won't suffer from the same issue (and is what our entrypoint script uses for that same purpose).

@yosifkit
Copy link
Member

Rebased and applied update.sh to get the shell change to all versions.

@yosifkit
Copy link
Member

Travis tests pass for the changed Alpine versions (all Debian versions fail because the version is no longer available in the apt repo; the bump has already been pushed to master).

@yosifkit yosifkit merged commit 6d1f671 into docker-library:master Feb 14, 2020
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Feb 15, 2020
Changes:

- docker-library/postgres@05ac2d3: Merge pull request docker-library/postgres#680 from infosiftr/eol-9.4
- docker-library/postgres@6bb7ce5: Remove EOL 9.4
- docker-library/postgres@6d1f671: Merge pull request docker-library/postgres#669 from yaroze/master
docker-library-bot added a commit to docker-library-bot/official-images that referenced this pull request Feb 18, 2020
Changes:

- docker-library/postgres@4dc16e4: Merge pull request docker-library/postgres#686 from infosiftr/xz-files
- docker-library/postgres@4f70bf2: Add .sql.xz support to docker-entrypoint-initdb.d
- docker-library/postgres@ba0e45b: Merge pull request docker-library/postgres#685 from infosiftr/clarify-unset
- docker-library/postgres@f1bc878: Clarify that "POSTGRES_PASSWORD" should be non-empty
- docker-library/postgres@05ac2d3: Merge pull request docker-library/postgres#680 from infosiftr/eol-9.4
- docker-library/postgres@6bb7ce5: Remove EOL 9.4
- docker-library/postgres@6d1f671: Merge pull request docker-library/postgres#669 from yaroze/master
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.

3 participants