WIP: use secrets for postgres creds #145
Conversation
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.
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
[test] |
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
|
|
||
| function set_password() { | ||
| local user="$1" password="$2" | ||
| psql --command "ALTER USER \"${user}\" WITH ENCRYPTED PASSWORD '${password}';" |
There was a problem hiding this comment.
There is psql --set option which should guard against (also not intentional) sql injection.
| [ -f "$credpath/password" ] && \ | ||
| [[ "$(<"$credpath/username")" =~ $psql_identifier_regex ]] && \ | ||
| [[ "$(<"$credpath/password")" =~ $psql_password_regex ]] && | ||
| [ "$(wc -c < "$credpath/username")" -le 63 ] |
There was a problem hiding this comment.
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.
|
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? |
|
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.. |
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). |
|
@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? |
|
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 ( |
|
That makes sense to me. Can we work with subdirectories like |
|
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. |
|
Subdirectories username: my_user
password: my_passis mapped to the filesystem when mounted as two files in /path/to/secret, username and password. |
|
I do understand your argument re code complexity btw. I am totally fine with weeding out the replicated logic re |
|
Can one of the admins verify this patch? |
|
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. |
|
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! |
|
Can one of the admins verify this patch? |
5 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
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
postgrescan 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.