Re: Transparent column encryption

On 10.04.24 16:14, Jelte Fennema-Nio wrote:
>> (The CEK can't be rotated easily, since
>> that would require reading out all the data from a table/column and
>> reencrypting it. We could/should add some custom tooling for that,
>> but it wouldn't be a routine operation.)
>
> This seems like something that requires some more thought because CEK
> rotation seems just as important as CMK rotation (often both would be
> compromised at the same time).

Hopefully, the reason for key rotation is mainly that policies require
key rotation, not that keys get compromised all the time. That's the
reason for having this two-tier key system in the first place. This
seems pretty standard to me. For example, I can change the password on
my laptop's file system encryption, which somehow wraps a lower-level
key, but I can't reencrypt the actual file system in place.

> + The plaintext inside
> + the ciphertext is always in text format, but this is invisible to the
> + protocol.
>
> It seems like it would be useful to have a way of storing the
> plaintext in binary form too. I'm not saying this should be part of
> the initial version, but it would be good to keep that in mind with
> the design.

Two problems here: One, for deterministic encryption, everyone needs to
agree on the representation, otherwise equality comparisons won't work.
Two, if you give clients the option of storing text or binary, then
clients also get back a mix of text or binary, and it will be a mess.
Just giving the option of storing the payload in binary wouldn't be that
hard, but it's not clear what you can sensibly do with that in the end.

> + The session-specific identifier of the key.
>
> Is it necessary for this identifier to be session-specific? Why not
> use a global identifier like an oid? Anything session specific makes
> the job of transaction poolers quite a bit harder. If this identifier
> would be global, then the message can be forwarded as is to the client
> instead of re-mapping identifiers between clients and servers (like is
> needed for prepared statements).

The point was just to avoid saying specifically that the OID will be
sent, because then that would tie the catalog representation to the
protocol, which seems unnecessary. Maybe we can reword that somehow.

In terms of connection pooling, this feature as it is conceived right
now would only work in session pooling anyway. Even if the identifiers
somehow were global (but OIDs can also change and are not guaranteed
unique forever), the state of which keys have already been sent is
session state.

> + Additional algorithms may be added to this protocol specification without a
> + change in the protocol version number.
>
> What's the reason for not requiring a version bump for this?

This is kind of like SASL or TLS can add new methods dynamically without
requiring a new version. I mean, as we are learning, making new
protocol versions is kind of hard, so the point was to avoid it.

> + If the protocol extension <literal>_pq_.column_encryption</literal> is
> + enabled (see <xref linkend="protocol-flow-column-encryption"/>), then
> + there is also the following for each parameter:
>
> It seems a little bit wasteful to include these for all columns, even
> the ones that don't require encryption. How about only adding these
> fields when format code is 0x11

I guess you could do that, but wouldn't that making the decoding of
these messages much more complicated? You would first have to read the
"short" variant, decode the format, and then decide to read the rest.
Seems weird.

> Finally, I'm trying to figure out if this really needs to be a
> protocol extension or if a protocol version bump would work as well
> without introducing a lot of work for clients/poolers that don't care
> about it (possibly with some modifications to the proposed protocol
> changes).

That's not something this patch cares about, but the philosophical
discussions in the other thread on protocol versioning etc. appear to
lean toward protocol extension.

> What makes this a bit difficult for me is that there's not
> much written in the documentation on what is supposed to happen for
> encrypted columns when the protocol extension is not enabled. Is the
> content just returned/written like it would be with a bytea?

Yes, that's what would happen, and that's the intention, so that for
example you can use pg_dump to back up encrypted columns without having
to decrypt them.

> A related question to this is that currently libpq throws an error if
> e.g. a master key realm is not defined but another one is. Is that
> really what we want? Is not having one of the realms really that
> different from not providing any realms at all?

Can you provide a more concrete example of what scenario you have a
concern about?