Re: Transparent column encryption

On 28.10.22 12:16, Jehan-Guillaume de Rorthais wrote:
> I did a review of the documentation and usability.

I have incorporated some of your feedback into the v11 patch I just posted.

> # Applying patch
>
> The patch applied on top of f13b2088fa2 without trouble. Notice a small
> warning during compilation:
>
> colenccmds.c:134:27: warning: ‘encval’ may be used uninitialized
>
> A simple fix could be:
>
> +++ b/src/backend/commands/colenccmds.c
> @@ -119,2 +119,3
> encval = defGetString(encvalEl);
> + *encval_p = encval;
> }
> @@ -132,4 +133,2
> *alg_p = alg;
> - if (encval_p)
> - *encval_p = encval;
> }

fixed

> # Documentation
>
> * In page "create_column_encryption_key.sgml", both encryption algorithms for
> a CMK are declared as the default one:
>
> + <para>
> + The encryption algorithm that was used to encrypt the key material of
> + this column encryption key. Supported algorithms are:
> + <itemizedlist>
> + <listitem>
> + <para><literal>RSAES_OAEP_SHA_1</literal> (default)</para>
> + </listitem>
> + <listitem>
> + <para><literal>RSAES_OAEP_SHA_256</literal> (default)</para>
> + </listitem>
> + </itemizedlist>
> + </para>
>
> As far as I understand the code, I suppose RSAES_OAEP_SHA_1 should be the
> default.

fixed

> I believe two information should be clearly shown to user somewhere in
> chapter 5.5 instead of being buried deep in documentation:
>
> * «COPY does not support column decryption», currently buried in pg_dump page
> * «When transparent column encryption is enabled, the client encoding must
> match the server encoding», currently buried in the protocol description
> page.
>
> * In the libpq doc of PQexecPrepared2, "paramForceColumnEncryptions" might
> deserve a little more detail about the array format, like «0 means "don't
> enforce" and anything else enforce the encryption is enabled on this
> column». By the way, maybe this array could be an array of boolean?
>
> * In chapter 55.2.5 (protocol-flow) is stated: «when column encryption is
> used, the plaintext is always in text format (not binary format).». Does it
> means parameter "resultFormat" in "PQexecPrepared2" should always be 0? If
> yes, is it worth keeping this argument? Moreover, this format constraint
> should probably be explained in the libpq page as well.

I will keep these suggestions around. Some of these things will
probably change again, so I'll make sure to update the documentation
when I touch it again.

> # Protocol
>
> * In the ColumnEncryptionKey message, it seems the field holding the length
> key material is redundant with the message length itself, as all other
> fields have a known size. The key material length is the message length -
> (4+4+4+2). For comparison, the AuthenticationSASLContinue message has a
> variable data size but rely only on the message length without additional
> field.

I find that weird, though. An explicit length seems better. Things
like AuthenticationSASLContinue only work if they have exactly one
variable-length data item.

> * I wonder if encryption related fields in ParameterDescription and
> RowDescription could be optional somehow? The former might be quite large
> when using a lot of parameters (like, imagine a large and ugly
> "IN($1...$N)"). On the other hand, these messages are not sent in high
> frequency anyway...

They are only used if you turn on the column_encryption protocol option.
Or did you mean make them optional even then?

> # libpq
>
> Would it be possible to have an encryption-ready PQexecParams() equivalent
> of what PQprepare/describe/exec do?

I plan to do that.

> # psql
>
> * You already mark \d in the TODO. But having some psql command to list the
> known CEK/CMK might be useful as well.

right

> * about write queries using psql, would it be possible to use psql
> parameters? Eg.:
>
> => \set c1 myval
> => INSERT INTO mytable VALUES (:'c1') \gencr

No, because those are resolved by psql before libpq sees them.

> # Manual tests
>
> * The lookup error message is shown twice for some reason:
>
> => select * from test_tce;
> no CMK lookup found for realm ""
>
> no CMK lookup found for realm ""
>
> It might worth adding the column name and the CMK/CEK names related to each
> error message? Last, notice the useless empty line between messages.

I'll look into that.

> * When "DROP IF EXISTS" a missing CEK or CMK, the command raise an
> "unrecognized object type":
>
> => DROP COLUMN MASTER KEY IF EXISTS noexists;
> ERROR: unrecognized object type: 10
> => DROP COLUMN ENCRYPTION KEY IF EXISTS noexists;
> ERROR: unrecognized object type: 8

fixed

> * I was wondering what "pg_typeof" should return. It currently returns
> "pg_encrypted_*". If this is supposed to be transparent from the client
> perspective, shouldn't it return "attrealtypid" when the field is encrypted?

Interesting question. Need to think about it. I'm not sure what the
purpose of pg_typeof really is. The only use I can recall is for pgTAP.

> * any reason to not support altering the CMK realm?

This could be added. I have that in my notes.