From f02c72d47e8ca88dc4eca5b5b8bfca33182998aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20=C3=85strand?= Date: Tue, 17 Jun 2025 08:19:14 +0200 Subject: [PATCH] DO NOT MERGE EVER MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some comments I had during a cursory read-through of the docs. My comments are prefixed with AÅ: --- contrib/pg_tde/documentation/docs/architecture/index.md | 6 ++++++ .../documentation/docs/command-line-tools/pg-waldump.md | 2 ++ contrib/pg_tde/documentation/docs/faq.md | 4 ++++ contrib/pg_tde/documentation/docs/features.md | 3 +++ contrib/pg_tde/documentation/docs/functions.md | 5 +++++ .../global-key-provider-configuration/set-principal-key.md | 2 ++ .../pg_tde/documentation/docs/how-to/multi-tenant-setup.md | 2 ++ contrib/pg_tde/documentation/docs/how-to/uninstall.md | 2 ++ .../pg_tde/documentation/docs/index/supported-versions.md | 1 + .../pg_tde/documentation/docs/index/table-access-method.md | 1 + contrib/pg_tde/documentation/docs/index/tde-limitations.md | 2 ++ contrib/pg_tde/documentation/docs/setup.md | 1 + contrib/pg_tde/documentation/docs/test.md | 1 + contrib/pg_tde/documentation/docs/variables.md | 1 + contrib/pg_tde/documentation/docs/yum.md | 1 + 15 files changed, 34 insertions(+) diff --git a/contrib/pg_tde/documentation/docs/architecture/index.md b/contrib/pg_tde/documentation/docs/architecture/index.md index 9abd070c4c9eb..2593a3603d797 100644 --- a/contrib/pg_tde/documentation/docs/architecture/index.md +++ b/contrib/pg_tde/documentation/docs/architecture/index.md @@ -4,6 +4,8 @@ Let's break down what it means. +AÅ: We need to make it clear that this file are the ambitions for pg_tde, because a lot of this is not yet in there. + **Customizable** means that `pg_tde` aims to support many different use cases: * Encrypting either every table in every database or only some tables in some databases @@ -349,8 +351,10 @@ In this case existing references to global providers, or the global default prin ## Typical setup scenarios ### Simple "one principal key" encryption +AÅ: I'm no linguist, but since these are instructions for someone to follow the progressive tense doesn't seem right to me. Even weirder that step 5 and 6 does not use the progressive tense but the rest does. 1. Passing the option from the postgres config file the extension: `shared_preload_libraries=‘pg_tde’` +AÅ: The above sentence makes no sense to me 2. `CREATE EXTENSION pg_tde;` in `template1` 3. Adding a global key provider 4. Adding a default principal key using the same global provider @@ -371,6 +375,7 @@ encryption is managed by the admins, normal users only have to create tables wit specific databases HAVE to use the global key provider Note: setting the `default_table_access_method` to `tde_heap` is possible, but instead of `ALTER SYSTEM` only per database using `ALTER DATABASE`, after a principal key is configured for that specific database. +AÅ: Above language seems a bit awkward here to me Alternatively `ALTER SYSTEM` is possible, but table creation in the database will fail if there's no principal key for the database, that has to be created first. @@ -382,5 +387,6 @@ Alternatively `ALTER SYSTEM` is possible, but table creation in the database wil 4. Changing the WAL encryption to use the proper global key provider No default configuration: key providers / principal keys are configured as a per database level, permissions are managed per database +AÅ: what does "configured as a per database level" mean? Same note about `default_table_access_method` as above - but in a multi tenant setup, `ALTER SYSTEM` doesn't make much sense. diff --git a/contrib/pg_tde/documentation/docs/command-line-tools/pg-waldump.md b/contrib/pg_tde/documentation/docs/command-line-tools/pg-waldump.md index a3ac50df4df02..cf0b9df4ec892 100644 --- a/contrib/pg_tde/documentation/docs/command-line-tools/pg-waldump.md +++ b/contrib/pg_tde/documentation/docs/command-line-tools/pg-waldump.md @@ -1,5 +1,7 @@ # pg_waldump +AÅ: We should probably include a notice about WAL encrypting being BETA and this information is subject to change + [`pg_waldump` :octicons-link-external-16:](https://www.postgresql.org/docs/current/pgwaldump.html) is a tool to display a human-readable rendering of the Write-Ahead Log (WAL) of a PostgreSQL database cluster. To read encrypted WAL records, `pg_waldump` supports the following additional arguments: diff --git a/contrib/pg_tde/documentation/docs/faq.md b/contrib/pg_tde/documentation/docs/faq.md index df80bea17f555..8b7d0960a0355 100644 --- a/contrib/pg_tde/documentation/docs/faq.md +++ b/contrib/pg_tde/documentation/docs/faq.md @@ -87,6 +87,7 @@ The initial decision on what file to encrypt is based on the table access method The principal key is used to encrypt the internal keys. The principal key is stored in the key management store. When you query the table, the principal key is retrieved from the key store to decrypt the table. Then the internal key for that table is used to decrypt the data. ### WAL encryption +AÅ: We should mention this is a BETA feature and subject to change. WAL encryption is done globally for the entire database cluster. All modifications to any database within a PostgreSQL cluster are written to the same WAL to maintain data consistency and integrity and ensure that PostgreSQL cluster can be restored to a consistent state. Therefore, WAL is encrypted globally. @@ -117,6 +118,7 @@ The support of other encryption mechanisms such as AES256 is planned for future ## Is post-quantum encryption supported? No, it's not yet supported. In our implementation we reply on OpenSSL libraries that don't yet support post-quantum encryption. +AÅ: Afaik newer versions of openssl support post-quantum algorithms. Maybe we should use some better excuse or mention what version we use? ## Can I encrypt an existing table? @@ -134,6 +136,7 @@ You must restart the database in the following cases to apply the changes: * after you enabled the `pg_tde` extension * to turn on / off the WAL encryption +AÅ: WAL encrytion is not yet production ready and should probably not be casually mentioned as something you can "turn on". After that, no database restart is required. When you create or alter the table using the `tde_heap` access method, the files are marked as those that require encryption. The encryption happens at the storage manager level, before a transaction is written to disk. Read more about [how tde_heap works](index/table-access-method.md#how-tde_heap-works). @@ -150,6 +153,7 @@ In `pg_tde`, multi-tenancy is supported via a separate principal key per databas To control user access to the databases, you can use role-based access control (RBAC). WAL files are encrypted globally across the entire PostgreSQL cluster using the same encryption keys. Users don't interact with WAL files as these are used by the database management system to ensure data integrity and durability. +AÅ: Again, WAL encryption hould probably not be mentioned as if it was a feature already. ## Are my backups safe? Can I restore from them? diff --git a/contrib/pg_tde/documentation/docs/features.md b/contrib/pg_tde/documentation/docs/features.md index e7dc760e1c821..6cf5690c12956 100644 --- a/contrib/pg_tde/documentation/docs/features.md +++ b/contrib/pg_tde/documentation/docs/features.md @@ -10,15 +10,18 @@ The following features are available for the extension: * Index data for encrypted tables * TOAST tables * Temporary tables created during database operations +AÅ: Created how? !!! note Metadata of those tables is not encrypted. * Global Write-Ahead Log (WAL) encryption for data in both encrypted and non-encrypted tables +AÅ: WAL encryption should not be mentioned as a feature since we don't yet have it in more than a beta preview. * Single-tenancy support via a global keyring provider * Multi-tenancy support * Table-level granularity for encryption and access control * Multiple Key management options * Logical replication of encrypted tables +AÅ: We do not do anything for logical replication, do we? [Overview](index/index.md){.md-button} [Get Started](install.md){.md-button} diff --git a/contrib/pg_tde/documentation/docs/functions.md b/contrib/pg_tde/documentation/docs/functions.md index 563d359aa2cf4..1c85d5c2f6641 100644 --- a/contrib/pg_tde/documentation/docs/functions.md +++ b/contrib/pg_tde/documentation/docs/functions.md @@ -10,6 +10,7 @@ However, database owners can run the “view keys” and “set principal key” * `GRANT EXECUTE` * `REVOKE EXECUTE` +AÅ: This should probably say `GRANT EXECUTE ON FUNCTION` and similar for revoke ## Key provider management @@ -52,6 +53,8 @@ The `change` functions require the same parameters as the `add` functions. They Provider specific parameters differ for each implementation. Refer to the respective subsection for details. +AÅ: We should probably have a notice here about the modified provider settings will need to be able to provide the exact same principal keys as the original settings. + **Some provider specific parameters contain sensitive information, such as passwords. Never specify these directly, use the remote configuration option instead.** #### Adding or modifying Vault providers @@ -231,6 +234,7 @@ These functions list the details of all key providers for the current database o ## Principal key management Use these functions to create a new principal key at a given keyprover, and to use those keys for a specific scope such as a current database, a global or default scope. You can also use them to start using a different existing key for a specific scope. +AÅ: "keyprover"? Princial keys are stored on key providers by the name specified in this function - for example, when using the Vault provider, after creating a key named "foo", a key named "foo" will be visible on the Vault server at the specified mount point. @@ -281,6 +285,7 @@ SELECT pg_tde_set_key_using_global_key_provider( ### pg_tde_set_server_key_using_global_key_provider Sets or rotates the server principal key using the specified global key provider. Use this function to set a principal key for WAL encryption. +AÅ: Note that WAL encryption is not yet production ready ```sql SELECT pg_tde_set_server_key_using_global_key_provider( diff --git a/contrib/pg_tde/documentation/docs/global-key-provider-configuration/set-principal-key.md b/contrib/pg_tde/documentation/docs/global-key-provider-configuration/set-principal-key.md index 4c3cc3802d04e..674cc0258318a 100644 --- a/contrib/pg_tde/documentation/docs/global-key-provider-configuration/set-principal-key.md +++ b/contrib/pg_tde/documentation/docs/global-key-provider-configuration/set-principal-key.md @@ -36,6 +36,8 @@ The key material (actual cryptographic key) is auto-generated by `pg_tde` and st !!! note This process sets the **default principal key** for the server. Any database without its own key configuration will use this key. +AÅ: This is confusing to me, but maybe makes sense in the rendered version. The Example below does _not_ set a default key, does it? + ## Example This example is for testing purposes only. Replace the key name and provider name with your values: diff --git a/contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md b/contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md index 247a1878c254b..53e44d6a88383 100644 --- a/contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md +++ b/contrib/pg_tde/documentation/docs/how-to/multi-tenant-setup.md @@ -82,6 +82,7 @@ You must do these steps for every database where you have created the extension. === "With HashiCorp Vault" The Vault server setup is out of scope of this document. + AÅ: Why doesn't KMIP have a similar comment? Is KMIP more in scope than vault? Why isn't OpenBao mentioned here? ```sql SELECT pg_tde_add_database_key_provider_vault_v2('provider-name', 'url', 'mount', 'secret_token_path', 'ca_path'); @@ -144,6 +145,7 @@ You must do these steps for every database where you have created the extension. * `name-of-the-key` is the name of the principal key. You will use this name to identify the key. * `provider-name` is the name of the key provider you added before. The principal key will be associated with this provider. + AÅ: It will not only be "associated" with it, it is where the key will be stored and fetched from. :material-information: Warning: This example is for testing purposes only: diff --git a/contrib/pg_tde/documentation/docs/how-to/uninstall.md b/contrib/pg_tde/documentation/docs/how-to/uninstall.md index 7901f778fd8f4..64998f08e12c4 100644 --- a/contrib/pg_tde/documentation/docs/how-to/uninstall.md +++ b/contrib/pg_tde/documentation/docs/how-to/uninstall.md @@ -4,6 +4,8 @@ If you no longer wish to use TDE in your deployment, you can remove the `pg_tde` Here's how to do it: +AÅ: We don't think that anyone would like to drop the extension, but keep their data? Ie. should this document mention decryption of tables before dropping rather than only how to drop the tables? + 1. Drop the extension using the `DROP EXTENSION` command: ```sql diff --git a/contrib/pg_tde/documentation/docs/index/supported-versions.md b/contrib/pg_tde/documentation/docs/index/supported-versions.md index ed4758d0b671c..17f7ec1fa709d 100644 --- a/contrib/pg_tde/documentation/docs/index/supported-versions.md +++ b/contrib/pg_tde/documentation/docs/index/supported-versions.md @@ -9,6 +9,7 @@ The extension is tightly integrated with Percona Server for PostgreSQL to delive By using our PostgreSQL distribution, you get: - **Full encryption support** through the `tde_heap` access method, including tables, indexes, WAL data, and logical replication. +AÅ: Logical replication? - **Enhanced performance and enterprise-ready features** not available in community builds. - **Regular updates and security patches** backed by Percona’s expert support team. - **Professional support** and guidance for secure PostgreSQL deployments. diff --git a/contrib/pg_tde/documentation/docs/index/table-access-method.md b/contrib/pg_tde/documentation/docs/index/table-access-method.md index 1e366f1f16887..0bb282b1ccb19 100644 --- a/contrib/pg_tde/documentation/docs/index/table-access-method.md +++ b/contrib/pg_tde/documentation/docs/index/table-access-method.md @@ -53,6 +53,7 @@ Here is how you can set the new default table access method: 1. Add the access method to the `default_table_access_method` parameter: === "via the SQL statement" +AÅ: I think "the" shouldn't be here, should it? It's also weird that it's called a "statement" here but "command" below. Maybe "via ALTER SYSTEM" is a better heading? Use the `ALTER SYSTEM` command. This requires superuser or ALTER SYSTEM privileges. diff --git a/contrib/pg_tde/documentation/docs/index/tde-limitations.md b/contrib/pg_tde/documentation/docs/index/tde-limitations.md index cc10f80519b22..f6f9bc2d83614 100644 --- a/contrib/pg_tde/documentation/docs/index/tde-limitations.md +++ b/contrib/pg_tde/documentation/docs/index/tde-limitations.md @@ -1,5 +1,7 @@ # Limitations of pg_tde +AÅ: We should mention that WAL encryption isn't yet ready for use rather than mentioning pg_rewind specifically. + * Keys in the local keyfile are stored unencrypted. For better security we recommend using the Key management storage. * System tables are currently not encrypted. This means that statistics data and database metadata are currently not encrypted. diff --git a/contrib/pg_tde/documentation/docs/setup.md b/contrib/pg_tde/documentation/docs/setup.md index a83546a83f095..61a5a1f7cf735 100644 --- a/contrib/pg_tde/documentation/docs/setup.md +++ b/contrib/pg_tde/documentation/docs/setup.md @@ -12,6 +12,7 @@ The `pg_tde` extension requires additional shared memory. You need to configure You can configure the `shared_preload_libraries` parameter in two ways: * Add the following line to the `shared_preload_libraries` file: +AÅ: Which file? ```bash shared_preload_libraries = 'pg_tde' diff --git a/contrib/pg_tde/documentation/docs/test.md b/contrib/pg_tde/documentation/docs/test.md index c0cf972cc0b4a..4a48dfe9aea5e 100644 --- a/contrib/pg_tde/documentation/docs/test.md +++ b/contrib/pg_tde/documentation/docs/test.md @@ -30,6 +30,7 @@ After enabling the `pg_tde` extension for a database, you can begin encrypting d ``` The function returns `t` if the table is encrypted and `f` - if not. +AÅ: The function returns `true` or `false`. the psql client specifically renders them as `t` or `f` however. 3. (Optional) Rotate the principal key. diff --git a/contrib/pg_tde/documentation/docs/variables.md b/contrib/pg_tde/documentation/docs/variables.md index 9947eacc385fe..f977f4e2ea8c9 100644 --- a/contrib/pg_tde/documentation/docs/variables.md +++ b/contrib/pg_tde/documentation/docs/variables.md @@ -3,6 +3,7 @@ The `pg_tde` extension provides GUC variables to configure the behaviour of the extension: ## pg_tde.wal_encrypt +AÅ: Note that this shouldn't be enabled for production systems **Type** - boolean
**Default** - off diff --git a/contrib/pg_tde/documentation/docs/yum.md b/contrib/pg_tde/documentation/docs/yum.md index d87f81e521cbc..b8659e8234f12 100644 --- a/contrib/pg_tde/documentation/docs/yum.md +++ b/contrib/pg_tde/documentation/docs/yum.md @@ -9,6 +9,7 @@ Make sure you check the [list of supported platforms](install.md#__tabbed_1_1) b The `pg_tde` uses memory locks (mlocks) to keep internal encryption keys in RAM, both for WAL and for user data. A memory lock (`mlock`) is a system call to lock a specified memory range in RAM for a process. The maximum amount of memory that can be locked differs between systems. You can check the current setting with this command: +AÅ: This isn't necessarily true. Maybe OpenSSL does this under the hood? We don't do it in our code. ```bash ulimit -a