Address review comments

Author: artemgavrilov

contrib/pg_tde/expected/access_control.out

@@ -37,6 +37,7 @@ GRANT EXECUTE ON FUNCTION pg_tde_delete_global_key_provider(TEXT) TO regress_pg_
 GRANT EXECUTE ON FUNCTION pg_tde_set_default_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
 GRANT EXECUTE ON FUNCTION pg_tde_set_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
 GRANT EXECUTE ON FUNCTION pg_tde_set_server_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
+GRANT EXECUTE ON FUNCTION pg_tde_delete_default_key() TO regress_pg_tde_access_control;
 SET ROLE regress_pg_tde_access_control;
 SELECT pg_tde_add_database_key_provider_file('local-file-provider', '/tmp/pg_tde_test_keyring.per');
 ERROR:  must be superuser to modify key providers
@@ -56,5 +57,7 @@ SELECT pg_tde_set_default_key_using_global_key_provider('key1', 'global-file-pro
 ERROR:  must be superuser to access global key providers
 SELECT pg_tde_set_server_key_using_global_key_provider('key1', 'global-file-provider');
 ERROR:  must be superuser to access global key providers
+SELECT pg_tde_delete_default_key();
+ERROR:  must be superuser to access global key providers
 RESET ROLE;
 DROP EXTENSION pg_tde CASCADE;

contrib/pg_tde/pg_tde--1.0-rc.sql

@@ -263,11 +263,13 @@ CREATE FUNCTION pg_tde_delete_key()
 RETURNS VOID
 LANGUAGE C
 AS 'MODULE_PATHNAME';
+REVOKE ALL ON FUNCTION pg_tde_delete_key() FROM PUBLIC;
 
 CREATE FUNCTION pg_tde_delete_default_key()
 RETURNS VOID
 LANGUAGE C
 AS 'MODULE_PATHNAME';
+REVOKE ALL ON FUNCTION pg_tde_delete_default_key() FROM PUBLIC;
 
 CREATE FUNCTION pg_tde_key_info()
 RETURNS TABLE ( key_name TEXT,

contrib/pg_tde/sql/access_control.sql

@@ -29,6 +29,7 @@ GRANT EXECUTE ON FUNCTION pg_tde_delete_global_key_provider(TEXT) TO regress_pg_
 GRANT EXECUTE ON FUNCTION pg_tde_set_default_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
 GRANT EXECUTE ON FUNCTION pg_tde_set_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
 GRANT EXECUTE ON FUNCTION pg_tde_set_server_key_using_global_key_provider(TEXT, TEXT, BOOLEAN) TO regress_pg_tde_access_control;
+GRANT EXECUTE ON FUNCTION pg_tde_delete_default_key() TO regress_pg_tde_access_control;
 
 SET ROLE regress_pg_tde_access_control;
 
@@ -41,6 +42,7 @@ SELECT pg_tde_delete_global_key_provider('global-file-provider');
 SELECT pg_tde_set_key_using_global_key_provider('key1', 'global-file-provider');
 SELECT pg_tde_set_default_key_using_global_key_provider('key1', 'global-file-provider');
 SELECT pg_tde_set_server_key_using_global_key_provider('key1', 'global-file-provider');
+SELECT pg_tde_delete_default_key();
 
 RESET ROLE;
 

contrib/pg_tde/src/access/pg_tde_tdemap.c

@@ -515,19 +515,16 @@ pg_tde_delete_principal_key_redo(Oid dbOid)
 {
 	char		path[MAXPGPATH];
 
-	LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
-
 	pg_tde_set_db_file_path(dbOid, path);
-	durable_unlink(path, WARNING);
 
+	LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
+	durable_unlink(path, WARNING);
 	LWLockRelease(tde_lwlock_enc_keys());
 }
 
 /*
  * Deletes the principal key for the database. This fucntion checks if key map
  * file has any entries, and if not, it removes the file. Otherwise raises an error.
- *
- * The caller must hold an exclusive lock on the files before calling this function.
  */
 void
 pg_tde_delete_principal_key(Oid dbOid)
@@ -686,8 +683,6 @@ pg_tde_find_map_entry(const RelFileLocator *rlocator, TDEMapEntryType key_type,
  * positive is more harmful this might not be.
  *
  * Works even if the database has no map file.
- * The caller must hold a shared or exclusive lock on the
- * tde_lwlock_enc_keys() lock.
  */
 int
 pg_tde_count_relations(Oid dbOid)

contrib/pg_tde/src/catalog/tde_principal_key.c

@@ -604,6 +604,8 @@ pg_tde_delete_key(PG_FUNCTION_ARGS)
 	if (principal_key == NULL)
 		ereport(ERROR, errmsg("principal key does not exists for the database"));
 
+	ereport(LOG, errmsg("Deleting principal key [%s] for the database", principal_key->keyInfo.name));
+
 	/*
 	 * If database has something encryted, we can try to fallback to the
 	 * default principal key
@@ -658,12 +660,19 @@ pg_tde_delete_default_key(PG_FUNCTION_ARGS)
 	TDEPrincipalKey *default_principal_key;
 	List	   *dbs = NIL;
 
+	if (!superuser())
+		ereport(ERROR,
+				errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				errmsg("must be superuser to access global key providers"));
+
 	LWLockAcquire(tde_lwlock_enc_keys(), LW_EXCLUSIVE);
 
 	default_principal_key = GetPrincipalKeyNoDefault(DEFAULT_DATA_TDE_OID, LW_EXCLUSIVE);
 	if (default_principal_key == NULL)
 		ereport(ERROR, errmsg("default principal key is not set"));
 
+	ereport(LOG, errmsg("Deleting default principal key [%s]", default_principal_key->keyInfo.name));
+
 	/*
 	 * Take row exclusive lock, as we do not want anybody to create/drop a
 	 * database in parallel. If it happens, its not the end of the world, but
@@ -707,7 +716,6 @@ pg_tde_delete_default_key(PG_FUNCTION_ARGS)
 		pg_tde_delete_principal_key(dbOid);
 		clear_principal_key_cache(dbOid);
 	}
-	list_free(dbs);
 
 	systable_endscan(scan);
 	table_close(rel, RowExclusiveLock);
@@ -717,6 +725,9 @@ pg_tde_delete_default_key(PG_FUNCTION_ARGS)
 	clear_principal_key_cache(DEFAULT_DATA_TDE_OID);
 
 	LWLockRelease(tde_lwlock_enc_keys());
+
+	list_free(dbs);
+
 	PG_RETURN_VOID();
 }
 

contrib/pg_tde/t/expected/basic.out

@@ -12,11 +12,9 @@ CREATE EXTENSION IF NOT EXISTS pg_tde;
 		ORDER BY pg_proc.oid::regprocedure::text;
               oid              
 -------------------------------
- pg_tde_delete_default_key()
- pg_tde_delete_key()
  pg_tde_is_encrypted(regclass)
  pg_tde_version()
-(4 rows)
+(2 rows)
 
 
 SELECT extname, extversion FROM pg_extension WHERE extname = 'pg_tde';