feat: get configuration parameters from the db and allow reloading config with NOTIFY

[steve-chavez]

Feature

For #1562. Allows configuring postgrest from the db by setting config options on the connection role. For example:

-- postgrest.conf having:
-- db-settings-prefix = "instance"

ALTER ROLE postgrest_test_authenticator SET pgrst.instance.jwt-secret = "REALLYREALLYREALLYREALLYVERYSAFE"

That will set the jwt-secret config option accordingly.

SUPERUSER privileges are required for ALTERing role settings(see #1562 (comment)), so this might not work on some cloud-managed databases.

Config options can only be set on ALTER ROLE .. SET .. and not ALTER DATABASE .. SET. Though the same privileges are required(SUPERUSER), ROLE level settings are better because they're scoped to our authenticator role.

[steve-chavez]

One thing that is missing is a test for reloading the config with the db settings. But it doesn't make much sense to reload with a SIGUSR2 signal. I think we should extend our NOTIFY channel to allow reloading the config from there.

So, how about:

-- These two for reloading the schema cache 
NOTIFY pgrst;
NOTIFY pgrst, 'SIGUSR1';

-- This one for reloading the config
NOTIFY pgrst, 'SIGUSR2';

Thoughts?

[wolfgangwalther]

One thing that is missing is a test for reloading the config with the db settings. But it doesn't make much sense to reload with a SIGUSR2 signal. I think we should extend our NOTIFY channel to allow reloading the config from there.

So, how about:

-- These two for reloading the schema cache 
NOTIFY pgrst;
NOTIFY pgrst, 'SIGUSR1';

-- This one for reloading the config
NOTIFY pgrst, 'SIGUSR2';

Thoughts?

I think that was suggested / discussed somewhere else already, right?

I am not sure what is currently implemented already and whether that would be any breaking change, but how about:

• NOTIFY pgrst - reloads both schema cache and config
• NOTIFY pgrst, 'reload schema'
• NOTIFY pgrst, 'reload config'

While we can use the signals for that too, I think it'd be easier to have some more explicit strings. We're not really sending those signals. And a string like this is easier to extend in the future.

[wolfgangwalther]

Feature

For #1562. Allows configuring postgrest from the db by setting config options on the connection role. For example:

-- postgrest.conf having:
-- db-settings-prefix = "instance"

ALTER ROLE postgrest_test_authenticator SET pgrst.instance.jwt-secret = "REALLYREALLYREALLYREALLYVERYSAFE"

That will set the jwt-secret config option accordingly.

Awesome! :)

Config options can only be set on ALTER ROLE .. SET .. and not ALTER DATABASE .. SET. Though the same privileges are required(SUPERUSER), ROLE level settings are better because they're scoped to our authenticator role.

I think the SQL query could easily be extended to support ALTER DATABASE too, right? Is this a deliberate decision to not support that or more like "we can do that later"?
If we could support both and role settings would override database settings already at the query level, that would be cool.

[wolfgangwalther]

A couple of config options to think about:

• db-anon-role. Is currently required on the first load already, so it's not possible to set this only through the database. We should make it optional and block anonymous access without it as discussed elsewhere. This would allow startup without it.
• db-schemas. Can we make this optional, too, to allow setting it on the database level? I suggest we just respond with an error to every request when we don't have a valid db-schemas setting.
• server-* + db-uri + db-pool* - can we add a deny-list for those settings, so that they are not loaded from the database? Otherwise a --dump-config would really be misleading, because it would show overridden settings, that are not in effect.

[jsommr]

Just tested in a cloud hosted Postgres on Aiven, and as you've already discussed, it doesn't work unfortunately. I hope you find a way at some point to allow non-superusers to use the db as config. I think it would make it easier to do jwt key rotation by simply updating a config in the db.

[steve-chavez]

I think the SQL query could easily be extended to support ALTER DATABASE too, right? Is this a deliberate decision to not support that or more like "we can do that later"?

@wolfgangwalther Deliberate decision, and now it's on point with what LorenzHenk mentioned on #1179 (comment):

uninstalling PostgREST will still leave its trails in the DDL of views, which is harder to track down than e.g. having to delete 1 table & 1 view containing all the metadata

If we keep this isolated to the authenticator role, uninstalling postgrest(not that we want that) would be matter of dropping the roles + schema. I also feel that this would look better on the docs - doing an ALTER DATABASE looks more invasive(though requiring the same privileges unfortunately).

Also, the ROLE approach it's more flexible, we could even remove the db-settings-prefix if we tell the user to create different ROLEs for different postgrest instances on the same db. I've noted it's a bit inconvenient to set a prefix when you only have a single postgrest instance.

So that's why I think we should restrict to ROLEs only.

I am not sure what is currently implemented already and whether that would be any breaking change, but how about:
NOTIFY pgrst - reloads both schema cache and config
NOTIFY pgrst, 'reload schema'
NOTIFY pgrst, 'reload config'
While we can use the signals for that too, I think it'd be easier to have some more explicit strings. We're not really sending those signals. And a string like this is easier to extend in the future.

I like it! The strings look much better. I'll add that.

server-* + db-uri + db-pool* - can we add a deny-list for those settings, so that they are not loaded from the database? Otherwise a --dump-config would really be misleading, because it would show overridden settings, that are not in effect.

Totally agree. I'll deny changing those settings.

db-anon-role. Is currently required on the first load already, so it's not possible to set this only through the database.

I think that one deserves it's own PR, it's a breaking change after all - there would be no panic when it's missing.

db-schemas. Can we make this optional, too, to allow setting it on the database level? I suggest we just respond with an error to every request when we don't have a valid db-schemas setting.

Same as above. It's also a bit weird to enable running postgrest when it's in a "broken state". Would need more discussion.

WDYT?

[steve-chavez]

@nerfpops Thanks for reporting, too bad about Aiven. Later on we could create a postgrest extension to register the GUCs so no SUPERUSER is needed. Or we could just wait for SCHEMA VARIABLES to land on postgres as mentioned by Wolfgang on #1562 (comment).

[wolfgangwalther]

If we keep this isolated to the authenticator role, uninstalling postgrest(not that we want that) would be matter of dropping the roles + schema. I also feel that this would look better on the docs - doing an ALTER DATABASE looks more invasive(though requiring the same privileges unfortunately).

I'm not really convinced by this argument...

Also, the ROLE approach it's more flexible, we could even remove the db-settings-prefix if we tell the user to create different ROLEs for different postgrest instances on the same db. I've noted it's a bit inconvenient to set a prefix when you only have a single postgrest instance.

... but very much but this one! Even the current implementation doesn't need the prefix at all, you're right. Using different ROLEs is all we need. Let's get rid of it right away.

So that's why I think we should restrict to ROLEs only.

I can kind of see use-cases where it would be easier to have the database option, too. But since that's easy to extend, I will come back to this, once I should really hit one of those cases. So see you in a couple of years... - ah wait, we'll have schema variables by then... :o)

db-anon-role. Is currently required on the first load already, so it's not possible to set this only through the database.

I think that one deserves it's own PR, it's a breaking change after all - there would be no panic when it's missing.

db-schemas. Can we make this optional, too, to allow setting it on the database level? I suggest we just respond with an error to every request when we don't have a valid db-schemas setting.

Same as above. [...]

WDYT?

Yes, absolutely fine to put those into different PRs. Just wanted to bring it up, because those are related here, too.

It's also a bit weird to enable running postgrest when it's in a "broken state". Would need more discussion.

I think the "broken, but defined state" is better then any undefined state. Assume you're just setting up PostgreSQL and PostgREST. Most setups would probably restart PostgREST once it errors out. Assume you're initializing the database and are not creating all the roles and objects etc. in one transaction. PostgREST would kind of magically start serving the schema once the config variable is set - even though the schema might have any number of objects so far (empty, some created, all created - randomly). It'd be much better if, after setting everything up, you just NOTIFY PostgREST to reload. Before reload we will serve an error message. After reload we will serve the full schema. But nothing in parts only (undefined). A lot less surprising. Will also teach people a bit earlier about the need and how to reload the schema.

[wolfgangwalther]

Also, the ROLE approach it's more flexible, we could even remove the db-settings-prefix if we tell the user to create different ROLEs for different postgrest instances on the same db. I've noted it's a bit inconvenient to set a prefix when you only have a single postgrest instance.

... but very much but this one! Even the current implementation doesn't need the prefix at all, you're right. Using different ROLEs is all we need. Let's get rid of it right away.

Do we still have a config option for this? Or do we just load config options from database all the time? Not sure if there's any downside to doing so - I don't see any right now.

[steve-chavez]

Do we still have a config option for this? Or do we just load config options from database all the time? Not sure if there's any downside to doing so - I don't see any right now.

I think not! I would also like removing the prefix config. I'll check it out.

[steve-chavez]

Actually, one downside to load the db settings always is that it's wasteful for users that don't have superuser over their db(most of our users maybe). The query is simple, but still.

Another one is that it slows down the io-tests, I'm getting a test_io.PostgrestTimedOut on all the tests now.

So, how about having this config:

db-settings = true

@wolfgangwalther WDYT?

[wolfgangwalther]

Hm. I'm not too concerned about performance, because it's only on reload, not while serving requests. In the context of reloading the overhead should be rather small. IO test performance is a thing however.

So if we can make the default db-settings = true, then we could still change it to false for the io tests. Users that care for reload performance can do that, too.

Enabling by default seems appealing to me, because the whole point of putting the config in the database is, that I want to have the least amount of config needed in my config file / docker-compose file.

So instead of having this:

  environment:
    PGRST_DB_URI: ...
    PGRST_DB_SETTINGS: "true"

it would be

  environment:
    PGRST_DB_URI: ...

It's not much, but it "just works".

Does that make sense?

---

Thinking about the name of the config option. Currently we are only using the term "settings" in the context of app.settings.... In the docs we call everything else "configuration" and "configuration parameters". Two of the three boolean config options we currently have, do have some "boolean" indicator, too: "jwt-secret-is-base64" and "db-channel-enabled".

Given that, I suggest we call it db-load-guc-config. We could also rename db-prepared-statements to db-use-prepared-statements and db-channel-enabled to db-enable-channel for consistency. I think both of those are currently unreleased, so that would not be a breaking change, yet.

WDYT?

[steve-chavez]

Given that, I suggest we call it db-load-guc-config.

I like that 👍, I've renamed the config and made it True by default.

[wolfgangwalther]

Looking good. Suggested just a few more minor changes.

One thing that is missing is a test for reloading the config with the db settings. But it doesn't make much sense to reload with a SIGUSR2 signal. I think we should extend our NOTIFY channel to allow reloading the config from there.

Are you going to do that in this PR? If yes, cool. If no, I suggest to still add a test for now that tests the reloading with a SIGUSR2 signal. This can then easily be switched to a NOTIFY, but we should have test coverage good immediately. This is also related to my still open comment above about the condition for reload, that needs to be changed to make test-case like this without config file pass.

[wolfgangwalther]

One other note: I wonder why codecov doesn't add any status checks here. The upload part says this:

View reports at https://codecov.io/github/steve-chavez/postgrest/commit/684abc0461948f8d60541346dc4229ab36ba70d3

@steve-chavez have you enabled codecov for you fork, too? Maybe that's causing this.

It could also be circleci, that's enabled for your fork. I noticed that when you currently open a PR for the first time CircleCI has already run the pipeline in your fork and will not report anything in the PR until you trigger a rebuild somehow. I had the same thing while I had Circle enabled in my own fork. Once I disabled it, it behaved as expected again.

[steve-chavez]

I noticed that when you currently open a PR for the first time CircleCI has already run the pipeline in your fork and will not report anything in the PR until you trigger a rebuild somehow.

Triggered a rebuild but it didn't seem to work.

It could also be circleci, that's enabled for your fork.
Once I disabled it, it behaved as expected again.

Yes, I do have it enabled in my fork. It's helpful for running tests before making a PR though, so I would prefer not to disable it.

have you enabled codecov for you fork, too? Maybe that's causing this.

That one I could disable. I'll try it out.

[wolfgangwalther]

Yes, I do have it enabled in my fork. It's helpful for running tests before making a PR though, so I would prefer not to disable it.

Yeah. I digged further and it seems like disabling it for the fork is in fact the only way to "solve" this. Circle not showing up when I first open the PR was annoying enough for me to do that. I'm totally fine with you keeping it enabled, though. It will eventually show up once anything changes in the PR or if we manually trigger a rebuild.

have you enabled codecov for you fork, too? Maybe that's causing this.

That one I could disable. I'll try it out.

I assume it will not work. This is because we are using the tokenless-upload. Therefore the codecov upload script has to take the repo info from what it gets from CircleCI - and this is running with your fork's repo name. In this case you could try to add the postgrest repo's codecov token to your fork in CircleCI. This way, the upload script should pick up this token when running under your CircleCI user and still upload to the main repo.

[wolfgangwalther]

I guess that's just what I assumed:

==> Uploading reports
    url: https://codecov.io
    query: branch=in-db-config&commit=684abc0461948f8d60541346dc4229ab36ba70d3&build=3877&build_url=&name=&tag=&slug=steve-chavez%2Fpostgrest&service=circleci&flags=&pr=1729&job=0&cmd_args=f

That slug=steve-chavez/postgrest is probably taken from one of the circle ci env vars.

[wolfgangwalther]

Compare this to the same from #1724. I don't have circle enabled for my fork and it shows this:

==> Uploading reports
    url: https://codecov.io
    query: branch=pull%2F1724&commit=4f3aea70a9bb9efdde8ff09af75a72ff93c25329&build=7966&build_url=&name=&tag=&slug=PostgREST%2Fpostgrest&service=circleci&flags=&pr=1724&job=0&cmd_args=f

I hope setting up the codecov token in your circleci user will work.

[steve-chavez]

I hope setting up the codecov token in your circleci user will work.

Let's see. I've just copied the CODECOV_TOKEN from https://app.codecov.io/gh/PostgREST/postgrest/settings to my postgrest project env vars on CircleCI.

Edit: Worked!

[wolfgangwalther]

Edit: Worked!

Sweet!

codecov/patch is currently not hitting the target. This seems to be mainly about parts of the refactor and --dump-schema. I'm pretty certain that most of those lines will be covered once you rebase on current main, where I merged the new io tests. The two remaining uncovered lines will be two of the few cases where we need the coverage overlay later on - we can't test the error branches in the settings and dump-schema queries.

[steve-chavez]

can we still call the next release the performance release? Or should we call it the configuration release?

@wolfgangwalther Ah, and there's also more security because of the switch from "set locals" to prepared statements. Perhaps we can call it something like enterprise-grade release? Not sure for now :)

[steve-chavez]

Btw, code coverage is amazing(first time user here). I see we have a badge already on the repo. But maybe we could also list it as a feature on the release since it means more quality and trust in the postgrest codebase.

[ruslantalpa]

While the settings are scoped per role, they are readable by everybody that can connect to the database.
This is a bad idea for storing jwt-secret specifically (unless you really tightly control who can connect to the database and run queries).
It is convenient for rotating the secret but it's very insecure.

[ruslantalpa]

while naming the settings like pgrst."openapi-server-proxy-uri", the alter statement works, and they are stored in the pg config tables, however, all the settings named like this can not be read using show setting or current_setting, and it should work under normal conditions (i bet that is why you used that semi complicated statement to get them back).

The problem here is the - character, it it were named like pgrst.openapi_server_proxy_uri (or maybe PGRST.OPENAPI_SERVER_PROXY_URI like the env vars) then show and current_setting functions work properly and the dbSettingsStatement statement can be a bit more clear/explicit as to what it's fetching from the database

[ruslantalpa]

discussion here https://www.postgresql.org/message-id/flat/20210209144059.GA21360%40depesz.com

[wolfgangwalther]

It might not work with SHOW, but it does work with current_setting.

set app."test-a-b-c" = 'hello world';
select current_setting('app.test-a-b-c');

returns "hello world" for me. No problem there.

[wolfgangwalther]

PG 13.1 here.

[wolfgangwalther]

Hm. I'm probably mis-understanding something. The same thing works for me with SHOW.

[wolfgangwalther]

Hm. Happens only when using ALTER ROLE ... SET - then loading the guc on login is broken once there is a - in it. I'd consider this a bug in Postgres.

[steve-chavez]

Ah, can reproduce as well on pg 11.5 - not even current_setting works when the guc has a dash. I guess this should be fine for our config since we get it through pg_db_role_setting. But not for using the guc configs through functions, which should be the case for jwt-secret.

Not sure if we should change the dashes to underscores, looks like this is already been considered as a bug on the pg mailing list.

[wolfgangwalther]

Looking at the latest post on the mailing list, the bug seems to be that those variables are actually available with _ instead of - already.

So for all PG versions that have the bug, SELECT current_setting('pgrst.openapi_server_proxy_uri') should work even without changes from our side.

[steve-chavez]

@wolfgangwalther Great! I've also confirmed this works.

Though I guess it'd be good to keep the config option names consistent. I'll change the dashes to underscores on another PR.

[wolfgangwalther]

This will be our only way forward, too, because the consensus on the PG mailing list now is to don't allow - in GUC names anymore.

[ruslantalpa]

While it might be a bug, it will be fixed only for 13+ which means postgrest will not work properly for any other version. This is not released yet so its the perfect time to switch to _ just like the env vars

…

On 9 Feb 2021, at 18:39, Steve Chavez ***@***.***> wrote:  @steve-chavez commented on this pull request. In test/fixtures/roles.sql: > @@ -5,3 +5,32 @@ CREATE ROLE postgrest_test_default_role; CREATE ROLE postgrest_test_author; GRANT postgrest_test_anonymous, postgrest_test_default_role, postgrest_test_author TO :USER; + +-- reloadable config options for io tests +ALTER ROLE postgrest_test_authenticator SET pgrst."jwt-aud" = 'https://example.org'; +ALTER ROLE postgrest_test_authenticator SET pgrst."openapi-server-proxy-uri" = 'https://example.org/api'; Ah, can reproduce as well on pg 11.5 - not even current_setting works when the guc has a dash. I guess this should be fine for our config since we get it through pg_db_role_setting. But not for using the guc configs through functions, which should be the case for jwt-secret. Not sure if we should change the dashes to underscores, looks like this is already been considered as a bug on the pg mailing list. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[steve-chavez]

This is a bad idea for storing jwt-secret specifically (unless you really tightly control who can connect to the database and run queries).

True. That's something the app.settings.jwt-secret solution avoided since it was transaction scoped. I don't see a way to restrict access unless schema variables(as mentioned on #1562 (comment)) land on pg.

It's not a risk if you don't allow a direct SQL connection for external users though. Which I think it's the case for most PostgREST users.

[ruslantalpa]

the jwt-secret thing..., the convenience is too good to pass on. A good middle ground is, when the docs are written for this feature, make a big note in bold/red next to jwt warning people that it's on them to balance security vs convenience. In a world where everybody connects with a superuser to the database, this is not a big deal (and maybe with some work, access to pg_catalog can be restricted).

[steve-chavez]

(and maybe with some work, access to pg_catalog can be restricted).

I think this would be enough for protecting the setttings:

-- don't allow reading the settings for every role
REVOKE SELECT ON pg_catalog.pg_db_role_setting FROM public;

-- explicitly allow the authenticator to read the settings
GRANT SELECT ON pg_catalog.pg_db_role_setting TO authenticator;

Then every non-SUPERUSER role that connects to the db cannot read from pg_db_role_setting. Access would have to be granted explicitly.

The above would have to be done for every postgrest-enabled database.

[wolfgangwalther]

If we keep this isolated to the authenticator role, uninstalling postgrest(not that we want that) would be matter of dropping the roles + schema. I also feel that this would look better on the docs - doing an ALTER DATABASE looks more invasive(though requiring the same privileges unfortunately).

Also, the ROLE approach it's more flexible, we could even remove the db-settings-prefix if we tell the user to create different ROLEs for different postgrest instances on the same db. I've noted it's a bit inconvenient to set a prefix when you only have a single postgrest instance.

So that's why I think we should restrict to ROLEs only.

So ALTER ROLE is better than ALTER DATABASE - agreed. I just noticed, however, that it only works with ALTER ROLE ... IN DATABASE SET ..., but does not when using ALTER ROLE ... SET.

In my current project, I don't know the name of the database in advance (could be development, staging, ...) and there is nothing like ALTER ROLE ... IN CURRENT DATABASE ..., yet.

We could probably read both the settings for the current database and the settings for all databases at the same time from pg_db_role_setting, right? We'd just need to make sure to merge properly in case of conflicts.

WDYT?

[wolfgangwalther]

(and maybe with some work, access to pg_catalog can be restricted).

I think this would be enough for protecting the setttings:

-- don't allow reading the settings for every role
REVOKE SELECT ON pg_catalog.pg_db_role_setting FROM public;

-- explicitly allow the authenticator to read the settings
GRANT SELECT ON pg_catalog.pg_db_role_setting TO authenticator;

Then every non-SUPERUSER role that connects to the db cannot read from pg_db_role_setting. Access would have to be granted explicitly.

The above would have to be done for every postgrest-enabled database.

Sounds cool. I haven't tested it - does ALTER ROLE ... SET still work for a regular user on login with that? Not sure if permissions are checked at login time when SET-ing all the stuff.

[wolfgangwalther]

So ALTER ROLE is better than ALTER DATABASE - agreed. I just noticed, however, that it only works with ALTER ROLE ... IN DATABASE SET ..., but does not when using ALTER ROLE ... SET.

In my current project, I don't know the name of the database in advance (could be development, staging, ...) and there is nothing like ALTER ROLE ... IN CURRENT DATABASE ..., yet.

We could probably read both the settings for the current database and the settings for all databases at the same time from pg_db_role_setting, right? We'd just need to make sure to merge properly in case of conflicts.

WDYT?

Looking at the test-cases in here a bit more closely, I guess my problem is somewhere else. Still.. looking at the code I wonder what happens when somebody sets both ALTER ROLE ... SET and ALTER ROLE ... IN DATABASE ... SET? I don't think this will be handled properly.

And the query seems to assume that all settings in pg_db_role_setting apply to the current database, too. But that's not the case, this table is cluster-wide - so we'd need at least a filter for setdatabase to filter for the current database and NULLs only. Right now, I think it's possible to add a setting in a completely unrelated database and have it affect the postgrest instance running in this database.

[steve-chavez]

I just noticed, however, that it only works with ALTER ROLE ... IN DATABASE SET ..., but does not when using ALTER ROLE ... SET.

Oh, that shouldn't be the case. According to tests:

postgrest/test/fixtures/roles.sql

Lines 10 to 15 in 0ddd676

	ALTER ROLE postgrest_test_authenticator SET pgrst."jwt-aud" = 'https://example.org';
	ALTER ROLE postgrest_test_authenticator SET pgrst."openapi-server-proxy-uri" = 'https://example.org/api';
	ALTER ROLE postgrest_test_authenticator SET pgrst."raw-media-types" = 'application/vnd.pgrst.db-config';
	ALTER ROLE postgrest_test_authenticator SET pgrst."jwt-secret" = 'REALLYREALLYREALLYREALLYVERYSAFE';
	ALTER ROLE postgrest_test_authenticator SET pgrst."jwt-secret-is-base64" = 'true';
	ALTER ROLE postgrest_test_authenticator SET pgrst."jwt-role-claim-key" = '."a"."role"';

In my current project, I don't know the name of the database in advance (could be development, staging, ...) and there is nothing like ALTER ROLE ... IN CURRENT DATABASE ..., yet.
what happens when somebody sets both ALTER ROLE ... SET and ALTER ROLE ... IN DATABASE ... SET? I don't think this will be handled properly.
And the query seems to assume that all settings in pg_db_role_setting apply to the current database

I definitely intended for the ROLE guc settings to be global - all databases. I didn't know this was only being applied for the current db, I'll check it out.

does ALTER ROLE ... SET still work for a regular user on login with that? Not sure if permissions are checked at login time when SET-ing all the stuff.

If the authenticator role doesn't have GRANT SELECT ON pg_catalog.pg_db_role_setting, this is what happens:

An error ocurred when trying to query database settings for the config parameters:
SessionError (QueryError "\n      with\n      role_setting as (\n        select unnest(setconfig) as setting from pg_catalog.pg_db_role_setting where setrole = current_user::regrole::oid\n      ),\n      kv_settings as (\n        select split_part(setting, '=', 1) as key, split_part(setting, '=', 2) as value from role_setting\n      )\n      select replace(key, 'pgrst.', '') as key, value from kv_settings where key like 'pgrst.%';\n    " [] (ResultError (ServerError "42501" "permission denied for table pg_db_role_setting" Nothing Nothing)))
Attempting to connect to the database...
Listening on port 3000
Connection successful
Schema cache loaded

So postgrest still starts as usual, but it cannot get the guc settings from the db, and it sticks to the config file settings.

If the authenticator is GRANTed SELECT, then the error is cleared and the guc settings are loaded as usual.

[wolfgangwalther]

I definitely intended for the ROLE guc settings to be global - all databases. I didn't know this was only being applied for the current db, I'll check it out.

Not sure whether there is a mis-understanding. The problem is this:

• postgrest runs on database A
• ALTER ROLE authenticator IN DATABASE B SET pgrst.xxx TO zzz;

When postgrest (connected to database A) reads pg_db_role_setting it will get a row for pgrst.xxx back - even though that is set in database B only. The table pg_db_role_setting is cluster-wide instead of database-wide, so we need to account for that by selecting for the current db only! And for global settings (db column = NULL), of course.

If the authenticator role doesn't have GRANT SELECT ON pg_catalog.pg_db_role_setting, this is what happens:
[...]
If the authenticator is GRANTed SELECT, then the error is cleared and the guc settings are loaded as usual.

Definitely a mis-understanding.

I wanted to know if everything still works correctly after REVOKE SELECT:

create role alice login;
alter role alice set a.b to 'c';
revoke select on pg_catalog.pg_db_role_setting from public;

Is it still possible to login as user alice without error? I just tested it and it still works. SHOW a.b; works, too - the value is correctly set on login. So no problem here.

[wolfgangwalther]

And another problem with loading the config:

I have a docker setup with one container for the database and one container for postgrest. When I launch both containers at the same time (docker-compose up), the database container will seed itself and take a bit of time to start up. While that happens PostgREST tries to connect already, but fails at first. It then reconnects sucessfully - but at that time the config is not reloaded from the database. This means that the database config is never loaded.

This is the postgrest output:

postgrest_1  | An error ocurred when trying to query database settings for the config parameters:
postgrest_1  | ConnectionError (Just "could not connect to server: Connection refused\n\tIs the server running on host \"database\" (172.19.0.2) and accepting\n\tTCP/IP connections on port 5432?\n")
postgrest_1  | Attempting to connect to the database...
postgrest_1  | Listening on port 3000
postgrest_1  | {"details":"could not connect to server: Connection refused\n\tIs the server running on host \"database\" (172.19.0.2) and accepting\n\tTCP/IP connections on port 5432?\n","code":"","message":"Database connection error. Retrying the connection."}
postgrest_1  | Attempting to reconnect to the database in 0 seconds...
postgrest_1  | Connection successful
postgrest_1  | Schema cache loaded

What's basically missing is a Config reloaded before the Schema cache loaded, right?

Should we reload the config whenever the connection is re-tried? And possibly the schema cache, too? (If not already the case)

[wolfgangwalther]

Summarizing a TODO list from all the points above, so we're not missing anything:

• Document security concerns with pgrst.jwt-secret and how to protect the setting table
• Change pgrst.xxx-yyy variables to use _ instead of -. Or maybe ., would be more consistent: pgrst.jwt.secret = 'xyz'
• Fix config reloading after reconnect
• Ignore settings from other databases (ALTER ROLE ... IN DATABASE B SET ... should not affect postgrest in database A)
• Merge database specific and global settings consistently (ALTER ROLE ... IN DATABASE ... SET should override ALTER ROLE ... SET)

[steve-chavez]

Working this on #1760.

Change pgrst.xxx-yyy variables to use _ instead of -. Or maybe ., would be more consistent: pgrst.jwt.secret = 'xyz'

I went with underscore - pgrst.jwt_secret. They look similar to env var settings and I thought names like pgrst.server.unix.socket.mode looked weird, only server is a namespace there.