PG-1968 Add support for AES 256 encryption #465

dAdAbird · 2025-11-12T13:07:14Z

A new GUC option is applied to all newly created keys - both principal and internal.
We had to change the _keys file format to accommodate 32-byte internal keys. So the keys file migration routine was added at server start.

codecov-commenter · 2025-11-12T15:12:40Z

Codecov Report

❌ Patch coverage is 38.62661% with 143 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.07%. Comparing base (44684d3) to head (2c38527).
⚠️ Report is 15 commits behind head on main.

❌ Your project status has failed because the head coverage (59.07%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #465      +/-   ##
==========================================
- Coverage   59.69%   59.07%   -0.63%     
==========================================
  Files          67       68       +1     
  Lines       10471    10653     +182     
  Branches     1813     1836      +23     
==========================================
+ Hits         6251     6293      +42     
- Misses       3524     3659     +135     
- Partials      696      701       +5

Components	Coverage Δ
access	`72.96% <19.49%> (-11.93%)`	⬇️
catalog	`87.93% <100.00%> (ø)`
common	`77.77% <ø> (ø)`
encryption	`73.63% <85.71%> (+2.06%)`	⬆️
keyring	`73.98% <100.00%> (+0.44%)`	⬆️
src	`92.11% <65.38%> (-2.10%)`	⬇️
smgr	`94.06% <100.00%> (ø)`
transam	`∅ <ø> (∅)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

AndersAstrand

The commit messages needs work since they current reveal very little of what the commits actually do.

I also think we need to discuss the technical debt we take on here by pushing generalization of the file format versions to the future. Adding "old" and "new" versions of all the functions works for now ofc, but means that it's something we need to clean up later.

Maybe it's worth taking on that debt now, I'm not sure.

src/encryption/enc_aes.c

src/encryption/enc_tde.c

src/pg_tde_guc.c

src/smgr/pg_tde_smgr.c

AndersAstrand · 2025-11-19T16:03:03Z

src/keyring/keyring_api.c

We should probably log a WARNING or NOTICE if the key loaded from the KMS doesn't match the current setting for TdeKeyLength, since they obviously want some other cipher than what they're getting.

Maybe also add a error_hint() about how to rotate the principal key?

It might also make sense to check that the Principal Key's length matches that of the Internal Key (and issue a WARNING if it doesn't) whenever we decrypt the latter. What do you think? Or is it an overkill?

Hmm... yes. that makes sense. I wonder if we want WARNING on them or just LOG or INFO? I think we could spam the warning log a lot otherwise.

src/pg_tde_guc.c

fetools/pg17/pg_basebackup/pg_basebackup.c

AndersAstrand · 2025-11-19T18:29:37Z

src/access/pg_tde_tdemap.c

 				errmsg("TDE map file \"%s\" is corrupted: %m", tde_filename));
 	}
+
+	return fheader->file_version;


I'm not sure what we gain by returning this value instead of just using the field from the header structure after the call?

It seemed like a cleaner interface to me. But looking at it now, you're right, it doesn't worth changes

AndersAstrand · 2025-11-20T12:00:56Z

src/access/pg_tde_xlog_keys.c

+typedef struct WalKeyFileEntry
+{
+	uint32		key_len;
+	CipherType	cipher;			/* Cipher type. We support only AES for now. */


Should we use uint32 here as we do with the other enum that's written directly to file?

This probably doesn't matter in practice for this enum as it's unlikely to hold more than a handful of values, but I would prefer to get an error if we add so many entries the size of the enum grows rather than the file format randomly changing because of it.

jeltz · 2025-11-13T20:18:41Z

fetools/pg17/pg_basebackup/pg_basebackup.c

 		}
 		pg_tde_save_server_key(principalKey, false);
-		TDEXLogSmgrInitWrite(true);
+		TDEXLogSmgrInitWrite(true, 16);


I don't like seeing a random 16 in the source, but will need to think what the right solution is.

I think it's fine as an intermediate step before the longer keys are supported in the later commits.

I'll fix it in the upcoming PR(s) when taking care of fetools

src/access/pg_tde_tdemap.c

dAdAbird · 2025-11-24T12:59:53Z

I also think we need to discuss the technical debt we take on here by pushing generalization of the file format versions to the future. Adding "old" and "new" versions of all the functions works for now ofc, but means that it's something we need to clean up later.

Maybe it's worth taking on that debt now, I'm not sure.

@AndersAstrand what other option do you see?
We could clean up the migration and old/new stuff in the next next release if there is no posibillity to skip releases while upgrading

AndersAstrand · 2025-11-25T12:47:07Z

I also think we need to discuss the technical debt we take on here by pushing generalization of the file format versions to the future. Adding "old" and "new" versions of all the functions works for now ofc, but means that it's something we need to clean up later.
Maybe it's worth taking on that debt now, I'm not sure.

@AndersAstrand what other option do you see? We could clean up the migration and old/new stuff in the next next release if there is no posibillity to skip releases while upgrading

The other option would be to not be just "old/new" but instead more explicit about versions. But I think it's ok to leave this in for now and fix it when we need to.

I think I'm also just a little allergic to using the words "old/new" in code as it tends to not say much about what it actually is.

dAdAbird · 2025-11-25T16:17:39Z

I also think we need to discuss the technical debt we take on here by pushing generalization of the file format versions to the future. Adding "old" and "new" versions of all the functions works for now ofc, but means that it's something we need to clean up later.
Maybe it's worth taking on that debt now, I'm not sure.

@AndersAstrand what other option do you see? We could clean up the migration and old/new stuff in the next next release if there is no posibillity to skip releases while upgrading

The other option would be to not be just "old/new" but instead more explicit about versions. But I think it's ok to leave this in for now and fix it when we need to.

I think I'm also just a little allergic to using the words "old/new" in code as it tends to not say much about what it actually is.

Tbh, I hacked that for the POC and focused on other things after that. But I agree with your disdain for old/new. I have some Idea and will try to do an additional commit (or a refactoring PR if we merge this one by then) later this week.

src/access/pg_tde_tdemap.c

dutow · 2025-11-27T18:06:20Z

src/encryption/enc_aes.c

 		*ctxPtr = EVP_CIPHER_CTX_new();

-		if (EVP_CipherInit_ex(*ctxPtr, cipher_ctr_ecb, NULL, key, NULL, 1) == 0)
+		if (EVP_CipherInit_ex(*ctxPtr, cipher, NULL, key, NULL, 1) == 0)


Not completely related to the PR, but I realized that now we only cache the context for WAL and nothing else.

For everything else, ctx is always created internally every time.

This is just something we should investigate, as the mixed implementation most likely isn't practical.

Maybe we should also have an assert about the cipher size in this function, when we are reusing an older context?

src/include/pg_tde.h

src/access/pg_tde_tdemap.c

Adds a respective GUC and enables TDE AES lib to handle 128 and 256-bit keys. Also adds the support for 256 bit encryption of primary keys

It adds `pg_tde/keys_version`, which contains the latest magic number (version) of the wal and smgr keys. If those versions don't match the current ones at the server start, it will rewrite _keys to the new format if needed and update keys_version. If the start crashed along the way updating _keys, it will retry again on the next start since keys_version wasn't updated.

Instead of _old/_new we can extract the version from FILE_MAGIC. And define a function that extracts an entry from this particular file version and transforms it to the current entry format. That way, there is no need to define new "FILE_MAGIC" constants; instead, simply bump the version of the current ones, keeping the naming format. That way, we are restricted to 255 file versions, but let's solve it when we get there. Also, all named size constants were replaced with the numeric values in the previous format instances to avoid breaking old formats if we change any of those sizes for the current one. If it's ok, I'll squash it into the prev commit

WAL and replication tested with the modified pg_rewind test

dAdAbird force-pushed the aes_256 branch from 226259f to dfdc064 Compare November 13, 2025 11:16

dAdAbird mentioned this pull request Nov 14, 2025

PG-1923 WIP 256-bit encryption (take 2) percona/postgres#587

Closed

AndersAstrand requested changes Nov 20, 2025

View reviewed changes

jeltz reviewed Nov 21, 2025

View reviewed changes

dAdAbird force-pushed the aes_256 branch from dfdc064 to 6b677bf Compare November 25, 2025 17:21

dutow reviewed Nov 27, 2025

View reviewed changes

AndersAstrand changed the title ~~Add support for AES 256 encryption~~ PG-1968 Add support for AES 256 encryption Nov 28, 2025

dAdAbird force-pushed the aes_256 branch from 6b677bf to b810d78 Compare November 28, 2025 15:23

dutow reviewed Nov 28, 2025

View reviewed changes

src/include/pg_tde.h Outdated Show resolved Hide resolved

src/access/pg_tde_tdemap.c Outdated Show resolved Hide resolved

PG-1968 Add support for AES 256

5fb6d73

Adds a respective GUC and enables TDE AES lib to handle 128 and 256-bit keys. Also adds the support for 256 bit encryption of primary keys

dAdAbird force-pushed the aes_256 branch from b810d78 to cabd7ef Compare November 28, 2025 16:09

dAdAbird added 4 commits November 28, 2025 20:51

PG-1968 Add support for AES 256 to WAL

b3e548a

PG-1968 Add AES 256 support to SMGR

d8f3d76

dAdAbird force-pushed the aes_256 branch from cabd7ef to 8c52991 Compare November 28, 2025 18:51

dAdAbird requested review from AndersAstrand, dutow and jeltz November 28, 2025 19:32

PG-2016 Add basic TAP tests for AES 256

2c38527

WAL and replication tested with the modified pg_rewind test

dAdAbird force-pushed the aes_256 branch from 8c52991 to 2c38527 Compare November 28, 2025 19:36

PG-1968 Add support for AES 256 encryption #465

Are you sure you want to change the base?

PG-1968 Add support for AES 256 encryption #465

Uh oh!

Conversation

dAdAbird commented Nov 12, 2025

Uh oh!

codecov-commenter commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AndersAstrand left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dAdAbird commented Nov 24, 2025

Uh oh!

AndersAstrand commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dAdAbird commented Nov 25, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Nov 12, 2025 •

edited

Loading

AndersAstrand left a comment •

edited

Loading

AndersAstrand commented Nov 25, 2025 •

edited

Loading