Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove 'percona_tde.pg_tde_key_provider' user catalog #230

Merged
merged 7 commits into from
Jul 5, 2024
Merged

Remove 'percona_tde.pg_tde_key_provider' user catalog #230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
47bce9a
Remove 'percona_tde.pg_tde_key_provider' user catalog and introduce p…
codeforall Jul 3, 2024
362ce9c
Fix test case
codeforall Jul 3, 2024
7f3d4fe
Update src/catalog/tde_keyring.c
codeforall Jul 5, 2024
6298d44
Update src/common/pg_tde_utils.c
codeforall Jul 5, 2024
343955a
Taking care of review comments
codeforall Jul 5, 2024
1839d39
Update src/catalog/tde_keyring.c
codeforall Jul 5, 2024
3025907
Update src/catalog/tde_keyring.c
codeforall Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions expected/keyprovider_dependency.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,4 @@ SELECT pg_tde_set_principal_key('test-db-principal-key','mk-file');
t
(1 row)

-- Try dropping the in-use key provider
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'mk-file'; -- Should fail
ERROR: Key provider "mk-file" cannot be deleted
DETAIL: The principal key for the database depends on this key provider.
-- Now delete the un-used key provider
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'free-file'; -- Should pass
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'V2-vault'; -- Should pass
DROP EXTENSION pg_tde;
33 changes: 4 additions & 29 deletions pg_tde--1.0.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,37 +3,16 @@
-- complain if script is sourced in psql, rather than via CREATE EXTENSION
\echo Use "CREATE EXTENSION pg_tde" to load this file. \quit

-- pg_tde catalog tables
CREATE SCHEMA percona_tde;
-- Note: The table is created using heap storage becasue we do not want this table
-- to be encrypted by pg_tde. This table is used to store key provider information
-- and we do not want to encrypt this table using pg_tde.
CREATE TABLE percona_tde.pg_tde_key_provider(provider_id SERIAL,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we replace this with a view that provides the same functionality?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we would need to provide a view and edit function as mentioned in the comment #230 (comment), but we can handle it as a separate PR

keyring_type VARCHAR(10) CHECK (keyring_type IN ('file', 'vault-v2')),
provider_name VARCHAR(255) UNIQUE NOT NULL, options JSON, PRIMARY KEY(provider_id)) using heap;

-- If you want to add new provider types, you need to make appropriate changes
-- in include/catalog/tde_keyring.h and src/catalog/tde_keyring.c files.

SELECT pg_catalog.pg_extension_config_dump('percona_tde.pg_tde_key_provider', '');

-- Trigger function to check principal key dependency on key provider row
CREATE FUNCTION keyring_delete_dependency_check_trigger()
RETURNS TRIGGER
-- Key Provider Management
CREATE FUNCTION pg_tde_add_key_provider_internal(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)
RETURNS INT
AS 'MODULE_PATHNAME'
LANGUAGE C;

CREATE TRIGGER pg_tde_key_provider_delete_dependency_check_trigger
BEFORE DELETE ON percona_tde.pg_tde_key_provider
FOR EACH ROW
EXECUTE FUNCTION keyring_delete_dependency_check_trigger();

-- Key Provider Management

CREATE OR REPLACE FUNCTION pg_tde_add_key_provider(provider_type VARCHAR(10), provider_name VARCHAR(128), options JSON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have pg_tde_remove_key_provider() and/or pg_tde_edit_key_provider()? And also pg_tde_list_key_providers(), since there is no way to check what providers are there

Plus, since we ain't restricted with the db access anymore, it make sense to have ability to set a "global" key provider which applies to all dbs by default by can be redefined to a db-local one. It needs thoughts around security restrictions. But met be convenient for clusters with multiple dbs. It's definitely not for now, just thoughts out loud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea. We can take it up as a separate PR.

RETURNS INT
AS $$
INSERT INTO percona_tde.pg_tde_key_provider (keyring_type, provider_name, options) VALUES (provider_type, provider_name, options) RETURNING provider_id;
SELECT pg_tde_add_key_provider_internal(provider_type, provider_name, options);
$$
LANGUAGE SQL;

Expand Down Expand Up @@ -77,10 +56,6 @@ AS $$
$$
LANGUAGE SQL;

CREATE FUNCTION pg_tde_get_keyprovider(provider_name text)
RETURNS VOID
AS 'MODULE_PATHNAME'
LANGUAGE C;
-- Table access method
CREATE FUNCTION pg_tdeam_handler(internal)
RETURNS table_am_handler
Expand Down
6 changes: 0 additions & 6 deletions sql/keyprovider_dependency.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,4 @@ SELECT pg_tde_add_key_provider_vault_v2('V2-vault','vault-token','percona.com/va

SELECT pg_tde_set_principal_key('test-db-principal-key','mk-file');

-- Try dropping the in-use key provider
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'mk-file'; -- Should fail
-- Now delete the un-used key provider
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'free-file'; -- Should pass
DELETE FROM percona_tde.pg_tde_key_provider WHERE provider_name = 'V2-vault'; -- Should pass

DROP EXTENSION pg_tde;
14 changes: 3 additions & 11 deletions src/access/pg_tde_tdemap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

#include "postgres.h"
#include "access/pg_tde_tdemap.h"
#include "catalog/pg_tablespace_d.h"
#include "transam/pg_tde_xact_handler.h"
#include "storage/fd.h"
#include "utils/wait_event.h"
Expand All @@ -29,6 +28,7 @@
#include "encryption/enc_aes.h"
#include "encryption/enc_tde.h"
#include "keyring/keyring_api.h"
#include "common/pg_tde_utils.h"

#include <openssl/rand.h>
#include <openssl/err.h>
Expand Down Expand Up @@ -273,21 +273,13 @@ tde_decrypt_rel_key(TDEPrincipalKey *principal_key, RelKeyData *enc_rel_key_data
inline void
pg_tde_set_db_file_paths(const RelFileLocator *rlocator, char *map_path, char *keydata_path)
{
char *db_path;

/* If this is a global space, than the call might be in a critial section
* (during XLog write) so we can't do GetDatabasePath as it calls palloc()
*/
if (rlocator->spcOid == GLOBALTABLESPACE_OID)
db_path = "global";
else
db_path = GetDatabasePath(rlocator->dbOid, rlocator->spcOid);

char *db_path = pg_tde_get_tde_file_dir(rlocator->dbOid, rlocator->spcOid);

if (map_path)
join_path_components(map_path, db_path, PG_TDE_MAP_FILENAME);
if (keydata_path)
join_path_components(keydata_path, db_path, PG_TDE_KEYDATA_FILENAME);
pfree(db_path);
}

/*
Expand Down
Loading
Loading