Reload external file on SIGUSR2/NOTIFY

steve-chavez · steve-chavez · commit a36f949760b5 · 2021-02-26T21:19:58.000-05:00
diff --git a/main/Main.hs b/main/Main.hs
@@ -70,7 +70,7 @@ main = do
   pathEnvConf <- either panic identity <$> readAppConfig mempty env cliPath Nothing Nothing
 
   -- read external files
-  dbUriFile <- readDbUriFile $ configDbUri pathEnvConf
+  dbUriFile  <- readDbUriFile $ configDbUri pathEnvConf
   secretFile <- readSecretFile $ configJwtSecret pathEnvConf
 
   -- add the external files to AppConfig
@@ -111,10 +111,19 @@ main = do
   -- Config that can change at runtime
   refConf <- newIORef conf
 
-  let dbConfigReader startingUp = readDbConfig startingUp pool gucConfigEnabled env cliPath dbUriFile secretFile refConf
-
-  -- Override the config with config options from the db, only if db-load-guc-config is true
-  when gucConfigEnabled $ dbConfigReader True
+  let
+    -- re-reads config file + db config
+    dbConfigReReader startingUp = when gucConfigEnabled $
+      reReadConfig startingUp pool gucConfigEnabled env cliPath refConf dbUriFile secretFile
+    -- re-reads jwt-secret external file + config file + db config
+    fullConfigReReader =
+      reReadConfig False pool gucConfigEnabled env cliPath refConf
+        dbUriFile =<< -- db-uri external file could be re-read, but it doesn't make sense as db-uri is not reloadable
+        readSecretFile (configJwtSecret pathEnvConf)
+
+  -- Override the config with config options from the db
+  -- TODO: the same operation is repeated on connectionWorker, ideally this would be done only once, but dump CmdDumpConfig needs it for tests.
+  dbConfigReReader True
 
   case cliCommand of
     CmdDumpConfig ->
@@ -133,7 +142,8 @@ main = do
   -- This is passed to the connectionWorker method so it can kill the main thread if the PostgreSQL's version is not supported.
   mainTid <- myThreadId
 
-  let connWorker = connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) $ dbConfigReader False
+  let connWorker = connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) $
+                     dbConfigReReader False
 
   -- Sets the initial refDbStructure
   connWorker
@@ -156,13 +166,13 @@ main = do
 
   -- Re-read the config on SIGUSR2
   void $ installHandler sigUSR2 (
-    Catch $ dbConfigReader False
+    Catch fullConfigReReader
     ) Nothing
 #endif
 
   -- reload schema cache + config on NOTIFY
   when dbChannelEnabled $
-    listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker $ dbConfigReader False
+    listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWorker fullConfigReReader
 
   -- ask for the OS time at most once per second
   getTime <- mkAutoUpdate defaultUpdateSettings {updateAction = getCurrentTime}
@@ -219,7 +229,7 @@ connectionWorker
   -> (Bool, MVar ConnectionStatus) -- ^ For interacting with the LISTEN channel
   -> IO ()
   -> IO ()
-connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) cfRereader = do
+connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEnabled, mvarConnectionStatus) dbCfReader = do
   isWorkerOn <- readIORef refIsWorkerOn
   unless isWorkerOn $ do -- Prevents multiple workers to be running at the same time. Could happen on too many SIGUSR1s.
     atomicWriteIORef refIsWorkerOn True
@@ -235,7 +245,7 @@ connectionWorker mainTid pool refConf refDbStructure refIsWorkerOn (dbChannelEna
         NotConnected                -> return ()                                     -- Unreachable because connectionStatus will keep trying to connect
         Connected actualPgVersion   -> do                                            -- Procede with initialization
           putStrLn ("Connection successful" :: Text)
-          cfRereader
+          dbCfReader -- this could be fail because the connection drops, but the loadSchemaCache will pick the error and retry again
           scStatus <- loadSchemaCache pool actualPgVersion refConf refDbStructure
           case scStatus of
             SCLoaded    -> pure ()            -- do nothing and proceed if the load was successful
@@ -340,9 +350,9 @@ listener dbUri dbChannel pool refConf refDbStructure mvarConnectionStatus connWo
     errorMessage = "Could not listen for notifications on the " <> dbChannel <> " channel" :: Text
     retryMessage = "Retrying listening for notifications on the " <> dbChannel <> " channel.." :: Text
 
--- | Reads the config options from the db
-readDbConfig :: Bool -> P.Pool -> Bool -> Environment -> Maybe FilePath -> Maybe Text -> Maybe BS.ByteString -> IORef AppConfig -> IO ()
-readDbConfig startingUp pool gucConfigEnabled env path dbUriFile secretFile refConf = do
+-- | Re-reads the config plus config options from the db
+reReadConfig :: Bool -> P.Pool -> Bool -> Environment -> Maybe FilePath -> IORef AppConfig -> Maybe Text -> Maybe BS.ByteString -> IO ()
+reReadConfig startingUp pool gucConfigEnabled env path refConf dbUriFile secretFile = do
   dbSettings <- if gucConfigEnabled then loadDbSettings else pure []
   readAppConfig dbSettings env path dbUriFile secretFile >>= \case
     Left err   ->
diff --git a/test/fixtures/roles.sql b/test/fixtures/roles.sql
@@ -7,55 +7,55 @@ 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';
-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"';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_tx_end" = 'commit-allow-override';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_schemas" = 'test, tenant1, tenant2';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_root_spec" = 'root';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_prepared_statements" = 'false';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_pre_request" = 'test.custom_headers';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_max_rows" = '1000';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_extra_search_path" = 'public, extensions';
+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"';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_tx_end = 'commit-allow-override';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_schemas = 'test, tenant1, tenant2';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_root_spec = 'root';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_prepared_statements = 'false';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_pre_request = 'test.custom_headers';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_max_rows = '1000';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_extra_search_path = 'public, extensions';
 
 -- override with database specific setting
-ALTER ROLE postgrest_test_authenticator IN DATABASE :DBNAME SET pgrst."jwt_secret" = 'OVERRIDEREALLYREALLYREALLYREALLYVERYSAFE';
-ALTER ROLE postgrest_test_authenticator IN DATABASE :DBNAME SET pgrst."db_extra_search_path" = 'public, extensions, private';
+ALTER ROLE postgrest_test_authenticator IN DATABASE :DBNAME SET pgrst.jwt_secret = 'OVERRIDEREALLYREALLYREALLYREALLYVERYSAFE';
+ALTER ROLE postgrest_test_authenticator IN DATABASE :DBNAME SET pgrst.db_extra_search_path = 'public, extensions, private';
 
 -- other database settings that should be ignored
 DROP DATABASE IF EXISTS other;
 CREATE DATABASE other;
-ALTER ROLE postgrest_test_authenticator IN DATABASE other SET pgrst."db_max_rows" = '1111';
+ALTER ROLE postgrest_test_authenticator IN DATABASE other SET pgrst.db_max_rows = '1111';
 
 -- non-reloadable configs for io tests
-ALTER ROLE postgrest_test_authenticator SET pgrst."server_host" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."server_port" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."server_unix_socket" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."server_unix_socket_mode" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."log_level" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_anon_role" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_uri" = 'postgresql://ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_channel_enabled" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_channel" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_pool" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_pool_timeout" = 'ignored';
-ALTER ROLE postgrest_test_authenticator SET pgrst."db_load_guc_config" = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.server_host = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.server_port = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.server_unix_socket = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.server_unix_socket_mode = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.log_level = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_anon_role = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_uri = 'postgresql://ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_channel_enabled = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_channel = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_pool = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_pool_timeout = 'ignored';
+ALTER ROLE postgrest_test_authenticator SET pgrst.db_load_guc_config = 'ignored';
 
 -- other authenticator reloadable config options for io tests
 CREATE ROLE other_authenticator LOGIN NOINHERIT;
-ALTER ROLE other_authenticator SET pgrst."jwt_aud" = 'https://otherexample.org';
-ALTER ROLE other_authenticator SET pgrst."openapi_server_proxy_uri" = 'https://otherexample.org/api';
-ALTER ROLE other_authenticator SET pgrst."raw_media_types" = 'application/vnd.pgrst.other-db-config';
-ALTER ROLE other_authenticator SET pgrst."jwt_secret" = 'ODERREALLYREALLYREALLYREALLYVERYSAFE';
-ALTER ROLE other_authenticator SET pgrst."jwt_secret_is_base64" = 'true';
-ALTER ROLE other_authenticator SET pgrst."jwt_role_claim_key" = '."other"."role"';
-ALTER ROLE other_authenticator SET pgrst."db_tx_end" = 'rollback-allow-override';
-ALTER ROLE other_authenticator SET pgrst."db_schemas" = 'test, other_tenant1, other_tenant2';
-ALTER ROLE other_authenticator SET pgrst."db_root_spec" = 'other_root';
-ALTER ROLE other_authenticator SET pgrst."db_prepared_statements" = 'false';
-ALTER ROLE other_authenticator SET pgrst."db_pre_request" = 'test.other_custom_headers';
-ALTER ROLE other_authenticator SET pgrst."db_max_rows" = '100';
-ALTER ROLE other_authenticator SET pgrst."db_extra_search_path" = 'public, extensions, other';
+ALTER ROLE other_authenticator SET pgrst.jwt_aud = 'https://otherexample.org';
+ALTER ROLE other_authenticator SET pgrst.openapi_server_proxy_uri = 'https://otherexample.org/api';
+ALTER ROLE other_authenticator SET pgrst.raw_media_types = 'application/vnd.pgrst.other-db-config';
+ALTER ROLE other_authenticator SET pgrst.jwt_secret = 'ODERREALLYREALLYREALLYREALLYVERYSAFE';
+ALTER ROLE other_authenticator SET pgrst.jwt_secret_is_base64 = 'true';
+ALTER ROLE other_authenticator SET pgrst.jwt_role_claim_key = '."other"."role"';
+ALTER ROLE other_authenticator SET pgrst.db_tx_end = 'rollback-allow-override';
+ALTER ROLE other_authenticator SET pgrst.db_schemas = 'test, other_tenant1, other_tenant2';
+ALTER ROLE other_authenticator SET pgrst.db_root_spec = 'other_root';
+ALTER ROLE other_authenticator SET pgrst.db_prepared_statements = 'false';
+ALTER ROLE other_authenticator SET pgrst.db_pre_request = 'test.other_custom_headers';
+ALTER ROLE other_authenticator SET pgrst.db_max_rows = '100';
+ALTER ROLE other_authenticator SET pgrst.db_extra_search_path = 'public, extensions, other';
diff --git a/test/fixtures/schema.sql b/test/fixtures/schema.sql
@@ -1928,7 +1928,7 @@ select * from pg_catalog.pg_prepared_statements;
 create or replace function change_max_rows_config(val int, notify bool default false) returns void as $_$
 begin
   execute format($$
-    alter role postgrest_test_authenticator set pgrst."db_max_rows" = %L;
+    alter role postgrest_test_authenticator set pgrst.db_max_rows = %L;
   $$, val);
   if notify then
     perform pg_notify('pgrst', 'reload config');
@@ -1937,33 +1937,39 @@ end $_$ volatile security definer language plpgsql ;
 
 create or replace function reset_max_rows_config() returns void as $_$
 begin
-  alter role postgrest_test_authenticator set pgrst."db_max_rows" = '1000';
+  alter role postgrest_test_authenticator set pgrst.db_max_rows = '1000';
 end $_$ volatile security definer language plpgsql ;
 
 create or replace function change_db_schema_and_full_reload(schemas text) returns void as $_$
 begin
   execute format($$
-    alter role postgrest_test_authenticator set pgrst."db_schemas" = %L;
+    alter role postgrest_test_authenticator set pgrst.db_schemas = %L;
   $$, schemas);
   perform pg_notify('pgrst', 'reload config');
   perform pg_notify('pgrst', 'reload schema');
 end $_$ volatile security definer language plpgsql ;
 
 create or replace function v1.reset_db_schema_config() returns void as $_$
 begin
-  alter role postgrest_test_authenticator set pgrst."db_schemas" = 'test';
+  alter role postgrest_test_authenticator set pgrst.db_schemas = 'test';
   perform pg_notify('pgrst', 'reload config');
   perform pg_notify('pgrst', 'reload schema');
 end $_$ volatile security definer language plpgsql ;
 
 create or replace function test.invalid_role_claim_key_reload() returns void as $_$
 begin
-  alter role postgrest_test_authenticator set pgrst."jwt_role_claim_key" = 'test';
+  alter role postgrest_test_authenticator set pgrst.jwt_role_claim_key = 'test';
   perform pg_notify('pgrst', 'reload config');
 end $_$ volatile security definer language plpgsql ;
 
 create or replace function test.reset_invalid_role_claim_key() returns void as $_$
 begin
-  alter role postgrest_test_authenticator set pgrst."jwt_role_claim_key" = '."a"."role"';
+  alter role postgrest_test_authenticator set pgrst.jwt_role_claim_key = '."a"."role"';
   perform pg_notify('pgrst', 'reload config');
 end $_$ volatile security definer language plpgsql ;
+
+create or replace function test.reload_pgrst_config() returns void as $_$
+begin
+  alter role postgrest_test_authenticator set pgrst.jwt_role_claim_key = '."a"."role"';
+  perform pg_notify('pgrst', 'reload config');
+end $_$ language plpgsql ;
diff --git a/test/io-tests/configs/sigusr2-settings-external-secret.config b/test/io-tests/configs/sigusr2-settings-external-secret.config
@@ -0,0 +1,5 @@
+db-pool = 1
+
+jwt-secret = "$(JWT_SECRET_FILE)"
+jwt-secret-is-base64 = false
+db-load-guc-config = false
diff --git a/test/io-tests/test_io.py b/test/io-tests/test_io.py
@@ -336,6 +336,7 @@ def test_stable_config(tmp_path, config, defaultenv):
         "ROLE_CLAIM_KEY": '."https://www.example.com/roles"[0].value',
         "POSTGREST_TEST_SOCKET": "/tmp/postgrest.sock",
         "POSTGREST_TEST_PORT": "80",
+        "JWT_SECRET_FILE": "a_file",
     }
 
     # Some configs expect input from stdin, at least on base64.
@@ -508,6 +509,53 @@ def test_jwt_secret_reload(tmp_path, defaultenv):
         assert response.status_code == 200
 
 
+def test_jwt_secret_external_file_reload(tmp_path, defaultenv):
+    "JWT secret external file should be reloaded when PostgREST is sent a SIGUSR2 or a NOTIFY."
+    config = CONFIGSDIR / "sigusr2-settings-external-secret.config"
+
+    headers = jwtauthheader({"role": "postgrest_test_author"}, SECRET)
+
+    external_secret_file = tmp_path / "jwt-secret-config"
+    external_secret_file.write_text("invalid" * 5)
+
+    env = {
+        **defaultenv,
+        "JWT_SECRET_FILE": f"@{external_secret_file}",
+        "PGRST_DB_CHANNEL_ENABLED": "true",
+    }
+
+    with run(config, env=env) as postgrest:
+        response = postgrest.session.get("/authors_only", headers=headers)
+        assert response.status_code == 401
+
+        # change external file
+        external_secret_file.write_text(SECRET)
+
+        # SIGUSR1 doesn't reload external files
+        postgrest.process.send_signal(signal.SIGUSR1)
+        time.sleep(0.1)
+
+        response = postgrest.session.get("/authors_only", headers=headers)
+        assert response.status_code == 401
+
+        # reload config and external file with SIGUSR2
+        postgrest.process.send_signal(signal.SIGUSR2)
+        time.sleep(0.1)
+
+        response = postgrest.session.get("/authors_only", headers=headers)
+        assert response.status_code == 200
+
+        # change external file to wrong value again
+        external_secret_file.write_text("invalid" * 5)
+
+        # reload config and external file with NOTIFY
+        postgrest.session.get("/rpc/reload_pgrst_config")
+        time.sleep(0.1)
+
+        response = postgrest.session.get("/authors_only", headers=headers)
+        assert response.status_code == 401
+
+
 def test_db_schema_reload(tmp_path, defaultenv):
     "DB schema should be reloaded when PostgREST is sent SIGUSR2."
     config = (CONFIGSDIR / "sigusr2-settings.config").read_text()
