Content-Type: multipart/mixed; boundary="------------SgU1vcN3tLH0ysipbHWdqh2M"
Message-ID: <5003d222-5975-38c1-e471-888e642f23aa@timescale.com>
Date: Thu, 19 Jan 2023 12:48:03 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: Transparent column encryption
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>
 <ddb316e8-a978-ee69-1edd-0ee82ecc6d8b@enterprisedb.com>
 <edb5eb1f-5818-5521-537d-20cf7f470742@enterprisedb.com>
 <6bd99fea-3298-854d-d37f-554151342f36@enterprisedb.com>
 <963aa100-7e78-3463-0645-700eaaa325f2@enterprisedb.com>
 <06830254-6d87-86b2-0280-bf2eee7736a5@enterprisedb.com>
 <a86c8d36-e9c5-cc5b-2c50-cc563d818419@enterprisedb.com>
 <75f394fa-f539-1875-079c-c654deceed41@enterprisedb.com>
 <8bcedb49-1496-0755-bd24-1cbabf30ec84@enterprisedb.com>
 <d9dd3634-6154-b0b7-f3a9-0d86dad7b8ff@enterprisedb.com>
 <bdb6433c-2d4e-8ae3-3bda-913f0b53fbb8@enterprisedb.com>
Content-Language: en-US
From: Jacob Champion <jchampion@timescale.com>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
In-Reply-To: <bdb6433c-2d4e-8ae3-3bda-913f0b53fbb8@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/5003d222-5975-38c1-e471-888e642f23aa%40timescale.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------SgU1vcN3tLH0ysipbHWdqh2M
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 12/31/22 06:17, Peter Eisentraut wrote:
> On 21.12.22 06:46, Peter Eisentraut wrote:
>> And another update.  The main changes are that I added an 'unspecified' 
>> CMK algorithm, which indicates that the external KMS knows what it is 
>> but the database system doesn't.  This was discussed a while ago.  I 
>> also changed some details about how the "cmklookup" works in libpq. Also 
>> added more code comments and documentation and rearranged some code.

Trying to delay a review until I had "completed it" has only led to me
not reviewing, so here's a partial one. Let me know what pieces of the
implementation and/or architecture you're hoping to get more feedback on.

I like the existing "caveats" documentation, and I've attached a sample
patch with some more caveats documented, based on some of the upthread
conversation:

- text format makes fixed-length columns leak length information too
- you only get partial protection against the Evil DBA
- RSA-OAEP public key safety

(Feel free to use/remix/discard as desired.)

When writing the paragraph on RSA-OAEP I was reminded that we didn't
really dig into the asymmetric/symmetric discussion. Assuming that most
first-time users will pick the builtin CMK encryption method, do we
still want to have an asymmetric scheme implemented first instead of a
symmetric keywrap? I'm still concerned about that public key, since it
can't really be made public. (And now that "unspecified" is available, I
think an asymmetric CMK could be easily created by users that have a
niche use case, and then we wouldn't have to commit to supporting it
forever.)

For the padding caveat:

> +      There is no concern if all values are of the same length (e.g., credit
> +      card numbers).

I nodded along to this statement last year, and then this year I learned
that CCNs aren't fixed-length. So with a 16-byte block, you're probably
going to be able to figure out who has an American Express card.

The column encryption algorithm is set per-column -- but isn't it
tightly coupled to the CEK, since the key length has to match? From a
layperson perspective, using the same key to encrypt the same plaintext
under two different algorithms (if they happen to have the same key
length) seems like it might be cryptographically risky. Is there a
reason I should be encouraged to do that?

With the loss of \gencr it looks like we also lost a potential way to
force encryption from within psql. Any plans to add that for v1?

While testing, I forgot how the new option worked and connected with
`column_encryption=on` -- and then I accidentally sent unencrypted data
to the server, since `on` means "not enabled". :( The server errors out
after the damage is done, of course, but would it be okay to strictly
validate that option's values?

Are there plans to document client-side implementation requirements, to
ensure cross-client compatibility? Things like the "PG\x00\x01"
associated data are buried at the moment (or else I've missed them in
the docs). If you're holding off until the feature is more finalized,
that's fine too.

Speaking of cross-client compatibility, I'm still disconcerted by the
ability to write the value "hello world" into an encrypted integer
column. Should clients be required to validate the text format, using
the attrealtypid?

It occurred to me when looking at the "unspecified" CMK scheme that the
CEK doesn't really have to be an encryption key at all. In that case it
can function more like a (possibly signed?) cookie for lookup, or even
be ignored altogether if you don't want to use a wrapping scheme
(similar to JWE's "direct" mode, maybe?). So now you have three ways to
look up or determine a column encryption key (CMK realm, CMK name, CEK
cookie)... is that a concept worth exploring in v1 and/or the documentation?

Thanks,
--Jacob
--------------SgU1vcN3tLH0ysipbHWdqh2M
Content-Type: text/plain; charset=UTF-8; name="caveats.diff.txt"
Content-Disposition: attachment; filename="caveats.diff.txt"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL2RvYy9zcmMvc2dtbC9kZGwuc2dtbCBiL2RvYy9zcmMvc2dtbC9kZGwu
c2dtbAppbmRleCA1NWYzM2EyZjVmLi4wNmUxYzA3N2Q1IDEwMDY0NAotLS0gYS9kb2Mvc3Jj
L3NnbWwvZGRsLnNnbWwKKysrIGIvZG9jL3NyYy9zZ21sL2RkbC5zZ21sCkBAIC0xNTg4LDcg
KzE1ODgsMzMgQEAgZXhwb3J0IFBHQ01LTE9PS1VQCiAgICAgY2FyZCBudW1iZXJzKS4gIEJ1
dCBpZiB0aGVyZSBhcmUgc2lnbmZpY2FudCBsZW5ndGggZGlmZmVyZW5jZXMgYmV0d2Vlbgog
ICAgIHZhbGlkIHZhbHVlcyBhbmQgdGhhdCBsZW5ndGggaW5mb3JtYXRpb24gaXMgc2VjdXJp
dHktc2Vuc2l0aXZlLCB0aGVuCiAgICAgYXBwbGljYXRpb24tc3BlY2lmaWMgd29ya2Fyb3Vu
ZHMgc3VjaCBhcyBwYWRkaW5nIHdvdWxkIG5lZWQgdG8gYmUgYXBwbGllZC4KLSAgICBIb3cg
dG8gZG8gdGhhdCBzZWN1cmVseSBpcyBiZXlvbmQgdGhlIHNjb3BlIG9mIHRoaXMgbWFudWFs
LgorICAgIEhvdyB0byBkbyB0aGF0IHNlY3VyZWx5IGlzIGJleW9uZCB0aGUgc2NvcGUgb2Yg
dGhpcyBtYW51YWwuICBOb3RlIHRoYXQKKyAgICBjb2x1bW4gZW5jcnlwdGlvbiBpcyBhcHBs
aWVkIHRvIHRoZSB0ZXh0IHJlcHJlc2VudGF0aW9uIG9mIHRoZSBzdG9yZWQgdmFsdWUsCisg
ICAgc28gbGVuZ3RoIGRpZmZlcmVuY2VzIGNhbiBiZSBsZWFrZWQgZXZlbiBmb3IgZml4ZWQt
bGVuZ3RoIGNvbHVtbiB0eXBlcyAoZS5nLgorICAgIDxsaXRlcmFsPmJpZ2ludDwvbGl0ZXJh
bD4sIHdob3NlIGxhcmdlc3QgZGVjaW1hbCByZXByZXNlbnRhdGlvbiBpcyBsb25nZXIKKyAg
ICB0aGFuIDE2IGJ5dGVzKS4KKyAgIDwvcGFyYT4KKworICAgPHBhcmE+CisgICAgQ29sdW1u
IGVuY3J5cHRpb24gcHJvdmlkZXMgb25seSBwYXJ0aWFsIHByb3RlY3Rpb24gYWdhaW5zdCBh
IG1hbGljaW91cworICAgIHVzZXIgd2l0aCB3cml0ZSBhY2Nlc3MgdG8gdGhlIHRhYmxlLiAg
T25jZSBlbmNyeXB0ZWQsIGFueSBtb2RpZmljYXRpb25zIHRvIGEKKyAgICBzdG9yZWQgdmFs
dWUgb24gdGhlIHNlcnZlciBzaWRlIHdpbGwgY2F1c2UgYSBkZWNyeXB0aW9uIGZhaWx1cmUg
b24gdGhlCisgICAgY2xpZW50LiAgSG93ZXZlciwgYSB1c2VyIHdpdGggd3JpdGUgYWNjZXNz
IG1heSBzdGlsbCBmcmVlbHkgc3dhcCBlbmNyeXB0ZWQKKyAgICB2YWx1ZXMgYmV0d2VlbiBy
b3dzIG9yIGNvbHVtbnMgKG9yIGV2ZW4gc2VwYXJhdGUgZGF0YWJhc2UgY2x1c3RlcnMpIGFz
IGxvbmcKKyAgICBhcyB0aGV5IHdlcmUgZW5jcnlwdGVkIHdpdGggdGhlIHNhbWUga2V5LiAg
QXR0YWNrZXJzIG1heSBhbHNvIHJlbW92ZSB2YWx1ZXMKKyAgICBieSByZXBsYWNpbmcgdGhl
bSB3aXRoIG51bGxzLCBhbmQgdXNlcnMgd2l0aCBvd25lcnNoaXAgb3ZlciB0aGUgdGFibGUg
c2NoZW1hCisgICAgbWF5IHJlcGxhY2UgZW5jcnlwdGlvbiBrZXlzIG9yIHN0cmlwIGVuY3J5
cHRpb24gZnJvbSB0aGUgY29sdW1ucyBlbnRpcmVseS4KKyAgICBBbGwgb2YgdGhpcyBpcyB0
byBzYXk6IHByb3BlciBhY2Nlc3MgY29udHJvbCBpcyBzdGlsbCBvZiB2aXRhbCBpbXBvcnRh
bmNlCisgICAgd2hlbiB1c2luZyB0aGlzIGZlYXR1cmUuCisgICA8L3BhcmE+CisKKyAgIDxw
YXJhPgorICAgIFdoZW4gdXNpbmcgdGhlIFJTQS1PQUVQIENFSyBlbmNyeXB0aW9uIG1ldGhv
ZHMsIHRoZSAicHVibGljIiBoYWxmIG9mIHRoZSBDTUsKKyAgICBtYXkgYmUgdXNlZCB0byBy
ZXBsYWNlIGV4aXN0aW5nIGNvbHVtbiBlbmNyeXB0aW9uIGtleXMgd2l0aCBrZXlzIG9mIGFu
CisgICAgYXR0YWNrZXIncyBjaG9vc2luZywgY29tcHJvbWlzaW5nIGNvbmZpZGVudGlhbGl0
eSBhbmQgYXV0aGVudGljaXR5IGZvcgorICAgIHZhbHVlcyBlbmNyeXB0ZWQgdW5kZXIgdGhh
dCBDTUsuICBGb3IgdGhpcyByZWFzb24sIGl0J3MgaW1wb3J0YW50IHRvIGtlZXAKKyAgICBi
b3RoIHRoZSBwcml2YXRlIDxlbXBoYXNpcz5hbmQ8L2VtcGhhc2lzPiBwdWJsaWMgaGFsdmVz
IG9mIHRoZSBDTUsga2V5cGFpcgorICAgIGNvbmZpZGVudGlhbC4KICAgIDwvcGFyYT4KIAog
ICAgPG5vdGU+Cg==

--------------SgU1vcN3tLH0ysipbHWdqh2M--