From 07756f27345b3243c379b2fd10f5ec396e3fc94a Mon Sep 17 00:00:00 2001 From: Andreas Karlsson Date: Sat, 5 Apr 2025 23:54:54 +0200 Subject: [PATCH] PG-1530 Fix off-by-one bug when determining which WAL key to use The end LSN of the current buffer to decript was exclusive while the end LSN of the key was inclusive which had led to confusion and an off-by-one bug. Also add a simple test case for the WAL encryption using the logical decoding's test plugin. --- contrib/pg_tde/src/access/pg_tde_tdemap.c | 2 +- .../pg_tde/src/access/pg_tde_xlog_encrypt.c | 2 +- contrib/pg_tde/t/009_wal_encrypt.pl | 47 ++++++++++++++++--- contrib/pg_tde/t/expected/009_wal_encrypt.out | 34 +++++++++++++- 4 files changed, 75 insertions(+), 10 deletions(-) diff --git a/contrib/pg_tde/src/access/pg_tde_tdemap.c b/contrib/pg_tde/src/access/pg_tde_tdemap.c index 4ec3a9c4a8e..ccdc445ce23 100644 --- a/contrib/pg_tde/src/access/pg_tde_tdemap.c +++ b/contrib/pg_tde/src/access/pg_tde_tdemap.c @@ -1460,7 +1460,7 @@ pg_tde_add_wal_key_to_cache(InternalKey *cached_key, XLogRecPtr start_lsn) else { tde_wal_key_last_rec->next = wal_rec; - tde_wal_key_last_rec->end_lsn = wal_rec->start_lsn - 1; + tde_wal_key_last_rec->end_lsn = wal_rec->start_lsn; tde_wal_key_last_rec = wal_rec; } diff --git a/contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c b/contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c index 5a6eee88040..42d6015076b 100644 --- a/contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c +++ b/contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c @@ -320,7 +320,7 @@ tdeheap_xlog_seg_read(int fd, void *buf, size_t count, off_t offset, * Check if the key's range overlaps with the buffer's and decypt * the part that does. */ - if (data_start <= curr_key->end_lsn && curr_key->start_lsn <= data_end) + if (data_start < curr_key->end_lsn && data_end > curr_key->start_lsn) { char iv_prefix[16]; diff --git a/contrib/pg_tde/t/009_wal_encrypt.pl b/contrib/pg_tde/t/009_wal_encrypt.pl index 77e9633ca41..1c5a0e7515d 100644 --- a/contrib/pg_tde/t/009_wal_encrypt.pl +++ b/contrib/pg_tde/t/009_wal_encrypt.pl @@ -19,6 +19,7 @@ my $pgdata = $node->data_dir; # UPDATE postgresql.conf to include/load pg_tde library open my $conf, '>>', "$pgdata/postgresql.conf"; print $conf "shared_preload_libraries = 'pg_tde'\n"; +print $conf "wal_level = 'logical'\n"; # NOT testing that it can't start: the test framework doesn't have an easy way to do this # print $conf "pg_tde.wal_encrypt = 1\n"; close $conf; @@ -35,27 +36,61 @@ PGTDE::append_to_file($stdout); $stdout = $node->safe_psql('postgres', "SELECT pg_tde_set_server_principal_key('global-db-principal-key', 'file-keyring-010');", extra_params => ['-a']); PGTDE::append_to_file($stdout); +$stdout = $node->safe_psql('postgres', 'ALTER SYSTEM SET pg_tde.wal_encrypt = on;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + # Restart the server, it should work with encryption now PGTDE::append_to_file("-- server restart with wal encryption"); $node->stop(); +$rt_value = $node->start(); +ok($rt_value == 1, "Restart Server"); -open $conf, '>>', "$pgdata/postgresql.conf"; -print $conf "pg_tde.wal_encrypt = 1\n"; -close $conf; +$stdout = $node->safe_psql('postgres', "SHOW pg_tde.wal_encrypt;", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "SELECT slot_name FROM pg_create_logical_replication_slot('tde_slot', 'test_decoding');", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'CREATE TABLE test_wal (id SERIAL, k INTEGER, PRIMARY KEY (id));', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'INSERT INTO test_wal (k) VALUES (1), (2);', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'ALTER SYSTEM SET pg_tde.wal_encrypt = off;', extra_params => ['-a']); +PGTDE::append_to_file($stdout); +PGTDE::append_to_file("-- server restart without wal encryption"); +$node->stop(); $rt_value = $node->start(); ok($rt_value == 1, "Restart Server"); $stdout = $node->safe_psql('postgres', "SHOW pg_tde.wal_encrypt;", extra_params => ['-a']); PGTDE::append_to_file($stdout); -$stdout = $node->safe_psql('postgres', 'CREATE TABLE test_wal(id SERIAL,k INTEGER,PRIMARY KEY (id));', extra_params => ['-a']); +$stdout = $node->safe_psql('postgres', 'INSERT INTO test_wal (k) VALUES (3), (4);', extra_params => ['-a']); PGTDE::append_to_file($stdout); -$stdout = $node->safe_psql('postgres', 'CHECKPOINT;', extra_params => ['-a']); +$stdout = $node->safe_psql('postgres', 'ALTER SYSTEM SET pg_tde.wal_encrypt = on;', extra_params => ['-a']); PGTDE::append_to_file($stdout); -# TODO: add WAL content testing after the wal rework + +PGTDE::append_to_file("-- server restart with wal encryption"); +$node->stop(); +$rt_value = $node->start(); +ok($rt_value == 1, "Restart Server"); + +$stdout = $node->safe_psql('postgres', "SHOW pg_tde.wal_encrypt;", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', 'INSERT INTO test_wal (k) VALUES (5), (6);', extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "SELECT data FROM pg_logical_slot_get_changes('tde_slot', NULL, NULL);", extra_params => ['-a']); +PGTDE::append_to_file($stdout); + +$stdout = $node->safe_psql('postgres', "SELECT pg_drop_replication_slot('tde_slot');", extra_params => ['-a']); +PGTDE::append_to_file($stdout); # DROP EXTENSION $stdout = $node->safe_psql('postgres', 'DROP EXTENSION pg_tde;', extra_params => ['-a']); diff --git a/contrib/pg_tde/t/expected/009_wal_encrypt.out b/contrib/pg_tde/t/expected/009_wal_encrypt.out index 67b4e5e56d9..f4a4b1922f8 100644 --- a/contrib/pg_tde/t/expected/009_wal_encrypt.out +++ b/contrib/pg_tde/t/expected/009_wal_encrypt.out @@ -3,9 +3,39 @@ SELECT pg_tde_add_global_key_provider_file('file-keyring-010','/tmp/pg_tde_test_ -1 SELECT pg_tde_set_server_principal_key('global-db-principal-key', 'file-keyring-010'); +ALTER SYSTEM SET pg_tde.wal_encrypt = on; -- server restart with wal encryption SHOW pg_tde.wal_encrypt; on -CREATE TABLE test_wal(id SERIAL,k INTEGER,PRIMARY KEY (id)); -CHECKPOINT; +SELECT slot_name FROM pg_create_logical_replication_slot('tde_slot', 'test_decoding'); +tde_slot +CREATE TABLE test_wal (id SERIAL, k INTEGER, PRIMARY KEY (id)); +INSERT INTO test_wal (k) VALUES (1), (2); +ALTER SYSTEM SET pg_tde.wal_encrypt = off; +-- server restart without wal encryption +SHOW pg_tde.wal_encrypt; +off +INSERT INTO test_wal (k) VALUES (3), (4); +ALTER SYSTEM SET pg_tde.wal_encrypt = on; +-- server restart with wal encryption +SHOW pg_tde.wal_encrypt; +on +INSERT INTO test_wal (k) VALUES (5), (6); +SELECT data FROM pg_logical_slot_get_changes('tde_slot', NULL, NULL); +BEGIN 739 +COMMIT 739 +BEGIN 740 +table public.test_wal: INSERT: id[integer]:1 k[integer]:1 +table public.test_wal: INSERT: id[integer]:2 k[integer]:2 +COMMIT 740 +BEGIN 741 +table public.test_wal: INSERT: id[integer]:3 k[integer]:3 +table public.test_wal: INSERT: id[integer]:4 k[integer]:4 +COMMIT 741 +BEGIN 742 +table public.test_wal: INSERT: id[integer]:5 k[integer]:5 +table public.test_wal: INSERT: id[integer]:6 k[integer]:6 +COMMIT 742 +SELECT pg_drop_replication_slot('tde_slot'); + DROP EXTENSION pg_tde;