Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ooMPr-0003qB-N0 for pgsql-hackers@arkaria.postgresql.org; Fri, 28 Oct 2022 10:16:43 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ooMPq-00013H-DW for pgsql-hackers@arkaria.postgresql.org; Fri, 28 Oct 2022 10:16:42 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ooMPq-000138-3K for pgsql-hackers@lists.postgresql.org; Fri, 28 Oct 2022 10:16:42 +0000 Received: from mail1.dalibo.net ([51.159.93.128] helo=mail.dalibo.com) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ooMPo-0004QR-09 for pgsql-hackers@postgresql.org; Fri, 28 Oct 2022 10:16:41 +0000 Received: from karst (larco.ioguix.net [78.202.0.6]) by mail.dalibo.com (Postfix) with ESMTPSA id 7DD0B1F77C; Fri, 28 Oct 2022 12:16:30 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=dalibo.com; s=a; t=1666952190; bh=TO7ckp7aGs7cPnCiiD453nA6uueMwevQdZkVKmLUTOg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=qlcIMMls+3bHTIZPMh/YmycN7TiX6eSAQ1zPt5ojLqEeZtioc7CqEluUeBOB/tjlF 2zJIwRtU+KzUoEvR+ifci42lj2xKJnMylEfGiWuJPqQTDhgEtRrP9v0fKmbSrofhWZ 7FE0j+0dcFHnAWY5QsMnjtfvC2ZKk+Ul8UXLWIqA= Date: Fri, 28 Oct 2022 12:16:29 +0200 From: Jehan-Guillaume de Rorthais To: Peter Eisentraut Cc: pgsql-hackers Subject: Re: Transparent column encryption Message-ID: <20221028121629.6467d8f9@karst> In-Reply-To: References: <89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com> <48a9f2c2-4a57-27d8-7c53-16a23a01014e@enterprisedb.com> <79f08a39-a7da-5157-cef4-378fb60c18f8@enterprisedb.com> <258c5064-437e-f41e-7537-5e8c343c33cc@enterprisedb.com>

<6bd99fea-3298-854d-d37f-554151342f36@enterprisedb.com> <963aa100-7e78-3463-0645-700eaaa325f2@enterprisedb.com> <06830254-6d87-86b2-0280-bf2eee7736a5@enterprisedb.com> Organization: Dalibo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk Hi, I did a review of the documentation and usability. # Applying patch The patch applied on top of f13b2088fa2 without trouble. Notice a small warning during compilation: =20 colenccmds.c:134:27: warning: =E2=80=98encval=E2=80=99 may be used unin= itialized =20 A simple fix could be: =20 +++ b/src/backend/commands/colenccmds.c @@ -119,2 +119,3 encval =3D defGetString(encvalEl); + *encval_p =3D encval; } @@ -132,4 +133,2 *alg_p =3D alg; - if (encval_p) - *encval_p =3D encval; } # Documentation * In page "create_column_encryption_key.sgml", both encryption algorithms= for a CMK are declared as the default one: + + The encryption algorithm that was used to encrypt the key materi= al of + this column encryption key. Supported algorithms are: + + + RSAES_OAEP_SHA_1 (default) + + + RSAES_OAEP_SHA_256 (default) + + + =20 As far as I understand the code, I suppose RSAES_OAEP_SHA_1 should be t= he default.=20 I believe two information should be clearly shown to user somewhere in chapter 5.5 instead of being buried deep in documentation: =20 * =C2=ABCOPY does not support column decryption=C2=BB, currently buried i= n pg_dump page * =C2=ABWhen transparent column encryption is enabled, the client encodin= g must match the server encoding=C2=BB, currently buried in the protocol descr= iption page. * In the libpq doc of PQexecPrepared2, "paramForceColumnEncryptions" might deserve a little more detail about the array format, like =C2=AB0 means= "don't enforce" and anything else enforce the encryption is enabled on this column=C2=BB. By the way, maybe this array could be an array of boolean= ?=20 * In chapter 55.2.5 (protocol-flow) is stated: =C2=ABwhen column encrypti= on is used, the plaintext is always in text format (not binary format).=C2=BB= . 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. # Protocol * In the ColumnEncryptionKey message, it seems the field holding the leng= th 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 addition= al field. * I wonder if encryption related fields in ParameterDescription and RowDescription could be optional somehow? The former might be quite lar= ge 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... # libpq Would it be possible to have an encryption-ready PQexecParams() equivalent of what PQprepare/describe/exec do? # psql * You already mark \d in the TODO. But having some psql command to list t= he known CEK/CMK might be useful as well. * about write queries using psql, would it be possible to use psql parameters? Eg.: =3D> \set c1 myval =3D> INSERT INTO mytable VALUES (:'c1') \gencr # Manual tests * The lookup error message is shown twice for some reason: =3D> select * from test_tce; no CMK lookup found for realm "" =20 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. * When "DROP IF EXISTS" a missing CEK or CMK, the command raise an "unrecognized object type": =3D> DROP COLUMN MASTER KEY IF EXISTS noexists; ERROR: unrecognized object type: 10 =3D> DROP COLUMN ENCRYPTION KEY IF EXISTS noexists; ERROR: unrecognized object type: 8 * 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 encry= pted? * any reason to not support altering the CMK realm? This patch is really interesting and would be a nice addition to the core. Thanks! Regards,