On Tue, 3 Mar 2020 at 08:49, Cary Huang <cary.hu...@highgo.ca> wrote:
>
> Hi Masahiko
> Please see below my comments regarding kms_v4.patch that you have shared
> earlier.
Thank you for reviewing this patch!
>
> (1)
> There is a discrepancy between the documentation and the actual function
> definition. For example in func.sgml, it states pg_wrap_key takes argument in
> text data type but in pg_proc.dat and kmgr.c, the function is defined to take
> argument in bytea data type.
>
> ===> doc/src/sgml/func.sgml
> + <entry>
> + <indexterm>
> + <primary>pg_wrap_key</primary>
> + </indexterm>
> + <literal><function>pg_wrap_key(<parameter>data</parameter>
> <type>text</type>)</function></literal>
> + </entry>
> + <entry>
> + <type>bytea</type>
> + </entry>
>
> ===> src/include/catalog/pg_proc.dat
> +{ oid => '8201', descr => 'wrap the given secret',
> + proname => 'pg_wrap_key',
> + provolatile => 'v', prorettype => 'bytea',
> + proargtypes => 'bytea', prosrc => 'pg_wrap_key' },
>
> ===> src/backend/crypto/kmgr.c
> +Datum
> +pg_wrap_key(PG_FUNCTION_ARGS)
> +{
> + bytea *data = PG_GETARG_BYTEA_PP(0);
> + bytea *res;
> + int datalen;
> + int reslen;
> + int len;
Fixed.
> +
>
> (2)
> I think the documentation needs to make clear the difference between a key
> and a user secret. Some parts of it are trying to mix the 2 terms together
> when they shouldn't. To my understanding, a key is expressed as binary data
> that is actually used in the encryption and decryption operations. A user
> secret, on the other hand, is more like a passphrase, expressed as string,
> that is used to derive a encryption key for subsequent encryption operations.
>
> If I just look at the function names "pg_wrap_key" and "pg_unwrap_key", I
> immediately feel that these functions are used to encapsulate and uncover
> cryptographic key materials. The input and output to these 2 functions should
> all be key materials expressed in bytea data type. In previous email
> discussion, there was only one key material in discussion, called the master
> key (generated during initdb and stored in cluster), and this somehow
> automatically makes people (including myself) associate pg_wrap_key and
> pg_unwrap_key to be used on this master key and raise a bunch of security
> concerns around it.
>
> Having read the documentation provided by the patch describing pg_wrap_key
> and pg_unwrap_key, they seem to serve another purpose. It states that
> pg_wrap_key is used to encrypt a user-supplied secret (text) with the master
> key and produce a wrapped secret while pg_unwrap_key does the opposite, so we
> can prevent user from having to enter the secret in plaintext when using
> pgcrypto functions.
>
> This use case is ok but user secret is not really a cryptographic key
> material is it? It is more similar to a secret passphrase expressed in text
> and pg_wrap_key is merely used to turn the passphrase into a wrapped
> passphrase expressed in bytea.
>
> If I give pg_wrap_key with a real key material expressed in bytea, I will not
> be able to unwrap it properly:
>
> Select pg_unwrap_key
> (pg_wrap_key(E'\\x2b073713476f9f0e761e45c64be8175424d2742e7d53df90b6416f1d84168e8a')
> );
>
> pg_unwrap_key
> ----------------------------------------------
> +\x077\x13Go\x0Ev\x1EEK\x17T$t.}SߐAo\x1D\x16
> (1 row)
>
> Maybe we should rename these SQL functions like this to prevent confusion.
> => pg_wrap_secret (takes a text, returns a bytea)
> => pg_unwrap_secret(takes a bytea, returns a text)
Agreed to change argument types. User secret will be normally text
password as we do with pgcrypto. So probably these functions can cover
most cases. I changed the function name to pg_wrap and pg_unwrap
because these functions generically wrap and unwrap the given data.
>
> if there is a use case for users to encapsulate key materials then we can
> define 2 more wrap functions for these, if there is no use case, don't bother:
> => pg_wrap_key (takes a bytea, returns a bytea)
> => pg_unwrap_key (takes a bytea, returns a bytea)
+1. Like pgcrypto has both pgp_sym_encrypt_bytea and pgp_sym_encrypt,
maybe we can have such functions.
>
> (3)
> I would rephrase "chapter 32: Encryption Key Management Part III. Server
> Administration" documentation like this:
>
> =====================
> PostgreSQL supports Encryption Key Management System, which is enabled when
> PostgreSQL is built with --with-openssl option and cluster_passphrase_command
> is specified during initdb process. The user-provided
> cluster_passphrase_command in postgresql.conf and the
> cluster_passphrase_command specified during initdb process must match,
> otherwise, the database cluster will not start up.
>
> The user-provided cluster passphrase is derived into a Key Encryption Key
> (KEK), which is used to encapsulate the Master Encryption Key generated
> during the initdb process. The encapsulated Master Encryption Key is stored
> inside the database cluster.
>
> Encryption Key Management System provides several functions to allow users to
> use the master encryption key to wrap and unwrap their own encryption secrets
> during encryption and decryption operations. This feature allows users to
> encrypt and decrypt data without knowing the actual key.
> =====================
>
> (4)
> I would rephrase "chapter 32.2: Wrap and Unwrap user secret" documentation
> like this: Note that I rephrase based on (2) and uses pg_(un)wrap_secret
> instead.
>
> =====================
> Encryption key management System provides several functions described in
> Table 9.97, to wrap and unwrap user secrets with the Master Encryption Key,
> which is uniquely and securely stored inside the database cluster.
>
> These functions allow user to encrypt and decrypt user data without having to
> provide user encryption secret in plain text. One possible use case is to use
> encryption key management together with pgcrypto. User wraps the user
> encryption secret with pg_wrap_secret() and passes the wrapped encryption
> secret to the pgcrypto encryption functions. The wrapped secret can be stored
> in the application server or somewhere secured and should be obtained
> promptly for cryptographic operations with pgcrypto.
> [same examples follow after...]
> =====================
Thank you for suggesting the updated sentences. I've updated the docs
based on your suggestions.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
configure | 2 +-
configure.in | 2 +-
doc/src/sgml/config.sgml | 49 ++-
doc/src/sgml/filelist.sgml | 1 +
doc/src/sgml/func.sgml | 60 ++++
doc/src/sgml/key-management.sgml | 142 +++++++++
doc/src/sgml/postgres.sgml | 1 +
doc/src/sgml/ref/initdb.sgml | 19 ++
src/backend/Makefile | 2 +-
src/backend/access/transam/xlog.c | 31 ++
src/backend/bootstrap/bootstrap.c | 7 +-
src/backend/crypto/Makefile | 17 ++
src/backend/crypto/kmgr.c | 289 ++++++++++++++++++
src/backend/postmaster/postmaster.c | 6 +
src/backend/tcop/postgres.c | 8 +
src/backend/utils/misc/guc.c | 25 ++
src/backend/utils/misc/postgresql.conf.sample | 5 +
src/bin/initdb/initdb.c | 34 ++-
src/bin/pg_checksums/pg_checksums.c | 1 +
src/bin/pg_controldata/pg_controldata.c | 3 +
src/common/Makefile | 3 +
src/common/cipher.c | 98 ++++++
src/common/cipher_openssl.c | 149 +++++++++
src/common/kmgr_utils.c | 418 ++++++++++++++++++++++++++
src/include/access/xlog.h | 3 +
src/include/catalog/pg_control.h | 7 +
src/include/catalog/pg_proc.dat | 13 +
src/include/common/cipher.h | 58 ++++
src/include/common/cipher_openssl.h | 39 +++
src/include/common/kmgr_utils.h | 84 ++++++
src/include/crypto/kmgr.h | 27 ++
src/include/pg_config.h.in | 3 +
src/include/utils/guc_tables.h | 1 +
src/test/Makefile | 2 +-
src/test/crypto/.gitignore | 2 +
src/test/crypto/Makefile | 24 ++
src/test/crypto/t/001_basic.pl | 32 ++
src/test/perl/PostgresNode.pm | 15 +-
38 files changed, 1670 insertions(+), 12 deletions(-)
