WIP: use secrets for postgres creds #145

ibotty · 2016-09-26T20:41:41Z

See issue #101.

This is only in the 9.5 directory for now.

What do you think about it? I don't really like the logic re "simple_db", etc, but I left it as is. When using secrets, it might make sense to not use magic account names (e.g. the admin user postgres can be in any secret, no need for "user" credentials, one valid set of credentials is enough, etc.). If you have a preference on how that should be handled, I'll gladly implement it.

Secrets are supposed to be mounted in subdirectories of /run/secrets/pgusers/. They should have `username` and `password` keys. Limitations: It does not check for duplicate user names or overlap with (legacy) environment variables setting credentials. Regarding startup works similar to the previous environment variable based logic. It will only start if one or both of the following is true. * `/run/secrets/pguser/user` are valid credentials and POSTGRESQL_DATABASE is set (a database will be created). * `/run/secrets/pguser/admin` are valid credentials and the username is `postgres`. `/run/secrets/pguser/master` is supposed to be the replication master user, if mounted.

openshift-bot · 2016-09-26T20:42:00Z

Can one of the admins verify this patch?

centos-ci · 2016-09-26T20:42:02Z

Can one of the admins verify this patch?

rhscl-bot · 2016-09-26T20:45:03Z

Can one of the admins verify this patch?

praiskup · 2016-09-29T13:19:52Z

[test]

praiskup · 2016-09-29T13:29:40Z

9.5/root/usr/share/container-scripts/postgresql/common.sh

@@ -65,13 +74,31 @@ function check_env_vars() {
    [ ${#POSTGRESQL_USER}     -le 63 ] || usage "PostgreSQL username too long (maximum 63 characters)"
    [ ${#POSTGRESQL_DATABASE} -le 63 ] || usage "Database name too long (maximum 63 characters)"
    postinitdb_actions+=",simple_db"
+  elif check_cred_secret "/run/secrets/pgusers/user" && [ -v POSTGRESQL_DATABASE ]; then


This looks like we limit the container for one user, forever? Sounds like we start to define API and that should be defined with respects to future.. I tried to "veto" before, but nobody listened to me (so now we can create just one db with environment variables). And this looks like similarly limited approach.

I do agree. I feel it's kind of limited. But if having one container manage exactly one database (a good idea imo) I can understand doing an automatic createdb. Being able to set the database's superuser also feels right. I don't like the coupling though.

As indicated above, I am fine with implementing any logic that makes sense to you, as long as it's possible to use it for many database users!

praiskup · 2016-09-29T13:32:00Z

9.5/root/usr/share/container-scripts/postgresql/common.sh

+
+function set_password() {
+    local user="$1" password="$2"
+    psql --command "ALTER USER \"${user}\" WITH ENCRYPTED PASSWORD '${password}';"


There is psql --set option which should guard against (also not intentional) sql injection.

praiskup · 2016-09-29T13:35:42Z

9.5/root/usr/share/container-scripts/postgresql/common.sh

+    [ -f "$credpath/password" ] && \
+    [[ "$(<"$credpath/username")" =~ $psql_identifier_regex ]] && \
+    [[ "$(<"$credpath/password")" =~ $psql_password_regex ]] &&
+    [ "$(wc -c < "$credpath/username")" -le 63 ]


I don't think that we need to bother doing the regexps, but I understand that this is sort of guideline. Reason: That's just re-implementing of the checks which are done by PostgreSQL itself. I bet this check exists in OpenShift gui, too.

praiskup · 2016-09-29T13:46:03Z

As long as some async inotify machinery isn't involved, I am not strictly against this. But it is basically useless, because environment variables can be used securely, too (see issue #101). So, to me, this doesn't help security and adds additional complexity for maintainers, reviewers, qa and everybody listening to this code-stream.

The thing is that this is still only going to be used by OpenShift, and we basically add yet another API for container <-> environment communication. Do we plan to remove the support for env. variables?

praiskup · 2016-09-29T14:00:39Z

Also, there is already prefered API --- kube can directly contact the server (e.g. via unix socket or IP), do such things in transaction way, and react on PostgreSQL errors properly. Except for protecting against technical debt, I'm mostly interested saving everybody's resources..

ibotty · 2016-09-29T14:45:42Z

But it is basically useless, because environment variables can be used securely, too (see issue #101).

For me the most important part of this patch is, that it makes it possible to keep in sync many database users/credentials. That's not nicely done with env vars (and pretty verbose too).

praiskup · 2016-09-29T14:50:58Z

@ibotty I still don't follow, what is meant by many database users/credentials? Do you mean many users within single container, or syncing one user/credentials among many containers?

ibotty · 2016-09-29T14:55:28Z

Sorry, we are talking past each other. So: an example (I am running a variation of that patch in production):

I have many database users connecting to a single database (running in one container). To do that, I mount many secrets (user1-pgcreds, ..., userN-pgcreds) in the container (mountpath: /run/secrets/pgusers/user1, ..., /run/secrets/pgusers/userN). On container start, all database users get set the right password.

praiskup · 2016-09-29T15:07:05Z

That makes sense to me. Can we work with subdirectories like .../userA/password, etc.?

praiskup · 2016-09-29T15:14:22Z

I mean, I see the benefit of defining new "API" which is able to handle more complicated init scenarios, I'm not sure that working with files is better than simply allowing you to bind-mount arbitrary script and sourcing/executing it.

ibotty · 2016-09-29T15:15:30Z

Subdirectories .../userA/password? If you mean, that .../userA is a directory, and password is a file, that's how it works.
A secret of the form

username: my_user
password: my_pass

is mapped to the filesystem when mounted as two files in /path/to/secret, username and password.

ibotty · 2016-09-29T15:20:02Z

I do understand your argument re code complexity btw. I am totally fine with weeding out the replicated logic re simple_db, and only doing the traverse users directory part.

rhscl-bot · 2016-10-10T08:07:01Z

Can one of the admins verify this patch?

bparees · 2016-10-10T17:09:59Z

it definitely seems simpler to have the image support an init script (Which i think we already have a PR for) and allow that init script to be provided via a mounted secret. The init script can then create multiple users, or do other useful things.

This mounting of additional files in this way feels fragile.

ibotty · 2016-10-10T17:42:22Z

I disagree. Without the possibility to set multiple users it's not easily possible to, say, have a template generate an application, consisting of a database pod, a management pod and a frontend pod (with restricted db permissions). The implementation of that feature is really easy. Most parts are only moving around functionality (which makes sense regardless of mounting creds with secrets). If the crude simpledb logic were not included, the core part of that patch is maybe 5 lines!

centos-ci · 2016-11-25T12:20:23Z

Can one of the admins verify this patch?

centos-ci · 2017-02-07T12:45:31Z

Can one of the admins verify this patch?

rhscl-bot · 2017-11-28T15:46:09Z

Can one of the admins verify this patch?

rhscl-bot · 2018-09-14T02:27:20Z

Can one of the admins verify this patch?

rhscl-bot · 2019-07-19T08:59:32Z

Can one of the admins verify this patch?

rhscl-automation · 2020-09-03T08:46:55Z

Can one of the admins verify this patch?

phracek · 2024-11-07T08:48:50Z

As this issue is not touched for couple years, closing it. We do not have capacity to to fix it.

If it is still valid, feel free to re-open this pull request again.

bparees assigned hhorak Sep 26, 2016

ibotty mentioned this pull request Sep 29, 2016

set passwords for a set of accounts #101

Open

praiskup reviewed Sep 29, 2016

View reviewed changes

use variables to prohibit sql injection

28ec496

phracek marked this pull request as draft November 7, 2024 08:47

phracek closed this Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: use secrets for postgres creds #145

WIP: use secrets for postgres creds #145

ibotty commented Sep 26, 2016 •

edited

Loading

openshift-bot commented Sep 26, 2016

centos-ci commented Sep 26, 2016

rhscl-bot commented Sep 26, 2016

praiskup commented Sep 29, 2016

praiskup Sep 29, 2016

ibotty Sep 29, 2016

praiskup Sep 29, 2016

praiskup Sep 29, 2016

praiskup commented Sep 29, 2016

praiskup commented Sep 29, 2016

ibotty commented Sep 29, 2016 •

edited

Loading

praiskup commented Sep 29, 2016 •

edited

Loading

ibotty commented Sep 29, 2016

praiskup commented Sep 29, 2016

praiskup commented Sep 29, 2016

ibotty commented Sep 29, 2016

ibotty commented Sep 29, 2016

rhscl-bot commented Oct 10, 2016

bparees commented Oct 10, 2016

ibotty commented Oct 10, 2016

centos-ci commented Nov 25, 2016

centos-ci commented Feb 7, 2017

rhscl-bot commented Nov 28, 2017

rhscl-bot commented Sep 14, 2018

rhscl-bot commented Jul 19, 2019

rhscl-automation commented Sep 3, 2020

phracek commented Nov 7, 2024

WIP: use secrets for postgres creds #145

WIP: use secrets for postgres creds #145

Conversation

ibotty commented Sep 26, 2016 • edited Loading

openshift-bot commented Sep 26, 2016

centos-ci commented Sep 26, 2016

rhscl-bot commented Sep 26, 2016

praiskup commented Sep 29, 2016

praiskup Sep 29, 2016

Choose a reason for hiding this comment

ibotty Sep 29, 2016

Choose a reason for hiding this comment

praiskup Sep 29, 2016

Choose a reason for hiding this comment

praiskup Sep 29, 2016

Choose a reason for hiding this comment

praiskup commented Sep 29, 2016

praiskup commented Sep 29, 2016

ibotty commented Sep 29, 2016 • edited Loading

praiskup commented Sep 29, 2016 • edited Loading

ibotty commented Sep 29, 2016

praiskup commented Sep 29, 2016

praiskup commented Sep 29, 2016

ibotty commented Sep 29, 2016

ibotty commented Sep 29, 2016

rhscl-bot commented Oct 10, 2016

bparees commented Oct 10, 2016

ibotty commented Oct 10, 2016

centos-ci commented Nov 25, 2016

centos-ci commented Feb 7, 2017

rhscl-bot commented Nov 28, 2017

rhscl-bot commented Sep 14, 2018

rhscl-bot commented Jul 19, 2019

rhscl-automation commented Sep 3, 2020

phracek commented Nov 7, 2024

ibotty commented Sep 26, 2016 •

edited

Loading

ibotty commented Sep 29, 2016 •

edited

Loading

praiskup commented Sep 29, 2016 •

edited

Loading