WAL: encrypt segment ranges #64

dAdAbird · 2025-02-13T12:40:44Z

WIP

Encryption and decryption works. Server would create WAL internal key during the start if needed (if wal_encryption GUC has changed)

TODO:

Refactoring
Comments
Docs

it-percona-cla · 2025-02-13T12:40:48Z

All committers have signed the CLA.

Pass additional args to seg_read/seg_write functions so they don't rely on page header to calculate IV offsets

We don't rely on page headers anymore. All nececary encryption data now in internal keys

dutow · 2025-02-17T08:24:25Z

src/include/access/xlog.h

@@ -58,6 +58,8 @@ extern PGDLLIMPORT int wal_decode_buffer_size;

 extern PGDLLIMPORT int CheckPointSegments;

+extern PGDLLIMPORT bool wal_need_seg_switch;


Leftover variable? I see no use anywhere.

dutow · 2025-02-17T08:38:24Z

contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c

 		{
-			if (key == NULL)
+			/* 
+			 * Check if the key's range overlaps with the buffer's and decypt


This is the optimal case, but if we can have multiple keys in the same segment file, not necessarily true. We can turn on / off / on / off / on / off ... wal encryption, and this function will have to deal with scenarios where the specified range is encrypted by many keys in this case.

It works with multiple keys. We decrypt everything up to the key's end_lsn or the buffer end (whatever comes first). And the go to the next key and so on and so forth.
The implementation here isn't the most efficient though. See my reply to your loop comments below.

dutow · 2025-02-17T08:40:40Z

contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c

+	XLogSegNoOffsetToRecPtr(segno, offset + count, wal_segsz_bytes, data_end);
+
+	curr_key = keys;
+	while (curr_key)


This while loop (and generally the use of a linked list here) seems to be far from optimal, especially for a long running server.

We can assume that usually we want to read the more recent records - iterating from the last key would make more sense

We can assume that multiple keys to the seq read function usually read sequentially. The key for the second call will be most likely the same key as the first key, or if not, the next one.

And if we have many keys, using a vector and logarithmic search would be much better.

Yeah, I know. This is one of TODOs, but I forgot to put it in writing.
Linked lists are rubbish and the implementation here is far from ideal. We shouldn't go through the key but rather pick buffer the needed range(s). But as you said, there has to be some b-tree or heap or whatever log data structure for that. There plethora of nuances that can be improved. But I'd like to make it work altogether and then improve from there.

dutow · 2025-02-17T08:41:53Z

contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c

+				PG_TDE_DECRYPT_DATA(iv_prefix, dec_off,
+							(char *) buf + (offset - dec_off),
+							dec_sz, (char *) buf + (offset - dec_off),
+							curr_key->key);


missing break here if we the current lsn of the key is > end lsn of data?

dutow · 2025-02-17T08:42:44Z

contrib/pg_tde/src/access/pg_tde_xlog_encrypt.c

 		if (key == NULL)
 		{
 			ereport(ERROR,
-					(errmsg("WAL encryption can only be enabled with a properly configured key. Disable pg_tde.wal_encrypt and create one using pg_tde_crete_wal_key() before enabling it.")));
+					(errmsg("WAL encryption can only be enabled with a properly configured principal key. Disable pg_tde.wal_encrypt and create one using pg_tde_set_server_principal_key() before enabling it.")));


default principal key also should work.

dutow · 2025-02-17T08:47:13Z

contrib/pg_tde/src/include/access/pg_tde_tdemap.h

+#define TDE_KEY_TYPE_HEAP_BASIC		0x01
+#define TDE_KEY_TYPE_SMGR			0x02
+#define TDE_KEY_TYPE_GLOBAL			0x04
+#define TDE_KEY_TYPE_WAL_DECRYPTED	0x08


Unencrypted?

dutow · 2025-02-17T09:37:43Z

Also, in this implementation we only generate a new internal key for WAL on server restart, if the encryption status is changed - the server has to be started with wal encryption off, shut down then again with wal encryption on, as it is only done in the initialization code after changes.

When we discussed ideas, I thought the concept was to have a different internal key for each wal segment file, to make it more robust/secure.

jeltz · 2025-02-17T10:46:04Z

src/backend/access/transam/xlog.c

@@ -144,6 +144,7 @@ bool		XLOG_DEBUG = false;

 int			wal_segment_size = DEFAULT_XLOG_SEG_SIZE;

+bool		wal_need_seg_switch = false;


Why do we need to swtich segments? I thought with the new solution we would not need to do that?

It's a leftover, I'll remove it

jeltz · 2025-02-17T10:51:00Z

contrib/pg_tde/src/include/access/pg_tde_tdemap.h

+	RelKeyData *key;
+
+	struct WALKeyCacheRec *next;
+} WALKeyCacheRec;


What is the plan to handle timelines? Do they cause any issues?

This is the thing a wanted to discuss today

dAdAbird added 2 commits February 15, 2025 19:42

WAL per-segment encryption

a1f8901

Pass additional args to seg_read/seg_write functions so they don't rely on page header to calculate IV offsets

Range-based WAL encryption

c8c3b97

We don't rely on page headers anymore. All nececary encryption data now in internal keys

dAdAbird force-pushed the wal_enc_range branch from 801bdd0 to c8c3b97 Compare February 15, 2025 17:46

dutow reviewed Feb 17, 2025

View reviewed changes

jeltz reviewed Feb 17, 2025

View reviewed changes

dAdAbird added 2 commits February 17, 2025 20:47

Fixed buffer allocation and addressed some review comments

ec3f49f

Fix replication

38274aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WAL: encrypt segment ranges #64

WAL: encrypt segment ranges #64

dAdAbird commented Feb 13, 2025 •

edited

Loading

it-percona-cla commented Feb 13, 2025 •

edited

Loading

dutow Feb 17, 2025

dutow Feb 17, 2025

dAdAbird Feb 17, 2025

dutow Feb 17, 2025

dAdAbird Feb 17, 2025

dutow Feb 17, 2025

dAdAbird Feb 17, 2025

dutow Feb 17, 2025

dutow Feb 17, 2025

dutow commented Feb 17, 2025

jeltz Feb 17, 2025

dAdAbird Feb 17, 2025

jeltz Feb 17, 2025

dAdAbird Feb 17, 2025

		@@ -58,6 +58,8 @@ extern PGDLLIMPORT int wal_decode_buffer_size;

		extern PGDLLIMPORT int CheckPointSegments;

		extern PGDLLIMPORT bool wal_need_seg_switch;

		@@ -144,6 +144,7 @@ bool XLOG_DEBUG = false;

		int wal_segment_size = DEFAULT_XLOG_SEG_SIZE;

		bool wal_need_seg_switch = false;

WAL: encrypt segment ranges #64

Are you sure you want to change the base?

WAL: encrypt segment ranges #64

Conversation

dAdAbird commented Feb 13, 2025 • edited Loading

it-percona-cla commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dutow commented Feb 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dAdAbird commented Feb 13, 2025 •

edited

Loading

it-percona-cla commented Feb 13, 2025 •

edited

Loading