Thread: Transparent column encryption

I want to present my proof-of-concept patch for the transparent column
encryption feature.  (Some might also think of it as automatic
client-side encryption or similar, but I like my name.)  This feature
enables the {automatic,transparent} encryption and decryption of
particular columns in the client.  The data for those columns then
only ever appears in ciphertext on the server, so it is protected from
the "prying eyes" of DBAs, sysadmins, cloud operators, etc.  The
canonical use case for this feature is storing credit card numbers
encrypted, in accordance with PCI DSS, as well as similar situations
involving social security numbers etc.  Of course, you can't do any
computations with encrypted values on the server, but for these use
cases, that is not necessary.  This feature does support deterministic
encryption as an alternative to the default randomized encryption, so
in that mode you can do equality lookups, at the cost of some
security.

This functionality also exists in other SQL database products, so the
overall concepts weren't invented by me by any means.

Also, this feature has nothing to do with the on-disk encryption
feature being contemplated in parallel.  Both can exist independently.

The attached patch has all the necessary pieces in place to make this
work, so you can have an idea how the overall system works.  It
contains some documentation and tests to help illustrate the
functionality.  But it's missing the remaining 90% of the work,
including additional DDL support, error handling, robust memory
management, protocol versioning, forward and backward compatibility,
pg_dump support, psql \d support, refinement of the cryptography, and
so on.  But I think obvious solutions exist to all of those things, so
it isn't that interesting to focus on them for now.

------

Now to the explanation of how it works.

You declare a column as encrypted in a CREATE TABLE statement.  The
column value is encrypted by a symmetric key called the column
encryption key (CEK).  The CEK is a catalog object.  The CEK key
material is in turn encrypted by an assymmetric key called the column
master key (CMK).  The CMK is not stored in the database but somewhere
where the client can get to it, for example in a file or in a key
management system.  When a server sends rows containing encrypted
column values to the client, it first sends the required CMK and CEK
information (new protocol messages), which the client needs to record.
Then, the client can use this information to automatically decrypt the
incoming row data and forward it in plaintext to the application.

For the CMKs, the catalog object specifies a "provider" and generic
options.  Right now, libpq has a "file" provider hardcoded, and it
takes a "filename" option.  Via some mechanism to be determined,
additional providers could be loaded and then talk to key management
systems via http or whatever.  I have left some comments in the libpq
code where the hook points for this could be.

The general idea would be for an application to have one CMK per area
of secret stuff, for example, for credit card data.  The CMK can be
rotated: each CEK can be represented multiple times in the database,
encrypted by a different CMK.  (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.)

The encryption algorithms are mostly hardcoded right now, but there
are facilities for picking algorithms and adding new ones that will be
expanded.  The CMK process uses RSA-OAEP.  The CEK process uses
AES-128-CBC right now; a more complete solution should probably
involve some HMAC thrown in.

In the server, the encrypted datums are stored in types called
encryptedr and encryptedd (for randomized and deterministic
encryption).  These are essentially cousins of bytea.  For the rest of
the database system below the protocol handling, there is nothing
special about those.  For example, encryptedr has no operators at all,
encryptedd has only an equality operator.  pg_attribute has a new
column attrealtypid that stores the original type of the data in the
column.  This is only used for providing it to clients, so that
higher-level clients can convert the decrypted value to their
appropriate data types in their environments.

Some protocol extensions are required.  These should be guarded by
some _pq_... setting, but this is not done in this patch yet.  As
mentioned above, extra messages are added for sending the CMKs and
CEKs.  In the RowDescription message, I have commandeered the format
field to add a bit that indicates that the field is encrypted.  This
could be made a separate field, and there should probably be
additional fields to indicate the algorithm and CEK name, but this was
easiest for now.  The ParameterDescription message is extended to
contain format fields for each parameter, for the same purpose.
Again, this could be done differently.

Speaking of parameter descriptions, the trickiest part of this whole
thing appears to be how to get transparently encrypted data into the
database (as opposed to reading it out).  It is required to use
protocol-level prepared statements (i.e., extended query) for this.
The client must first prepare a statement, then describe the statement
to get parameter metadata, which indicates which parameters are to be
encrypted and how.  So this will require some care by applications
that want to do this, but, well, they probably should be careful
anyway.  In libpq, the existing APIs make this difficult, because
there is no way to pass the result of a describe-statement call back
into execute-statement-with-parameters.  I added new functions that do
this, so you then essentially do

     res0 = PQdescribePrepared(conn, "");
     res = PQexecPrepared2(conn, "", 2, values, NULL, NULL, 0, res0);

(The name could obviously be improved.)  Other client APIs that have a
"statement handle" concept could do this more elegantly and probably
without any API changes.

Another challenge is that the parse analysis must check which
underlying column a parameter corresponds to.  This is similar to
resorigtbl and resorigcol in the opposite direction.  The current
implementation of this works for the test cases, but I know it has
some problems, so I'll continue working in this.  This functionality
is in principle available to all prepared-statement variants, not only
protocol-level.  So you can see in the tests that I expanded the
pg_prepared_statements view to show this information as well, which
also provides an easy way to test and debug this functionality
independent of column encryption.

And also, psql doesn't use prepared statements, so writing into
encrypted columns currently doesn't work at all via psql.  (Reading
works no problem.)  All the test code currently uses custom libpq C
programs.  We should think about a way to enable prepared statements
in psql, perhaps something like

INSERT INTO t1 VALUES ($1, $2) \gg 'val1' 'val2'

(\gexec and \gx are already taken.)

------

This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
on the direction.  As I mentioned above, a lot of the remaining work
is arguably mostly straightforward.  Some closer examination of the
issues surrounding the libpq API changes and psql would be useful.
Perhaps there are other projects where that kind of functionality
would also be useful.

On Fri, Dec 3, 2021 at 4:32 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> But it's missing the remaining 90% of the work,
> including additional DDL support, error handling, robust memory
> management, protocol versioning, forward and backward compatibility,
> pg_dump support, psql \d support, refinement of the cryptography, and
> so on.  But I think obvious solutions exist to all of those things, so
> it isn't that interesting to focus on them for now.

Right, we wouldn't want to get bogged down at this stage in little
details like, uh, everything.

> Some protocol extensions are required.  These should be guarded by
> some _pq_... setting, but this is not done in this patch yet.  As
> mentioned above, extra messages are added for sending the CMKs and
> CEKs.  In the RowDescription message, I have commandeered the format
> field to add a bit that indicates that the field is encrypted.  This
> could be made a separate field, and there should probably be
> additional fields to indicate the algorithm and CEK name, but this was
> easiest for now.  The ParameterDescription message is extended to
> contain format fields for each parameter, for the same purpose.
> Again, this could be done differently.

I think this is reasonable. I would choose to use an additional bit in
the format field as opposed to a separate field. It is worth
considering whether it makes more sense to extend the existing
ParameterDescription message conditionally on some protocol-level
option, or whether we should instead, say, add ParameterDescription2
or the moral equivalent. As I see it, the latter feels conceptually
simpler, but on the other hand, our wire protocol supposes that we
will never run out of 1-byte codes for messages, so perhaps some
prudence is needed.

> Speaking of parameter descriptions, the trickiest part of this whole
> thing appears to be how to get transparently encrypted data into the
> database (as opposed to reading it out).  It is required to use
> protocol-level prepared statements (i.e., extended query) for this.

Why? If the client knows the CEK, can't the client choose to send
unprepared insert or update statements with pre-encrypted blobs? That
might be a bad idea from a security perspective because the encrypted
blob might then got logged, but we sometimes log parameters, too.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, 2021-12-03 at 22:32 +0100, Peter Eisentraut wrote:
> This feature does support deterministic
> encryption as an alternative to the default randomized encryption, so
> in that mode you can do equality lookups, at the cost of some
> security.

> +                if (enc_det)
> +                    memset(iv, ivlen, 0);

I think reusing a zero IV will potentially leak more information than
just equality, depending on the cipher in use. You may be interested in
synthetic IVs and nonce-misuse resistance (e.g. [1]), since they seem
like they would match this use case exactly. (But I'm not a
cryptographer.)

> The encryption algorithms are mostly hardcoded right now, but there
> are facilities for picking algorithms and adding new ones that will be
> expanded.  The CMK process uses RSA-OAEP.  The CEK process uses
> AES-128-CBC right now; a more complete solution should probably
> involve some HMAC thrown in.

Have you given any thought to AEAD? As a client I'd like to be able to
tie an encrypted value to other column (or external) data. For example,
AEAD could be used to prevent a DBA from copying the (encrypted) value
of my credit card column into their account's row to use it.

> This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
> on the direction.

What kinds of attacks are you hoping to prevent (and not prevent)?

--Jacob

[1] https://datatracker.ietf.org/doc/html/rfc8452

On 06.12.21 19:28, Robert Haas wrote:
>> Speaking of parameter descriptions, the trickiest part of this whole
>> thing appears to be how to get transparently encrypted data into the
>> database (as opposed to reading it out).  It is required to use
>> protocol-level prepared statements (i.e., extended query) for this.
> Why? If the client knows the CEK, can't the client choose to send
> unprepared insert or update statements with pre-encrypted blobs? That
> might be a bad idea from a security perspective because the encrypted
> blob might then got logged, but we sometimes log parameters, too.

The client can send something like

PQexec(conn, "INSERT INTO tbl VALUES ('ENCBLOB', 'ENCBLOB')");

and it will work.  (See the included test suite where 'ENCBLOB' is 
actually computed by pgcrypto.)  But that is not transparent encryption. 
  The client wants to send "INSERT INTO tbl VALUES ('val1', 'val2')" and 
have libpq take care of encrypting 'val1' and 'val2' before hitting the 
wire.  For that you need to use the prepared statement API so that the 
values are available separately from the statement.  And furthermore the 
client needs to know what columns the insert statements is writing to, 
so that it can get the CEK for that column.  That's what it needs the 
parameter description for.

As alluded to, workarounds exist or might be made available to do part 
of that work yourself, but that shouldn't be the normal way of using it.

On 06.12.21 21:44, Jacob Champion wrote:
> I think reusing a zero IV will potentially leak more information than
> just equality, depending on the cipher in use. You may be interested in
> synthetic IVs and nonce-misuse resistance (e.g. [1]), since they seem
> like they would match this use case exactly. (But I'm not a
> cryptographer.)

I'm aware of this and plan to make use of SIV.  The current 
implementation is just an example.

> Have you given any thought to AEAD? As a client I'd like to be able to
> tie an encrypted value to other column (or external) data. For example,
> AEAD could be used to prevent a DBA from copying the (encrypted) value
> of my credit card column into their account's row to use it.

I don't know how that is supposed to work.  When the value is encrypted 
for insertion, the client may know things like table name or column 
name, so it can tie it to those.  But it doesn't know what row it will 
go in, so you can't prevent the value from being copied into another 
row.  You would need some permanent logical row ID for this, I think. 
For this scenario, the deterministic encryption mode is perhaps not the 
right one.

>> This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
>> on the direction.
> 
> What kinds of attacks are you hoping to prevent (and not prevent)?

The point is to prevent admins from getting at plaintext data.  The 
scenario you show is an interesting one but I think it's not meant to be 
addressed by this.  If admins can alter the database to their advantage, 
they could perhaps increase their account balance, create discount 
codes, etc. also.

If this is a problem, then perhaps a better approach would be to store 
parts of the data in a separate database with separate admins.

On Tue, 2021-12-07 at 16:39 +0100, Peter Eisentraut wrote:
> On 06.12.21 21:44, Jacob Champion wrote:
> > I think reusing a zero IV will potentially leak more information than
> > just equality, depending on the cipher in use. You may be interested in
> > synthetic IVs and nonce-misuse resistance (e.g. [1]), since they seem
> > like they would match this use case exactly. (But I'm not a
> > cryptographer.)
> 
> I'm aware of this and plan to make use of SIV.  The current 
> implementation is just an example.

Sounds good.

> > Have you given any thought to AEAD? As a client I'd like to be able to
> > tie an encrypted value to other column (or external) data. For example,
> > AEAD could be used to prevent a DBA from copying the (encrypted) value
> > of my credit card column into their account's row to use it.
> 
> I don't know how that is supposed to work.  When the value is encrypted 
> for insertion, the client may know things like table name or column 
> name, so it can tie it to those.  But it doesn't know what row it will 
> go in, so you can't prevent the value from being copied into another 
> row.  You would need some permanent logical row ID for this, I think.

Sorry, my description was confusing. There's nothing preventing the DBA
from copying the value inside the database, but AEAD can make it so
that the copied value isn't useful to the DBA.

Sample case. Say I have a webapp backed by Postgres, which stores
encrypted credit card numbers. Users authenticate to the webapp which
then uses the client (which has the keys) to talk to the database.
Additionally, I assume that:

- the DBA can't access the client directly (because if they can, then
they can unencrypt the victim's info using the client's keys), and

- the DBA can't authenticate as the user/victim (because if they can,
they can just log in themselves and have the data). The webapp might
for example use federated authn with a separate provider, using an
email address as an identifier.

Now, if the client encrypts a user's credit card number using their
email address as associated data, then it doesn't matter if the DBA
copies that user's encrypted card over to their own account. The DBA
can't log in as the victim, so the client will fail to authenticate the
value because its associated data won't match.

> > > This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
> > > on the direction.
> > 
> > What kinds of attacks are you hoping to prevent (and not prevent)?
> 
> The point is to prevent admins from getting at plaintext data.  The 
> scenario you show is an interesting one but I think it's not meant to be 
> addressed by this.  If admins can alter the database to their advantage, 
> they could perhaps increase their account balance, create discount 
> codes, etc. also.

Sure, but increasing account balances and discount codes don't lead to
getting at plaintext data, right? Whereas stealing someone else's
encrypted value seems like it would be covered under your threat model,
since it lets you trick a real-world client into decrypting it for you.

Other avenues of attack might depend on how you choose to add HMAC to
the current choice of AES-CBC. My understanding of AE ciphers (with or
without associated data) is that you don't have to design that
yourself, which is nice.

--Jacob

On 12/7/21 19:02, Jacob Champion wrote:
> On Tue, 2021-12-07 at 16:39 +0100, Peter Eisentraut wrote:
>> On 06.12.21 21:44, Jacob Champion wrote:
>>> I think reusing a zero IV will potentially leak more information than
>>> just equality, depending on the cipher in use. You may be interested in
>>> synthetic IVs and nonce-misuse resistance (e.g. [1]), since they seem
>>> like they would match this use case exactly. (But I'm not a
>>> cryptographer.)
>>
>> I'm aware of this and plan to make use of SIV.  The current
>> implementation is just an example.
> 
> Sounds good.
> 
>>> Have you given any thought to AEAD? As a client I'd like to be able to
>>> tie an encrypted value to other column (or external) data. For example,
>>> AEAD could be used to prevent a DBA from copying the (encrypted) value
>>> of my credit card column into their account's row to use it.
>>
>> I don't know how that is supposed to work.  When the value is encrypted
>> for insertion, the client may know things like table name or column
>> name, so it can tie it to those.  But it doesn't know what row it will
>> go in, so you can't prevent the value from being copied into another
>> row.  You would need some permanent logical row ID for this, I think.
> 
> Sorry, my description was confusing. There's nothing preventing the DBA
> from copying the value inside the database, but AEAD can make it so
> that the copied value isn't useful to the DBA.
> 
> Sample case. Say I have a webapp backed by Postgres, which stores
> encrypted credit card numbers. Users authenticate to the webapp which
> then uses the client (which has the keys) to talk to the database.
> Additionally, I assume that:
> 
> - the DBA can't access the client directly (because if they can, then
> they can unencrypt the victim's info using the client's keys), and
> 
> - the DBA can't authenticate as the user/victim (because if they can,
> they can just log in themselves and have the data). The webapp might
> for example use federated authn with a separate provider, using an
> email address as an identifier.
> 
> Now, if the client encrypts a user's credit card number using their
> email address as associated data, then it doesn't matter if the DBA
> copies that user's encrypted card over to their own account. The DBA
> can't log in as the victim, so the client will fail to authenticate the
> value because its associated data won't match.
> 
>>>> This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
>>>> on the direction.
>>>
>>> What kinds of attacks are you hoping to prevent (and not prevent)?
>>
>> The point is to prevent admins from getting at plaintext data.  The
>> scenario you show is an interesting one but I think it's not meant to be
>> addressed by this.  If admins can alter the database to their advantage,
>> they could perhaps increase their account balance, create discount
>> codes, etc. also.
> 
> Sure, but increasing account balances and discount codes don't lead to
> getting at plaintext data, right? Whereas stealing someone else's
> encrypted value seems like it would be covered under your threat model,
> since it lets you trick a real-world client into decrypting it for you.
> 
> Other avenues of attack might depend on how you choose to add HMAC to
> the current choice of AES-CBC. My understanding of AE ciphers (with or
> without associated data) is that you don't have to design that
> yourself, which is nice.
> 

IMO it's impossible to solve this attack within TCE, because it requires 
ensuring consistency at the row level, but TCE obviously works at column 
level only.

I believe TCE can do AEAD at the column level, which protects against 
attacks that flipping bits, and similar attacks. It's just a matter of 
how the client encrypts the data.

Extending it to protect the whole row seems tricky, because the client 
may not even know the other columns, and it's not clear to me how it'd 
deal with things like updates of the other columns, hint bits, dropped 
columns, etc.

It's probably possible to get something like this (row-level AEAD) by 
encrypting enriched data, i.e. not just the card number, but {user ID, 
card number} or something like that, and verify that in the webapp. The 
problem of course is that the "user ID" is just another column in the 
table, and there's nothing preventing the DBA from modifying that too.

So I think it's pointless to try extending this to row-level AEAD.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2021-12-07 at 22:21 +0100, Tomas Vondra wrote:
> IMO it's impossible to solve this attack within TCE, because it requires 
> ensuring consistency at the row level, but TCE obviously works at column 
> level only.

I was under the impression that clients already had to be modified to
figure out how to encrypt the data? If part of that process ends up
including enforcement of encryption for a specific column set, then the
addition of AEAD data could hypothetically be part of that hand-
waviness.

Unless "transparent" means that the client completely defers to the
server on whether to encrypt or not, and silently goes along with it if
the server tells it not to encrypt? That would only protect against a
_completely_ passive DBA, like someone reading unencrypted backups,
etc. And that still has a lot of value, certainly. But it seems like
this prototype is very close to a system where the client can reliably
secure data even if the server isn't trustworthy, if that's a use case
you're interested in.

> I believe TCE can do AEAD at the column level, which protects against 
> attacks that flipping bits, and similar attacks. It's just a matter of 
> how the client encrypts the data.

Right, I think authenticated encryption ciphers (without AD) will be
important to support in practice. I think users are going to want
*some* protection against active attacks.

> Extending it to protect the whole row seems tricky, because the client 
> may not even know the other columns, and it's not clear to me how it'd 
> deal with things like updates of the other columns, hint bits, dropped 
> columns, etc.

Covering the entire row automatically probably isn't super helpful in
practice. As you mention later:

> It's probably possible to get something like this (row-level AEAD) by 
> encrypting enriched data, i.e. not just the card number, but {user ID, 
> card number} or something like that, and verify that in the webapp. The 
> problem of course is that the "user ID" is just another column in the 
> table, and there's nothing preventing the DBA from modifying that too.

Right. That's why the client has to be able to choose AD according to
the application. In my previous example, the victim's email address can
be copied by the DBA, but they wouldn't be able to authenticate as that
user and couldn't convince the client to use the plaintext on their
behalf.

--Jacob

On 12/8/21 00:26, Jacob Champion wrote:
> On Tue, 2021-12-07 at 22:21 +0100, Tomas Vondra wrote:
>> IMO it's impossible to solve this attack within TCE, because it requires 
>> ensuring consistency at the row level, but TCE obviously works at column 
>> level only.
> 
> I was under the impression that clients already had to be modified to
> figure out how to encrypt the data? If part of that process ends up
> including enforcement of encryption for a specific column set, then the
> addition of AEAD data could hypothetically be part of that hand-
> waviness.
> 

I think "transparency" here means the client just uses the regular
prepared-statement API without having to explicitly encrypt/decrypt any
data. The problem is we can't easily tie this to other columns in the
table, because the client may not even know what values are in those
columns.

Imagine you do this

  UPDATE t SET encrypted_column = $1 WHERE another_column = $2;

but you want to ensure the encrypted value belongs to a particular row
(which may or may not be identified by the another_column value). How
would the client do that? Should it fetch the value or what?

Similarly, what if the client just does

  SELECT encrypted_column FROM t;

How would it verify the values belong to the row, without having all the
data for the row (or just the required columns)?

> Unless "transparent" means that the client completely defers to the
> server on whether to encrypt or not, and silently goes along with it if
> the server tells it not to encrypt?
I think that's probably a valid concern - a "bad DBA" could alter the
table definition to not contain the "ENCRYPTED" bits, and then peek at
the plaintext values.

But it's not clear to me how exactly would the AEAD prevent this?
Wouldn't that be also specified on the server, somehow? In which case
the DBA could just tweak that too, no?

In other words, this issue seems mostly orthogonal to the AEAD, and the
right solution would be to allow the client to define which columns have
to be encrypted (in which case altering the server definition would not
be enough).

> That would only protect against a
> _completely_ passive DBA, like someone reading unencrypted backups,
> etc. And that still has a lot of value, certainly. But it seems like
> this prototype is very close to a system where the client can reliably
> secure data even if the server isn't trustworthy, if that's a use case
> you're interested in.
> 

Right. IMHO the "passive attacker" is a perfectly fine model for use
cases that would be fine with e.g. pgcrypto if there was no risk of
leaking plaintext values to logs, system catalogs, etc.

If we can improve it to provide (at least some) protection against
active attackers, that'd be a nice bonus.

>> I believe TCE can do AEAD at the column level, which protects against 
>> attacks that flipping bits, and similar attacks. It's just a matter of 
>> how the client encrypts the data.
> 
> Right, I think authenticated encryption ciphers (without AD) will be
> important to support in practice. I think users are going to want
> *some* protection against active attacks.
> 
>> Extending it to protect the whole row seems tricky, because the client 
>> may not even know the other columns, and it's not clear to me how it'd 
>> deal with things like updates of the other columns, hint bits, dropped 
>> columns, etc.
> 
> Covering the entire row automatically probably isn't super helpful in
> practice. As you mention later:
> 
>> It's probably possible to get something like this (row-level AEAD) by 
>> encrypting enriched data, i.e. not just the card number, but {user ID, 
>> card number} or something like that, and verify that in the webapp. The 
>> problem of course is that the "user ID" is just another column in the 
>> table, and there's nothing preventing the DBA from modifying that too.
> 
> Right. That's why the client has to be able to choose AD according to
> the application. In my previous example, the victim's email address can
> be copied by the DBA, but they wouldn't be able to authenticate as that
> user and couldn't convince the client to use the plaintext on their
> behalf.
> 

Well, yeah. But I'm not sure how to make that work easily, because the
client may not have the data :-(

I was thinking about using a composite data type combining the data with
the extra bits - that'd not be all that transparent as it'd require the
client to build this manually and then also cross-check it after loading
the data. So the user would be responsible for having all the data.

But doing that automatically/transparently seems hard, because how would
you deal e.g. with SELECT queries reading data through a view or CTE?

How would you declare this, either at the client or server?

Do any other databases have this capability? How do they do it?



regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, 2021-12-08 at 02:58 +0100, Tomas Vondra wrote:
> 
> On 12/8/21 00:26, Jacob Champion wrote:
> > On Tue, 2021-12-07 at 22:21 +0100, Tomas Vondra wrote:
> > > IMO it's impossible to solve this attack within TCE, because it requires 
> > > ensuring consistency at the row level, but TCE obviously works at column 
> > > level only.
> > 
> > I was under the impression that clients already had to be modified to
> > figure out how to encrypt the data? If part of that process ends up
> > including enforcement of encryption for a specific column set, then the
> > addition of AEAD data could hypothetically be part of that hand-
> > waviness.
> 
> I think "transparency" here means the client just uses the regular
> prepared-statement API without having to explicitly encrypt/decrypt any
> data. The problem is we can't easily tie this to other columns in the
> table, because the client may not even know what values are in those
> columns.

The way I originally described my request -- "I'd like to be able to
tie an encrypted value to other column (or external) data" -- was not
very clear.

With my proposed model -- where the DBA (and the server) are completely
untrusted, and the DBA needs to be prevented from using the encrypted
value -- I don't think there's a useful way for the client to use
associated data that comes from the server. The client has to know what
the AD should be beforehand, because otherwise the DBA can make it so
the server returns whatever is correct.

> Imagine you do this
> 
>   UPDATE t SET encrypted_column = $1 WHERE another_column = $2;
> 
> but you want to ensure the encrypted value belongs to a particular row
> (which may or may not be identified by the another_column value). How
> would the client do that? Should it fetch the value or what?
> 
> Similarly, what if the client just does
> 
>   SELECT encrypted_column FROM t;
> 
> How would it verify the values belong to the row, without having all the
> data for the row (or just the required columns)?

So with my (hopefully more clear) model above, it wouldn't. The client
would already have the AD, and somehow tell libpq what that data was
for the query.

The rabbit hole I led you down is one where we use the rest of the row
as AD, to try to freeze pieces of it in place. That might(?) have some
useful security properties (if the client defines its use and doesn't
defer to the server). But it's not what I intended to propose and I'd
have to think about that case some more.

In my credit card example, I'm imagining something like (forgive the
contrived syntax):

    SELECT address, :{aead(users.credit_card, 'user@example.com')}
      FROM users WHERE email = 'user@example.com';

    UPDATE users
       SET :{aead(users.credit_card, 'user@example.com')} = '1234-...'
     WHERE email = 'user@example.com';

The client explicitly links a table's column to its AD for the duration
of the query. This approach can't scale to

    SELECT credit_card FROM users;

because in this case the AD for each row is different, but I'd argue
that's ideal for this particular case. The client doesn't need to (and
probably shouldn't) grab everyone's credit card details all at once, so
there's no reason to optimize for it.

> > Unless "transparent" means that the client completely defers to the
> > server on whether to encrypt or not, and silently goes along with it if
> > the server tells it not to encrypt?
> I think that's probably a valid concern - a "bad DBA" could alter the
> table definition to not contain the "ENCRYPTED" bits, and then peek at
> the plaintext values.
> 
> But it's not clear to me how exactly would the AEAD prevent this?
> Wouldn't that be also specified on the server, somehow? In which case
> the DBA could just tweak that too, no?
>
> In other words, this issue seems mostly orthogonal to the AEAD, and the
> right solution would be to allow the client to define which columns have
> to be encrypted (in which case altering the server definition would not
> be enough).

Right, exactly. When I mentioned AEAD I had assumed that "allow the
client to define which columns have to be encrypted" was already
planned or in the works; I just misunderstood pieces of Peter's email.
It's that piece where a client would probably have to add details
around AEAD and its use.

> > That would only protect against a
> > _completely_ passive DBA, like someone reading unencrypted backups,
> > etc. And that still has a lot of value, certainly. But it seems like
> > this prototype is very close to a system where the client can reliably
> > secure data even if the server isn't trustworthy, if that's a use case
> > you're interested in.
> 
> Right. IMHO the "passive attacker" is a perfectly fine model for use
> cases that would be fine with e.g. pgcrypto if there was no risk of
> leaking plaintext values to logs, system catalogs, etc.
> 
> If we can improve it to provide (at least some) protection against
> active attackers, that'd be a nice bonus.

I agree that resistance against offline attacks is a useful step
forward (it seems to be a strict improvement over pgcrypto). I have a
feeling that end users will *expect* some protection against online
attacks too, since an evil DBA is going to be well-positioned to do
exactly that.

> > > It's probably possible to get something like this (row-level AEAD) by 
> > > encrypting enriched data, i.e. not just the card number, but {user ID, 
> > > card number} or something like that, and verify that in the webapp. The 
> > > problem of course is that the "user ID" is just another column in the 
> > > table, and there's nothing preventing the DBA from modifying that too.
> > 
> > Right. That's why the client has to be able to choose AD according to
> > the application. In my previous example, the victim's email address can
> > be copied by the DBA, but they wouldn't be able to authenticate as that
> > user and couldn't convince the client to use the plaintext on their
> > behalf.
> 
> Well, yeah. But I'm not sure how to make that work easily, because the
> client may not have the data :-(
> 
> I was thinking about using a composite data type combining the data with
> the extra bits - that'd not be all that transparent as it'd require the
> client to build this manually and then also cross-check it after loading
> the data. So the user would be responsible for having all the data.
> 
> But doing that automatically/transparently seems hard, because how would
> you deal e.g. with SELECT queries reading data through a view or CTE?
> 
> How would you declare this, either at the client or server?

I'll do some more thinking on the case you're talking about here, where
pieces of the row are transparently tied together.

> Do any other databases have this capability? How do they do it?

BigQuery advertises AEAD support. I don't think their model is the same
as ours, though; from the docs it looks like it's essentially pgcrypto,
where you tell the server to encrypt stuff for you.

--Jacob

On 12/9/21 01:12, Jacob Champion wrote:
> On Wed, 2021-12-08 at 02:58 +0100, Tomas Vondra wrote:
>>
>> On 12/8/21 00:26, Jacob Champion wrote:
>>> On Tue, 2021-12-07 at 22:21 +0100, Tomas Vondra wrote:
>>>> IMO it's impossible to solve this attack within TCE, because it requires 
>>>> ensuring consistency at the row level, but TCE obviously works at column 
>>>> level only.
>>>
>>> I was under the impression that clients already had to be modified to
>>> figure out how to encrypt the data? If part of that process ends up
>>> including enforcement of encryption for a specific column set, then the
>>> addition of AEAD data could hypothetically be part of that hand-
>>> waviness.
>>
>> I think "transparency" here means the client just uses the regular
>> prepared-statement API without having to explicitly encrypt/decrypt any
>> data. The problem is we can't easily tie this to other columns in the
>> table, because the client may not even know what values are in those
>> columns.
> 
> The way I originally described my request -- "I'd like to be able to
> tie an encrypted value to other column (or external) data" -- was not
> very clear.
> 
> With my proposed model -- where the DBA (and the server) are completely
> untrusted, and the DBA needs to be prevented from using the encrypted
> value -- I don't think there's a useful way for the client to use
> associated data that comes from the server. The client has to know what
> the AD should be beforehand, because otherwise the DBA can make it so
> the server returns whatever is correct.
> 

True. With untrusted server the additional data would have to come from
some other source. Say, an isolated auth system or so.

>> Imagine you do this
>>
>>   UPDATE t SET encrypted_column = $1 WHERE another_column = $2;
>>
>> but you want to ensure the encrypted value belongs to a particular row
>> (which may or may not be identified by the another_column value). How
>> would the client do that? Should it fetch the value or what?
>>
>> Similarly, what if the client just does
>>
>>   SELECT encrypted_column FROM t;
>>
>> How would it verify the values belong to the row, without having all the
>> data for the row (or just the required columns)?
> 
> So with my (hopefully more clear) model above, it wouldn't. The client
> would already have the AD, and somehow tell libpq what that data was
> for the query.
> 
> The rabbit hole I led you down is one where we use the rest of the row
> as AD, to try to freeze pieces of it in place. That might(?) have some
> useful security properties (if the client defines its use and doesn't
> defer to the server). But it's not what I intended to propose and I'd
> have to think about that case some more.
> 

OK

> In my credit card example, I'm imagining something like (forgive the
> contrived syntax):
> 
>     SELECT address, :{aead(users.credit_card, 'user@example.com')}
>       FROM users WHERE email = 'user@example.com';
> 
>     UPDATE users
>        SET :{aead(users.credit_card, 'user@example.com')} = '1234-...'
>      WHERE email = 'user@example.com';
> 
> The client explicitly links a table's column to its AD for the duration
> of the query. This approach can't scale to
> 
>     SELECT credit_card FROM users;
> 
> because in this case the AD for each row is different, but I'd argue
> that's ideal for this particular case. The client doesn't need to (and
> probably shouldn't) grab everyone's credit card details all at once, so
> there's no reason to optimize for it.
> 

Maybe, but it seems like a rather annoying limitation, as it restricts
the client to single-row queries (or at least it looks like that to me).
Yes, it may be fine for some use cases, but I'd bet a DBA who can modify
data can do plenty other things - swapping "old" values, which will have
the right AD, for example.

>>> Unless "transparent" means that the client completely defers to the
>>> server on whether to encrypt or not, and silently goes along with it if
>>> the server tells it not to encrypt?
>> I think that's probably a valid concern - a "bad DBA" could alter the
>> table definition to not contain the "ENCRYPTED" bits, and then peek at
>> the plaintext values.
>>
>> But it's not clear to me how exactly would the AEAD prevent this?
>> Wouldn't that be also specified on the server, somehow? In which case
>> the DBA could just tweak that too, no?
>>
>> In other words, this issue seems mostly orthogonal to the AEAD, and the
>> right solution would be to allow the client to define which columns have
>> to be encrypted (in which case altering the server definition would not
>> be enough).
> 
> Right, exactly. When I mentioned AEAD I had assumed that "allow the
> client to define which columns have to be encrypted" was already
> planned or in the works; I just misunderstood pieces of Peter's email.
> It's that piece where a client would probably have to add details
> around AEAD and its use.
> 
>>> That would only protect against a
>>> _completely_ passive DBA, like someone reading unencrypted backups,
>>> etc. And that still has a lot of value, certainly. But it seems like
>>> this prototype is very close to a system where the client can reliably
>>> secure data even if the server isn't trustworthy, if that's a use case
>>> you're interested in.
>>
>> Right. IMHO the "passive attacker" is a perfectly fine model for use
>> cases that would be fine with e.g. pgcrypto if there was no risk of
>> leaking plaintext values to logs, system catalogs, etc.
>>
>> If we can improve it to provide (at least some) protection against
>> active attackers, that'd be a nice bonus.
> 
> I agree that resistance against offline attacks is a useful step
> forward (it seems to be a strict improvement over pgcrypto). I have a
> feeling that end users will *expect* some protection against online
> attacks too, since an evil DBA is going to be well-positioned to do
> exactly that.
> 

Yeah.

>>>> It's probably possible to get something like this (row-level AEAD) by 
>>>> encrypting enriched data, i.e. not just the card number, but {user ID, 
>>>> card number} or something like that, and verify that in the webapp. The 
>>>> problem of course is that the "user ID" is just another column in the 
>>>> table, and there's nothing preventing the DBA from modifying that too.
>>>
>>> Right. That's why the client has to be able to choose AD according to
>>> the application. In my previous example, the victim's email address can
>>> be copied by the DBA, but they wouldn't be able to authenticate as that
>>> user and couldn't convince the client to use the plaintext on their
>>> behalf.
>>
>> Well, yeah. But I'm not sure how to make that work easily, because the
>> client may not have the data :-(
>>
>> I was thinking about using a composite data type combining the data with
>> the extra bits - that'd not be all that transparent as it'd require the
>> client to build this manually and then also cross-check it after loading
>> the data. So the user would be responsible for having all the data.
>>
>> But doing that automatically/transparently seems hard, because how would
>> you deal e.g. with SELECT queries reading data through a view or CTE?
>>
>> How would you declare this, either at the client or server?
> 
> I'll do some more thinking on the case you're talking about here, where
> pieces of the row are transparently tied together.
> 

OK. In any case, I think we shouldn't require this capability from the
get go - it's fine to get the simple version done first, which gives us
privacy / protects against passive attacker. And then sometime in the
future improve this further.

>> Do any other databases have this capability? How do they do it?
> 
> BigQuery advertises AEAD support. I don't think their model is the same
> as ours, though; from the docs it looks like it's essentially pgcrypto,
> where you tell the server to encrypt stuff for you.
> 

Pretty sure it's server-side. The docs say it's for encryption at rest,
all the examples do the encryption/decryption in SQL, etc.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> In the server, the encrypted datums are stored in types called
> encryptedr and encryptedd (for randomized and deterministic
> encryption).  These are essentially cousins of bytea.

Does that mean someone could go in with psql and select out the data
without any keys and just get a raw bytea-like representation? That
seems like a natural and useful thing to be able to do. For example to
allow dumping a table and loading it elsewhere and transferring keys
through some other channel (perhaps only as needed).

On 16.12.21 05:47, Greg Stark wrote:
>> In the server, the encrypted datums are stored in types called
>> encryptedr and encryptedd (for randomized and deterministic
>> encryption).  These are essentially cousins of bytea.
> 
> Does that mean someone could go in with psql and select out the data
> without any keys and just get a raw bytea-like representation? That
> seems like a natural and useful thing to be able to do. For example to
> allow dumping a table and loading it elsewhere and transferring keys
> through some other channel (perhaps only as needed).

Yes to all of that.

On Thu, 2021-12-09 at 11:04 +0100, Tomas Vondra wrote:
> On 12/9/21 01:12, Jacob Champion wrote:
> > 
> > The rabbit hole I led you down is one where we use the rest of the row
> > as AD, to try to freeze pieces of it in place. That might(?) have some
> > useful security properties (if the client defines its use and doesn't
> > defer to the server). But it's not what I intended to propose and I'd
> > have to think about that case some more.

So after thinking about it some more, in the case where the client is
relying on the server to return both the encrypted data and its
associated data -- and you don't trust the server -- then tying even
the entire row together doesn't help you.

I was briefly led astray by the idea that you could include a unique or
primary key column in the associated data, and then SELECT based on
that column -- but a motivated DBA could simply corrupt state so that
the row they wanted got returned regardless of the query. So the client
still has to have prior knowledge.

> > In my credit card example, I'm imagining something like (forgive the
> > contrived syntax):
> > 
> >     SELECT address, :{aead(users.credit_card, 'user@example.com')}
> >       FROM users WHERE email = 'user@example.com';
> > 
> >     UPDATE users
> >        SET :{aead(users.credit_card, 'user@example.com')} = '1234-...'
> >      WHERE email = 'user@example.com';
> > 
> > The client explicitly links a table's column to its AD for the duration
> > of the query. This approach can't scale to
> > 
> >     SELECT credit_card FROM users;
> > 
> > because in this case the AD for each row is different, but I'd argue
> > that's ideal for this particular case. The client doesn't need to (and
> > probably shouldn't) grab everyone's credit card details all at once, so
> > there's no reason to optimize for it.
> 
> Maybe, but it seems like a rather annoying limitation, as it restricts
> the client to single-row queries (or at least it looks like that to me).
> Yes, it may be fine for some use cases, but I'd bet a DBA who can modify
> data can do plenty other things - swapping "old" values, which will have
> the right AD, for example.

Resurrecting old data doesn't help the DBA read the values, right? I
view that as similar to the "increasing account balance" problem, in
that it's definitely a problem but not one we're trying to tackle here.

(And I'm not familiar with any solutions for resurrections -- other
than having data expire and tying the timestamp into the
authentication, which I think again requires AD. Revoking signed data
is one of those hard problems. Do you know a better way?)

> OK. In any case, I think we shouldn't require this capability from the
> get go - it's fine to get the simple version done first, which gives us
> privacy / protects against passive attacker. And then sometime in the
> future improve this further.

Agreed. (And I think the client should be able to enforce encryption in
the first place, before I distract you too much with other stuff.)

--Jacob

On 17.12.21 01:41, Jacob Champion wrote:
> (And I think the client should be able to enforce encryption in
> the first place, before I distract you too much with other stuff.)

Yes, this is a useful point that I have added to my notes.

Here is a new version of this patch.  See also the original description 
quoted below.  I have done a significant amount of work on this over the 
last few months.  Some important news include:

- The cryptography has been improved.  It now uses an AEAD scheme, and 
for deterministic encryption a proper SIV construction.

- The OpenSSL-specific parts have been moved to a separate file in 
libpq.  Non-OpenSSL builds compile and work (without functionality, of 
course).

- libpq handles multiple CEKs and CMKs, including changing keys on the fly.

- libpq supports a mode to force encryption of certain values.

- libpq supports a flexible configuration system for looking up CMKs, 
including support for external key management systems.

- psql has a new \gencr command that allows passing in bind parameters 
for (potential) encryption.

- There is some more pg_dump and psql support.

- The new data types for storing encrypted data have been renamed for 
clarity.

- Various changes to the protocol compared to the previous patch.

- The patch contains full documentation of the protocol changes, 
glossary entries, and more new documentation.

The major pieces that are still missing are:

- DDL support for registering keys

- Protocol versioning or feature flags

Other than that it's pretty complete in my mind.

For interested reviewers, I have organized the patch so that you can 
start reading it top to bottom: The documentation comes first, then the 
tests, then the code changes.  Even some feedback on the first or first 
two aspects would be valuable to me.

Old news follows:

On 03.12.21 22:32, Peter Eisentraut wrote:
> I want to present my proof-of-concept patch for the transparent column
> encryption feature.  (Some might also think of it as automatic
> client-side encryption or similar, but I like my name.)  This feature
> enables the {automatic,transparent} encryption and decryption of
> particular columns in the client.  The data for those columns then
> only ever appears in ciphertext on the server, so it is protected from
> the "prying eyes" of DBAs, sysadmins, cloud operators, etc.  The
> canonical use case for this feature is storing credit card numbers
> encrypted, in accordance with PCI DSS, as well as similar situations
> involving social security numbers etc.  Of course, you can't do any
> computations with encrypted values on the server, but for these use
> cases, that is not necessary.  This feature does support deterministic
> encryption as an alternative to the default randomized encryption, so
> in that mode you can do equality lookups, at the cost of some
> security.
> 
> This functionality also exists in other SQL database products, so the
> overall concepts weren't invented by me by any means.
> 
> Also, this feature has nothing to do with the on-disk encryption
> feature being contemplated in parallel.  Both can exist independently.
> 
> The attached patch has all the necessary pieces in place to make this
> work, so you can have an idea how the overall system works.  It
> contains some documentation and tests to help illustrate the
> functionality.  But it's missing the remaining 90% of the work,
> including additional DDL support, error handling, robust memory
> management, protocol versioning, forward and backward compatibility,
> pg_dump support, psql \d support, refinement of the cryptography, and
> so on.  But I think obvious solutions exist to all of those things, so
> it isn't that interesting to focus on them for now.
> 
> ------
> 
> Now to the explanation of how it works.
> 
> You declare a column as encrypted in a CREATE TABLE statement.  The
> column value is encrypted by a symmetric key called the column
> encryption key (CEK).  The CEK is a catalog object.  The CEK key
> material is in turn encrypted by an assymmetric key called the column
> master key (CMK).  The CMK is not stored in the database but somewhere
> where the client can get to it, for example in a file or in a key
> management system.  When a server sends rows containing encrypted
> column values to the client, it first sends the required CMK and CEK
> information (new protocol messages), which the client needs to record.
> Then, the client can use this information to automatically decrypt the
> incoming row data and forward it in plaintext to the application.
> 
> For the CMKs, the catalog object specifies a "provider" and generic
> options.  Right now, libpq has a "file" provider hardcoded, and it
> takes a "filename" option.  Via some mechanism to be determined,
> additional providers could be loaded and then talk to key management
> systems via http or whatever.  I have left some comments in the libpq
> code where the hook points for this could be.
> 
> The general idea would be for an application to have one CMK per area
> of secret stuff, for example, for credit card data.  The CMK can be
> rotated: each CEK can be represented multiple times in the database,
> encrypted by a different CMK.  (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.)
> 
> The encryption algorithms are mostly hardcoded right now, but there
> are facilities for picking algorithms and adding new ones that will be
> expanded.  The CMK process uses RSA-OAEP.  The CEK process uses
> AES-128-CBC right now; a more complete solution should probably
> involve some HMAC thrown in.
> 
> In the server, the encrypted datums are stored in types called
> encryptedr and encryptedd (for randomized and deterministic
> encryption).  These are essentially cousins of bytea.  For the rest of
> the database system below the protocol handling, there is nothing
> special about those.  For example, encryptedr has no operators at all,
> encryptedd has only an equality operator.  pg_attribute has a new
> column attrealtypid that stores the original type of the data in the
> column.  This is only used for providing it to clients, so that
> higher-level clients can convert the decrypted value to their
> appropriate data types in their environments.
> 
> Some protocol extensions are required.  These should be guarded by
> some _pq_... setting, but this is not done in this patch yet.  As
> mentioned above, extra messages are added for sending the CMKs and
> CEKs.  In the RowDescription message, I have commandeered the format
> field to add a bit that indicates that the field is encrypted.  This
> could be made a separate field, and there should probably be
> additional fields to indicate the algorithm and CEK name, but this was
> easiest for now.  The ParameterDescription message is extended to
> contain format fields for each parameter, for the same purpose.
> Again, this could be done differently.
> 
> Speaking of parameter descriptions, the trickiest part of this whole
> thing appears to be how to get transparently encrypted data into the
> database (as opposed to reading it out).  It is required to use
> protocol-level prepared statements (i.e., extended query) for this.
> The client must first prepare a statement, then describe the statement
> to get parameter metadata, which indicates which parameters are to be
> encrypted and how.  So this will require some care by applications
> that want to do this, but, well, they probably should be careful
> anyway.  In libpq, the existing APIs make this difficult, because
> there is no way to pass the result of a describe-statement call back
> into execute-statement-with-parameters.  I added new functions that do
> this, so you then essentially do
> 
>      res0 = PQdescribePrepared(conn, "");
>      res = PQexecPrepared2(conn, "", 2, values, NULL, NULL, 0, res0);
> 
> (The name could obviously be improved.)  Other client APIs that have a
> "statement handle" concept could do this more elegantly and probably
> without any API changes.
> 
> Another challenge is that the parse analysis must check which
> underlying column a parameter corresponds to.  This is similar to
> resorigtbl and resorigcol in the opposite direction.  The current
> implementation of this works for the test cases, but I know it has
> some problems, so I'll continue working in this.  This functionality
> is in principle available to all prepared-statement variants, not only
> protocol-level.  So you can see in the tests that I expanded the
> pg_prepared_statements view to show this information as well, which
> also provides an easy way to test and debug this functionality
> independent of column encryption.
> 
> And also, psql doesn't use prepared statements, so writing into
> encrypted columns currently doesn't work at all via psql.  (Reading
> works no problem.)  All the test code currently uses custom libpq C
> programs.  We should think about a way to enable prepared statements
> in psql, perhaps something like
> 
> INSERT INTO t1 VALUES ($1, $2) \gg 'val1' 'val2'
> 
> (\gexec and \gx are already taken.)
> 
> ------
> 
> This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
> on the direction.  As I mentioned above, a lot of the remaining work
> is arguably mostly straightforward.  Some closer examination of the
> issues surrounding the libpq API changes and psql would be useful.
> Perhaps there are other projects where that kind of functionality
> would also be useful.

Rebased patch, no new functionality.

On 29.06.22 01:29, Peter Eisentraut wrote:
> Here is a new version of this patch.  See also the original description 
> quoted below.  I have done a significant amount of work on this over the 
> last few months.  Some important news include:
> 
> - The cryptography has been improved.  It now uses an AEAD scheme, and 
> for deterministic encryption a proper SIV construction.
> 
> - The OpenSSL-specific parts have been moved to a separate file in 
> libpq.  Non-OpenSSL builds compile and work (without functionality, of 
> course).
> 
> - libpq handles multiple CEKs and CMKs, including changing keys on the fly.
> 
> - libpq supports a mode to force encryption of certain values.
> 
> - libpq supports a flexible configuration system for looking up CMKs, 
> including support for external key management systems.
> 
> - psql has a new \gencr command that allows passing in bind parameters 
> for (potential) encryption.
> 
> - There is some more pg_dump and psql support.
> 
> - The new data types for storing encrypted data have been renamed for 
> clarity.
> 
> - Various changes to the protocol compared to the previous patch.
> 
> - The patch contains full documentation of the protocol changes, 
> glossary entries, and more new documentation.
> 
> The major pieces that are still missing are:
> 
> - DDL support for registering keys
> 
> - Protocol versioning or feature flags
> 
> Other than that it's pretty complete in my mind.
> 
> For interested reviewers, I have organized the patch so that you can 
> start reading it top to bottom: The documentation comes first, then the 
> tests, then the code changes.  Even some feedback on the first or first 
> two aspects would be valuable to me.
> 
> Old news follows:
> 
> On 03.12.21 22:32, Peter Eisentraut wrote:
>> I want to present my proof-of-concept patch for the transparent column
>> encryption feature.  (Some might also think of it as automatic
>> client-side encryption or similar, but I like my name.)  This feature
>> enables the {automatic,transparent} encryption and decryption of
>> particular columns in the client.  The data for those columns then
>> only ever appears in ciphertext on the server, so it is protected from
>> the "prying eyes" of DBAs, sysadmins, cloud operators, etc.  The
>> canonical use case for this feature is storing credit card numbers
>> encrypted, in accordance with PCI DSS, as well as similar situations
>> involving social security numbers etc.  Of course, you can't do any
>> computations with encrypted values on the server, but for these use
>> cases, that is not necessary.  This feature does support deterministic
>> encryption as an alternative to the default randomized encryption, so
>> in that mode you can do equality lookups, at the cost of some
>> security.
>>
>> This functionality also exists in other SQL database products, so the
>> overall concepts weren't invented by me by any means.
>>
>> Also, this feature has nothing to do with the on-disk encryption
>> feature being contemplated in parallel.  Both can exist independently.
>>
>> The attached patch has all the necessary pieces in place to make this
>> work, so you can have an idea how the overall system works.  It
>> contains some documentation and tests to help illustrate the
>> functionality.  But it's missing the remaining 90% of the work,
>> including additional DDL support, error handling, robust memory
>> management, protocol versioning, forward and backward compatibility,
>> pg_dump support, psql \d support, refinement of the cryptography, and
>> so on.  But I think obvious solutions exist to all of those things, so
>> it isn't that interesting to focus on them for now.
>>
>> ------
>>
>> Now to the explanation of how it works.
>>
>> You declare a column as encrypted in a CREATE TABLE statement.  The
>> column value is encrypted by a symmetric key called the column
>> encryption key (CEK).  The CEK is a catalog object.  The CEK key
>> material is in turn encrypted by an assymmetric key called the column
>> master key (CMK).  The CMK is not stored in the database but somewhere
>> where the client can get to it, for example in a file or in a key
>> management system.  When a server sends rows containing encrypted
>> column values to the client, it first sends the required CMK and CEK
>> information (new protocol messages), which the client needs to record.
>> Then, the client can use this information to automatically decrypt the
>> incoming row data and forward it in plaintext to the application.
>>
>> For the CMKs, the catalog object specifies a "provider" and generic
>> options.  Right now, libpq has a "file" provider hardcoded, and it
>> takes a "filename" option.  Via some mechanism to be determined,
>> additional providers could be loaded and then talk to key management
>> systems via http or whatever.  I have left some comments in the libpq
>> code where the hook points for this could be.
>>
>> The general idea would be for an application to have one CMK per area
>> of secret stuff, for example, for credit card data.  The CMK can be
>> rotated: each CEK can be represented multiple times in the database,
>> encrypted by a different CMK.  (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.)
>>
>> The encryption algorithms are mostly hardcoded right now, but there
>> are facilities for picking algorithms and adding new ones that will be
>> expanded.  The CMK process uses RSA-OAEP.  The CEK process uses
>> AES-128-CBC right now; a more complete solution should probably
>> involve some HMAC thrown in.
>>
>> In the server, the encrypted datums are stored in types called
>> encryptedr and encryptedd (for randomized and deterministic
>> encryption).  These are essentially cousins of bytea.  For the rest of
>> the database system below the protocol handling, there is nothing
>> special about those.  For example, encryptedr has no operators at all,
>> encryptedd has only an equality operator.  pg_attribute has a new
>> column attrealtypid that stores the original type of the data in the
>> column.  This is only used for providing it to clients, so that
>> higher-level clients can convert the decrypted value to their
>> appropriate data types in their environments.
>>
>> Some protocol extensions are required.  These should be guarded by
>> some _pq_... setting, but this is not done in this patch yet.  As
>> mentioned above, extra messages are added for sending the CMKs and
>> CEKs.  In the RowDescription message, I have commandeered the format
>> field to add a bit that indicates that the field is encrypted.  This
>> could be made a separate field, and there should probably be
>> additional fields to indicate the algorithm and CEK name, but this was
>> easiest for now.  The ParameterDescription message is extended to
>> contain format fields for each parameter, for the same purpose.
>> Again, this could be done differently.
>>
>> Speaking of parameter descriptions, the trickiest part of this whole
>> thing appears to be how to get transparently encrypted data into the
>> database (as opposed to reading it out).  It is required to use
>> protocol-level prepared statements (i.e., extended query) for this.
>> The client must first prepare a statement, then describe the statement
>> to get parameter metadata, which indicates which parameters are to be
>> encrypted and how.  So this will require some care by applications
>> that want to do this, but, well, they probably should be careful
>> anyway.  In libpq, the existing APIs make this difficult, because
>> there is no way to pass the result of a describe-statement call back
>> into execute-statement-with-parameters.  I added new functions that do
>> this, so you then essentially do
>>
>>      res0 = PQdescribePrepared(conn, "");
>>      res = PQexecPrepared2(conn, "", 2, values, NULL, NULL, 0, res0);
>>
>> (The name could obviously be improved.)  Other client APIs that have a
>> "statement handle" concept could do this more elegantly and probably
>> without any API changes.
>>
>> Another challenge is that the parse analysis must check which
>> underlying column a parameter corresponds to.  This is similar to
>> resorigtbl and resorigcol in the opposite direction.  The current
>> implementation of this works for the test cases, but I know it has
>> some problems, so I'll continue working in this.  This functionality
>> is in principle available to all prepared-statement variants, not only
>> protocol-level.  So you can see in the tests that I expanded the
>> pg_prepared_statements view to show this information as well, which
>> also provides an easy way to test and debug this functionality
>> independent of column encryption.
>>
>> And also, psql doesn't use prepared statements, so writing into
>> encrypted columns currently doesn't work at all via psql.  (Reading
>> works no problem.)  All the test code currently uses custom libpq C
>> programs.  We should think about a way to enable prepared statements
>> in psql, perhaps something like
>>
>> INSERT INTO t1 VALUES ($1, $2) \gg 'val1' 'val2'
>>
>> (\gexec and \gx are already taken.)
>>
>> ------
>>
>> This is not targeting PostgreSQL 15.  But I'd appreciate some feedback
>> on the direction.  As I mentioned above, a lot of the remaining work
>> is arguably mostly straightforward.  Some closer examination of the
>> issues surrounding the libpq API changes and psql would be useful.
>> Perhaps there are other projects where that kind of functionality
>> would also be useful.

Updated patch, to resolve some merge conflicts.

Also, I added some CREATE DDL commands.  These aren't fully robust yet, 
but they do the basic job, so it makes the test cases easier to write 
and read, and they can be referred to in the documentation.  (Note that 
the corresponding DROP aren't there yet.)  I also expanded the 
documentation in the DDL chapter to give a complete recipe of how to set 
it up and use it.

On 7/12/22 11:29, Peter Eisentraut wrote:
> 
> Updated patch, to resolve some merge conflicts.

Thank you for working on this; it's an exciting feature.

>     The CEK key
>     material is in turn encrypted by an assymmetric key called the column
>     master key (CMK).

I'm not yet understanding why the CMK is asymmetric. Maybe you could use
the public key to add ephemeral, single-use encryption keys that no one
but the private key holder could use (after you forget them on your
side, that is). But since the entire column is encrypted with a single
CEK, you would essentially only be able to do that if you created an
entirely new column or table; do I have that right?

I'm used to public keys being safe for... publication, but if I'm
understanding correctly, it's important that the server admin doesn't
get hold of the public key for your CMK, because then they could
substitute their own CEKs transparently and undermine future encrypted
writes. That seems surprising. Am I just missing something important
about RSAES-OAEP?

> +#define PG_CEK_AEAD_AES_128_CBC_HMAC_SHA_256   130
> +#define PG_CEK_AEAD_AES_192_CBC_HMAC_SHA_384   131
> +#define PG_CEK_AEAD_AES_256_CBC_HMAC_SHA_384   132
> +#define PG_CEK_AEAD_AES_256_CBC_HMAC_SHA_512   133

It looks like these ciphersuites were abandoned by the IETF. Are there
existing implementations of them that have been audited/analyzed? Are
they safe (and do we know that the claims made in the draft are
correct)? How do they compare to other constructions like AES-GCM-SIV
and XChacha20-Poly1305?

> +-- \gencr
> +-- (This just tests the parameter passing; there is no encryption here.)
> +CREATE TABLE test_gencr (a int, b text);
> +INSERT INTO test_gencr VALUES (1, 'one') \gencr
> +SELECT * FROM test_gencr WHERE a = 1 \gencr
> + a |  b
> +---+-----
> + 1 | one
> +(1 row)
> +
> +INSERT INTO test_gencr VALUES ($1, $2) \gencr 2 'two'
> +SELECT * FROM test_gencr WHERE a IN ($1, $2) \gencr 2 3
> + a |  b
> +---+-----
> + 2 | two
> +(1 row)
I'd expect \gencr to error out without sending plaintext. I know that
under the hood this is just setting up a prepared statement, but if I'm
using \gencr, presumably I really do want to be encrypting my data.
Would it be a problem to always set force-column-encryption for the
parameters we're given here? Any unencrypted columns could be provided
directly.

Another idle thought I had was that it'd be nice to have some syntax for
providing a null value to \gencr (assuming I didn't overlook it in the
patch). But that brings me to...

> +  <para>
> +   Null values are not encrypted by transparent column encryption; null values
> +   sent by the client are visible as null values in the database.  If the fact
> +   that a value is null needs to be hidden from the server, this information
> +   needs to be encoded into a nonnull value in the client somehow.
> +  </para>

This is a major gap, IMO. Especially with the switch to authenticated
ciphers, because it means you can't sign your NULL values. And having
each client or user that's out there solve this with a magic in-band
value seems like a recipe for pain.

Since we're requiring "canonical" use of text format, and the docs say
there are no embedded or trailing nulls allowed in text values, could we
steal the use of a single zero byte to mean NULL? One additional
complication would be that the client would have to double-check that
we're not writing a NULL into a NOT NULL column, and complain if it
reads one during decryption. Another complication would be that the
client would need to complain if it got a plaintext NULL.

(The need for robust client-side validation of encrypted columns might
be something to expand on in the docs more generally, since before this
feature, it could probably be assumed that the server was buggy if it
sent you unparsable junk in a column.)

> +   <para>
> +    The <quote>associated data</quote> in these algorithms consists of 4
> +    bytes: The ASCII letters <literal>P</literal> and <literal>G</literal>
> +    (byte values 80 and 71), followed by the algorithm ID as a 16-bit unsigned
> +    integer in network byte order.
> +   </para>

Is this AD intended as a placeholder for the future, or does it serve a
particular purpose?

Thanks,
--Jacob

On 15.07.22 19:47, Jacob Champion wrote:
>>      The CEK key
>>      material is in turn encrypted by an assymmetric key called the column
>>      master key (CMK).
> 
> I'm not yet understanding why the CMK is asymmetric.

I'm not totally sure either.  I started to build it that way because 
other systems were doing it that way, too.  But I have been thinking 
about adding a symmetric alternative for the CMKs as well (probably AESKW).

I think there are a couple of reasons why asymmetric keys are possibly 
useful for CMKs:

Some other products make use of secure enclaves to do computations on 
(otherwise) encrypted values on the server.  I don't fully know how that 
works, but I suspect that asymmetric keys can play a role in that.  (I 
don't have any immediate plans for that in my patch.  It seems to be a 
dying technology at the moment.)

Asymmetric keys gives you some more options for how you set up the keys 
at the beginning.  For example, you create the asymmetric key pair on 
the host where your client program that wants access to the encrypted 
data will run.  You put the private key in an appropriate location for 
run time.  You send the public key to another host.  On that other host, 
you create the CEK, encrypt it with the CMK, and then upload it into the 
server (CREATE COLUMN ENCRYPTION KEY).  Then you can wipe that second 
host.  That way, you can be even more sure that the unencrypted CEK 
isn't left anywhere.  I'm not sure whether this method is very useful in 
practice, but it's interesting.

In any case, as I mentioned above, this particular aspect is up for 
discussion.

Also note that if you use a KMS (cmklookup "run" method), the actual 
algorithm doesn't even matter (depending on details of the KMS setup), 
since you just tell the KMS "decrypt this", and the KMS knows by itself 
what algorithm to use.  Maybe there should be a way to specify "unknown" 
in the ckdcmkalg field.

>> +#define PG_CEK_AEAD_AES_128_CBC_HMAC_SHA_256   130
>> +#define PG_CEK_AEAD_AES_192_CBC_HMAC_SHA_384   131
>> +#define PG_CEK_AEAD_AES_256_CBC_HMAC_SHA_384   132
>> +#define PG_CEK_AEAD_AES_256_CBC_HMAC_SHA_512   133
> 
> It looks like these ciphersuites were abandoned by the IETF. Are there
> existing implementations of them that have been audited/analyzed? Are
> they safe (and do we know that the claims made in the draft are
> correct)? How do they compare to other constructions like AES-GCM-SIV
> and XChacha20-Poly1305?

The short answer is, these same algorithms are used in equivalent 
products (see MS SQL Server, MongoDB).  They even reference the same 
exact draft document.

Besides that, here is my analysis for why these are good choices: You 
can't use any of the counter modes, because since the encryption happens 
on the client, there is no way to coordinate to avoid nonce reuse.  So 
among mainstream modes, you are basically left with AES-CBC with a 
random IV.  In that case, even if you happen to reuse an IV, the 
possible damage is very contained.

And then, if you want to use AEAD, you combine that with some MAC, and 
HMAC is just as good as any for that.

The referenced draft document doesn't really contain any additional 
cryptographic insights, it's just a guide on a particular way to put 
these two together.

So altogether I think this is a pretty solid choice.

>> +-- \gencr
>> +-- (This just tests the parameter passing; there is no encryption here.)
>> +CREATE TABLE test_gencr (a int, b text);
>> +INSERT INTO test_gencr VALUES (1, 'one') \gencr
>> +SELECT * FROM test_gencr WHERE a = 1 \gencr
>> + a |  b
>> +---+-----
>> + 1 | one
>> +(1 row)
>> +
>> +INSERT INTO test_gencr VALUES ($1, $2) \gencr 2 'two'
>> +SELECT * FROM test_gencr WHERE a IN ($1, $2) \gencr 2 3
>> + a |  b
>> +---+-----
>> + 2 | two
>> +(1 row)
> I'd expect \gencr to error out without sending plaintext. I know that
> under the hood this is just setting up a prepared statement, but if I'm
> using \gencr, presumably I really do want to be encrypting my data.
> Would it be a problem to always set force-column-encryption for the
> parameters we're given here? Any unencrypted columns could be provided
> directly.

Yeah, this needs a bit of refinement.  You don't want something named 
"encr" but it only encrypts some of the time.  We could possibly do what 
you suggest and make it set the force-encryption flag, or maybe rename 
it or add another command that just uses prepared statements and doesn't 
promise anything about encryption from its name.

This also ties in with how pg_dump will eventually work.  I think by 
default pg_dump will just dump things encrypted and set it up so that 
COPY writes it back encrypted.  But there should probably be a mode that 
dumps out plaintext and then uses one of these commands to load the 
plaintext back in.  What these psql commands need to do also depends on 
what pg_dump needs them to do.

>> +  <para>
>> +   Null values are not encrypted by transparent column encryption; null values
>> +   sent by the client are visible as null values in the database.  If the fact
>> +   that a value is null needs to be hidden from the server, this information
>> +   needs to be encoded into a nonnull value in the client somehow.
>> +  </para>
> 
> This is a major gap, IMO. Especially with the switch to authenticated
> ciphers, because it means you can't sign your NULL values. And having
> each client or user that's out there solve this with a magic in-band
> value seems like a recipe for pain.
> 
> Since we're requiring "canonical" use of text format, and the docs say
> there are no embedded or trailing nulls allowed in text values, could we
> steal the use of a single zero byte to mean NULL? One additional
> complication would be that the client would have to double-check that
> we're not writing a NULL into a NOT NULL column, and complain if it
> reads one during decryption. Another complication would be that the
> client would need to complain if it got a plaintext NULL.

You're already alluding to some of the complications.  Also consider 
that null values could arise from, say, outer joins.  So you could be in 
a situation where encrypted and unencrypted null values coexist.  And of 
course the server doesn't know about the encrypted null values.  So how 
do you maintain semantics, like for aggregate functions, primary keys, 
anything that treats null values specially?  How do clients deal with a 
mix of encrypted and unencrypted null values, how do they know which one 
is real.  What if the client needs to send a null value back as a 
parameter?  All of this would create enormous complications, if they can 
be solved at all.

I think a way to look at this is that this column encryption feature 
isn't suitable for disguising the existence or absence of data, it can 
only disguise the particular data that you know exists.

>> +   <para>
>> +    The <quote>associated data</quote> in these algorithms consists of 4
>> +    bytes: The ASCII letters <literal>P</literal> and <literal>G</literal>
>> +    (byte values 80 and 71), followed by the algorithm ID as a 16-bit unsigned
>> +    integer in network byte order.
>> +   </para>
> 
> Is this AD intended as a placeholder for the future, or does it serve a
> particular purpose?

It has been recommended that you include the identity of the encryption 
algorithm in the AD.  This protects the client from having to decrypt 
stuff that wasn't meant to be decrypted (in that way).

On Mon, Jul 18, 2022 at 6:53 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I think a way to look at this is that this column encryption feature
> isn't suitable for disguising the existence or absence of data, it can
> only disguise the particular data that you know exists.

+1.

Even there, what can be accomplished with a feature that only encrypts
individual column values is by nature somewhat limited. If you have a
text column that, for one row, stores the value 'a', and for some
other row, stores the entire text of Don Quixote in the original
Spanish, it is going to be really difficult to keep an adversary who
can read from the disk from distinguishing those rows. If you want to
fix that, you're going to need to do block-level encryption or
something of that sort. And even then, you still have another version
of the problem, because now imagine you have one *table* that contains
nothing but the value 'a' and another that contains nothing but the
entire text of Don Quixote, it is going to be possible for an
adversary to tell those tables apart, even with the corresponding
files encrypted in their entirety.

But I don't think that this means that a feature like this has no
value. I think it just means that we need to clearly document how the
feature works and not over-promise.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 12.07.22 20:29, Peter Eisentraut wrote:
> Updated patch, to resolve some merge conflicts.

Rebased patch, no new functionality

On Tue, Jul 19, 2022 at 10:52 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 12.07.22 20:29, Peter Eisentraut wrote:
> > Updated patch, to resolve some merge conflicts.
>
> Rebased patch, no new functionality

Thank you for working on this and updating the patch!

I've mainly looked at the documentation and tests and done some tests.
Before looking at the code in depth, I'd like to share my
comments/questions.

---
Regarding the documentation, I'd like to have a page that describes
the generic information of the transparent column encryption for users
such as what this feature actually does, what can be achieved by this
feature, CMK rotation, and its known limitations. The patch has
"Transparent Column Encryption" section in protocol.sgml but it seems
to be more internal information.

---
In datatype.sgml, it says "Thus, clients that don't support
transparent column encryption or have disabled it will see the
encrypted values as byte arrays." but I got an error rather than
encrypted values when I tried to connect to the server using by
clients that don't support the encryption:

postgres(1:6040)=# select * from tbl;
no CMK lookup found for realm ""

no CMK lookup found for realm ""
postgres(1:6040)=#

---
In single-user mode, the user cannot decrypt the encrypted value but
probably it's fine in practice.

---
Regarding the column master key rotation, would it be useful if we
provide a tool for that? For example, it takes old and new CMK as
input, re-encrypt all CEKs realted to the CMK, and registers them to
the server.

---
Is there any convenient way to load a large amount of test data to the
encrypted columns? I tried to use generate_series() but it seems not
to work as it generates the data on the server side:

postgres(1:80556)=# create table a (i text encrypted with
(column_encryption_key = cek1));
CREATE TABLE
postgres(1:80556)=# insert into a select i::text from
generate_series(1, 1000) i;
2022-07-20 15:06:38.502 JST [80556] ERROR:  column "i" is of type
pg_encrypted_rnd but expression is of type text at character 22

I've also tried to load the data from a file on the client by using
\copy command, but it seems not to work:

postgres(1:80556)=# copy (select generate_series(1, 1000)::text) to
'/tmp/tmp.dat';
COPY 1000
postgres(1:80556)=# \copy a from '/tmp/tmp.dat'
COPY 1000
postgres(1:80556)=# select * from a;
out out memory

---
I got SEGV in the following two situations:

(1) SEGV by backend
postgres(1:59931)=# create table tbl (i int encrypted with
(column_encryption_key = cek1));
CREATE TABLE
postgres(1:59931)=# insert into tbl values ($1) \gencr 1
INSERT 0 1
postgres(1:59931)=# select * from tbl;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.

The backtrace is:

(lldb) bt
* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x0000000106830a30
postgres`pg_detoast_datum_packed(datum=0xffffffffab32c563) at
fmgr.c:1742:6
    frame #1: 0x00000001067d9dbf
postgres`byteaout(fcinfo=0x00007ffee9c311a8) at varlena.c:392:20
    frame #2: 0x000000010682ed0c
postgres`FunctionCall1Coll(flinfo=0x00007faeb28193a8, collation=0,
arg1=18446744072286815587) at fmgr.c:1124:11
    frame #3: 0x0000000106830611
postgres`OutputFunctionCall(flinfo=0x00007faeb28193a8,
val=18446744072286815587) at fmgr.c:1561:9
    frame #4: 0x0000000105fed702
postgres`printtup(slot=0x00007faeb2818960, self=0x00007faeb3809390) at
printtup.c:519:16
    frame #5: 0x0000000106319318
postgres`ExecutePlan(estate=0x00007faeb2818520,
planstate=0x00007faeb2818758, use_parallel_mode=false,
operation=CMD_SELECT, sendTuples=true, numberTuples=0,
direction=ForwardScanDirection, dest=0x00007faeb3809390,
execute_once=true) at execMain.c:1667:9
    frame #6: 0x0000000106319180
postgres`standard_ExecutorRun(queryDesc=0x00007faeb280d920,
direction=ForwardScanDirection, count=0, execute_once=true) at
execMain.c:363:3
    frame #7: 0x0000000106318f11
postgres`ExecutorRun(queryDesc=0x00007faeb280d920,
direction=ForwardScanDirection, count=0, execute_once=true) at
execMain.c:307:3
    frame #8: 0x000000010661139c
postgres`PortalRunSelect(portal=0x00007faeb5028920, forward=true,
count=0, dest=0x00007faeb3809390) at pquery.c:924:4
    frame #9: 0x0000000106610d3f
postgres`PortalRun(portal=0x00007faeb5028920,
count=9223372036854775807, isTopLevel=true, run_once=true,
dest=0x00007faeb3809390, altdest=0x00007faeb3809390,
qc=0x00007ffee9c31620) at pquery.c:768:18
    frame #10: 0x000000010660bb93
postgres`exec_simple_query(query_string="select * from tbl;") at
postgres.c:1246:10
    frame #11: 0x000000010660ac2f
postgres`PostgresMain(dbname="postgres", username="masahiko") at
postgres.c:4534:7
    frame #12: 0x000000010650d9c6
postgres`BackendRun(port=0x00007faeb3004210) at postmaster.c:4490:2
    frame #13: 0x000000010650cf8a
postgres`BackendStartup(port=0x00007faeb3004210) at
postmaster.c:4218:3
    frame #14: 0x000000010650bd57 postgres`ServerLoop at postmaster.c:1808:7
    frame #15: 0x00000001065094cf postgres`PostmasterMain(argc=5,
argv=0x00007faeb2406320) at postmaster.c:1480:11
    frame #16: 0x00000001063b4dcf postgres`main(argc=5,
argv=0x00007faeb2406320) at main.c:197:3
    frame #17: 0x00007fff721abcc9 libdyld.dylib`start + 1

(2) SEGV by psql

postgres(1:47762)=# create table tbl (t text encrypted with
(column_encryption_key = cek1));
CREATE TABLE
postgres(1:47762)=# insert into tbl values ('test');
INSERT 0 1
postgres(1:47762)=# select * from tbl;
Segmentation fault: 11 (core dumped)

The backtrace is:

(lldb) bt
* thread #1, stop reason = signal SIGSTOP
  * frame #0: 0x00007fff723a1b36
libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 566
    frame #1: 0x000000010c509a5f
libpq.5.dylib`get_message_auth_tag(md=0x000000010c7f28b8, mac_key="
\x1c,\x98g½ȩ[\x88\x16\x12Kiꔂ\v8g_\x80, mac_key_len=16, encr="test",
encrlen=-12, cekalg=130, md_value="", md_len_p=0x00007ffee380a720,
errmsgp=0x00007ffee380a8c0) at fe-encrypt-openssl.c:316:2
    frame #2: 0x000000010c509442
libpq.5.dylib`decrypt_value(res=0x00007fa098504770,
cek=0x00007fa0985045d0, cekalg=130, input="test", inputlen=4,
errmsgp=0x00007ffee380a8c0) at fe-encrypt-openssl.c:429:7
    frame #3: 0x000000010c4f3c0c
libpq.5.dylib`pqRowProcessor(conn=0x00007fa098808e00,
errmsgp=0x00007ffee380a8c0) at fe-exec.c:1670:21
    frame #4: 0x000000010c5013bb
libpq.5.dylib`getAnotherTuple(conn=0x00007fa098808e00, msgLength=16)
at fe-protocol3.c:882:6
    frame #5: 0x000000010c4ffbf2
libpq.5.dylib`pqParseInput3(conn=0x00007fa098808e00) at
fe-protocol3.c:410:11
    frame #6: 0x000000010c4f5b65
libpq.5.dylib`parseInput(conn=0x00007fa098808e00) at fe-exec.c:2598:2
    frame #7: 0x000000010c4f5c59
libpq.5.dylib`PQgetResult(conn=0x00007fa098808e00) at fe-exec.c:2684:3
    frame #8: 0x000000010c401a77
psql`ExecQueryAndProcessResults(query="select * from tbl;",
elapsed_msec=0x00007ffee380ab08, svpt_gone_p=0x00007ffee380aafe,
is_watch=false, opt=0x0000000000000000,
printQueryFout=0x0000000000000000) at common.c:1514:11
    frame #9: 0x000000010c402469 psql`SendQuery(query="select * from
tbl;") at common.c:1171:9
    frame #10: 0x000000010c41a4dd
psql`MainLoop(source=0x00007fff98ce8d90) at mainloop.c:439:16
    frame #11: 0x000000010c426b44 psql`main(argc=3,
argv=0x00007ffee380adc8) at startup.c:462:19
    frame #12: 0x00007fff721abcc9 libdyld.dylib`start + 1
    frame #13: 0x00007fff721abcc9 libdyld.dylib`start + 1
(lldb) f 1
frame #1: 0x000000010c509a5f
libpq.5.dylib`get_message_auth_tag(md=0x000000010c7f28b8, mac_key="
\x1c,\x98g½ȩ[\x88\x16\x12Kiꔂ\v8g_\x80, mac_key_len=16, encr="test",
encrlen=-12, cekalg=130, md_value="", md_len_p=0x00007ffee380a720,
errmsgp=0x00007ffee380a8c0) at fe-encrypt-openssl.c:316:2
   313  #else
   314          memcpy(buf, test_A, sizeof(test_A));
   315  #endif
-> 316          memcpy(buf + PG_AD_LEN, encr, encrlen);
   317          *(int64 *) (buf + PG_AD_LEN + encrlen) =
pg_hton64(PG_AD_LEN * 8);
   318
   319          if (!EVP_DigestSignInit(evp_md_ctx, NULL, md, NULL, pkey))
(lldb) p encrlen
(int) $0 = -12
(lldb)

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Mon, Jul 18, 2022 at 3:53 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> Some other products make use of secure enclaves to do computations on
> (otherwise) encrypted values on the server.  I don't fully know how that
> works, but I suspect that asymmetric keys can play a role in that.  (I
> don't have any immediate plans for that in my patch.  It seems to be a
> dying technology at the moment.)
>
> Asymmetric keys gives you some more options for how you set up the keys
> at the beginning.  For example, you create the asymmetric key pair on
> the host where your client program that wants access to the encrypted
> data will run.  You put the private key in an appropriate location for
> run time.  You send the public key to another host.  On that other host,
> you create the CEK, encrypt it with the CMK, and then upload it into the
> server (CREATE COLUMN ENCRYPTION KEY).  Then you can wipe that second
> host.  That way, you can be even more sure that the unencrypted CEK
> isn't left anywhere.  I'm not sure whether this method is very useful in
> practice, but it's interesting.

As long as it's clear to people trying this that the "public" key
cannot actually be made public, I suppose. That needs to be documented
IMO. I like your idea of providing a symmetric option as well.

> In any case, as I mentioned above, this particular aspect is up for
> discussion.
>
> Also note that if you use a KMS (cmklookup "run" method), the actual
> algorithm doesn't even matter (depending on details of the KMS setup),
> since you just tell the KMS "decrypt this", and the KMS knows by itself
> what algorithm to use.  Maybe there should be a way to specify "unknown"
> in the ckdcmkalg field.

+1, an officially client-defined method would probably be useful.

> The short answer is, these same algorithms are used in equivalent
> products (see MS SQL Server, MongoDB).  They even reference the same
> exact draft document.
>
> Besides that, here is my analysis for why these are good choices: You
> can't use any of the counter modes, because since the encryption happens
> on the client, there is no way to coordinate to avoid nonce reuse.  So
> among mainstream modes, you are basically left with AES-CBC with a
> random IV.  In that case, even if you happen to reuse an IV, the
> possible damage is very contained.)

I think both AES-GCM-SIV and XChaCha20-Poly1305 are designed to handle
the nonce problem as well. In any case, if I were deploying this, I'd
want to know the characteristics/limits of our chosen suites (e.g. how
much data can be encrypted per key) so that I could plan rotations
correctly. Something like the table in [1]?

> > Since we're requiring "canonical" use of text format, and the docs say
> > there are no embedded or trailing nulls allowed in text values, could we
> > steal the use of a single zero byte to mean NULL? One additional
> > complication would be that the client would have to double-check that
> > we're not writing a NULL into a NOT NULL column, and complain if it
> > reads one during decryption. Another complication would be that the
> > client would need to complain if it got a plaintext NULL.
>
> You're already alluding to some of the complications.  Also consider
> that null values could arise from, say, outer joins.  So you could be in
> a situation where encrypted and unencrypted null values coexist.

(I realize I'm about to wade into the pool of what NULL means in SQL,
the subject of which I've stayed mostly, gleefully, ignorant.)

To be honest that sounds pretty useful. Any unencrypted null must have
come from the server computation; it's a clear claim by the server
that no such rows exist. (If the encrypted column is itself NOT NULL
then there's no ambiguity to begin with, I think.) That wouldn't be
transparent behavior anymore, so it may (understandably) be a non-goal
for the patch, but it really does sound useful.

And it might be a little extreme, but if I as a user decided that I
wanted in-band encrypted null, it wouldn't be particularly surprising
to me if such a column couldn't be included in an outer join. Just
like I can't join on a randomized encrypted column, or add two
encrypted NUMERICs to each other. In fact I might even want the server
to enforce NOT NULL transparently on the underlying pg_encrypted_*
column, to make sure that I didn't accidentally push an unencrypted
NULL by mistake.

> And of
> course the server doesn't know about the encrypted null values.  So how
> do you maintain semantics, like for aggregate functions, primary keys,
> anything that treats null values specially?

Could you elaborate? Any special cases seem like they'd be important
to document regardless of whether or not we support in-band null
encryption. For example, do you plan to support encrypted primary
keys, null or not? That seems like it'd be particularly difficult
during CEK rotation.

> How do clients deal with a
> mix of encrypted and unencrypted null values, how do they know which one
> is real.

That one seems straightforward -- a bare null in an encrypted column
is an assertion by the server. An encrypted null had to have come from
the client side originally.

> What if the client needs to send a null value back as a
> parameter?

Couldn't the client just encrypt it, same as any other column? Or am I
missing what you mean by "parameter" here?

> All of this would create enormous complications, if they can
> be solved at all.

That could be. But I'm wondering if the complications exist
regardless, and the null example is just making them more obvious.

> It has been recommended that you include the identity of the encryption
> algorithm in the AD.  This protects the client from having to decrypt
> stuff that wasn't meant to be decrypted (in that way).

Do you have a link? I'd like to read up on that -- I naively assumed
that the suite wouldn't be able to decrypt another AEAD cipher without
complaining.

--Jacob

[1] https://doc.libsodium.org/secret-key_cryptography/aead

On Mon, Jul 18, 2022 at 9:07 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Even there, what can be accomplished with a feature that only encrypts
> individual column values is by nature somewhat limited. If you have a
> text column that, for one row, stores the value 'a', and for some
> other row, stores the entire text of Don Quixote in the original
> Spanish, it is going to be really difficult to keep an adversary who
> can read from the disk from distinguishing those rows. If you want to
> fix that, you're going to need to do block-level encryption or
> something of that sort.

A minimum padding option would fix the leak here, right? If every
entry is the same length then there's no information to be gained, at
least in an offline analysis.

I think some work around that is probably going to be needed for
serious use of this encryption, in part because of the use of text
format as the canonical input. If the encrypted values of 1, 10, 100,
and 1000 hypothetically leaked their exact lengths, then an encrypted
int wouldn't be very useful. So I'd want to quantify (and possibly
configure) exactly how much data you can encrypt in a single message
before the length starts being leaked, and then make sure that my
encrypted values stay inside that bound.

--Jacob

On Mon, Jul 18, 2022 at 12:53:23PM +0200, Peter Eisentraut wrote:
> Asymmetric keys gives you some more options for how you set up the keys at
> the beginning.  For example, you create the asymmetric key pair on the host
> where your client program that wants access to the encrypted data will run.
> You put the private key in an appropriate location for run time.  You send
> the public key to another host.  On that other host, you create the CEK,
> encrypt it with the CMK, and then upload it into the server (CREATE COLUMN
> ENCRYPTION KEY).  Then you can wipe that second host.  That way, you can be
> even more sure that the unencrypted CEK isn't left anywhere.  I'm not sure
> whether this method is very useful in practice, but it's interesting.
> 
> In any case, as I mentioned above, this particular aspect is up for
> discussion.

I caution against adding complexity without a good reason, because
historically complexity often leads to exploits and bugs, especially
with crypto.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

On Thu, Jul 21, 2022 at 2:30 PM Jacob Champion <jchampion@timescale.com> wrote:
> On Mon, Jul 18, 2022 at 9:07 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > Even there, what can be accomplished with a feature that only encrypts
> > individual column values is by nature somewhat limited. If you have a
> > text column that, for one row, stores the value 'a', and for some
> > other row, stores the entire text of Don Quixote in the original
> > Spanish, it is going to be really difficult to keep an adversary who
> > can read from the disk from distinguishing those rows. If you want to
> > fix that, you're going to need to do block-level encryption or
> > something of that sort.
>
> A minimum padding option would fix the leak here, right? If every
> entry is the same length then there's no information to be gained, at
> least in an offline analysis.

Sure, but padding every text column that you have, even the ones
containing only 'a', out to the length of Don Quixote in the original
Spanish, is unlikely to be an appealing option.

> I think some work around that is probably going to be needed for
> serious use of this encryption, in part because of the use of text
> format as the canonical input. If the encrypted values of 1, 10, 100,
> and 1000 hypothetically leaked their exact lengths, then an encrypted
> int wouldn't be very useful. So I'd want to quantify (and possibly
> configure) exactly how much data you can encrypt in a single message
> before the length starts being leaked, and then make sure that my
> encrypted values stay inside that bound.

I think most ciphers these days are block ciphers, so you're going to
get output that is a multiple of the block size anyway - e.g. I think
for AES it's 128 bits = 16 bytes. So small differences in length will
be concealed naturally, which may be good enough for some use cases.

I'm not really convinced that it's worth putting a lot of effort into
bolstering the security of this kind of tech above what it naturally
gives. I think it's likely to be a wild goose chase. If you have major
worries about someone reading your disk in its entirety, use full-disk
encryption. Selective encryption is only suitable when you want to add
a modest level of protection for individual value and are willing to
accept that some information leakage is likely if an adversary can in
fact read the full disk. Padding values to try to further obscure
things may be situationally useful, but if you find yourself worrying
too much about that sort of thing, you likely should have picked
stronger medicine initially.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jul 26, 2022 at 10:52 AM Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jul 21, 2022 at 2:30 PM Jacob Champion <jchampion@timescale.com> wrote:
> > A minimum padding option would fix the leak here, right? If every
> > entry is the same length then there's no information to be gained, at
> > least in an offline analysis.
>
> Sure, but padding every text column that you have, even the ones
> containing only 'a', out to the length of Don Quixote in the original
> Spanish, is unlikely to be an appealing option.

If you are honestly trying to conceal Don Quixote, I suspect you are
already in the business of making unappealing decisions. I don't think
that's necessarily an argument against hiding the length for
real-world use cases.

> > I think some work around that is probably going to be needed for
> > serious use of this encryption, in part because of the use of text
> > format as the canonical input. If the encrypted values of 1, 10, 100,
> > and 1000 hypothetically leaked their exact lengths, then an encrypted
> > int wouldn't be very useful. So I'd want to quantify (and possibly
> > configure) exactly how much data you can encrypt in a single message
> > before the length starts being leaked, and then make sure that my
> > encrypted values stay inside that bound.
>
> I think most ciphers these days are block ciphers, so you're going to
> get output that is a multiple of the block size anyway - e.g. I think
> for AES it's 128 bits = 16 bytes. So small differences in length will
> be concealed naturally, which may be good enough for some use cases.

Right. My point is, if you have a column that has exactly one
important value that is 17 bytes long when converted to text, you're
going to want to know that block size exactly, because the encryption
will be effectively useless for that value. That size needs to be
documented, and it'd be helpful to know that it's longer than, say,
the longest text representation of our fixed-length column types.

> I'm not really convinced that it's worth putting a lot of effort into
> bolstering the security of this kind of tech above what it naturally
> gives. I think it's likely to be a wild goose chase.

If the goal is to provide real encryption, and not just a toy, I think
you're going to need to put a *lot* of effort into analysis. Even if
the result of the analysis is "we don't plan to address this in v1".

Crypto is inherently a cycle of
make-it-and-break-it-and-fix-it-and-break-it-again. If that's
considered a "wild goose chase" and not seriously pursued at some
level, then this implementation will probably not last long in the
face of real abuse. (That doesn't mean you have to take my advice; I'm
just a dude with opinions -- but you will need to have real
cryptographers look at this, and you're going to need to think about
how the system will evolve when it's broken.)

> If you have major
> worries about someone reading your disk in its entirety, use full-disk
> encryption.

This patchset was designed to protect against the evil DBA case, I
think. Full disk encryption doesn't help.

> Selective encryption is only suitable when you want to add
> a modest level of protection for individual value and are willing to
> accept that some information leakage is likely if an adversary can in
> fact read the full disk.

...but there's a known countermeasure to this particular leakage,
right? Which would make it more suitable for that case.

> Padding values to try to further obscure
> things may be situationally useful, but if you find yourself worrying
> too much about that sort of thing, you likely should have picked
> stronger medicine initially.

In my experience, this entire field is the application of
situationally useful protection. That's one of the reasons it's hard,
and designing this sort of patch is going to be hard too. Putting that
on the user isn't quite fair when you're the ones designing the
system; you determine what they have to worry about when you choose
the crypto.

--Jacob

On Tue, Jul 26, 2022 at 2:27 PM Jacob Champion <jchampion@timescale.com> wrote:
> Right. My point is, if you have a column that has exactly one
> important value that is 17 bytes long when converted to text, you're
> going to want to know that block size exactly, because the encryption
> will be effectively useless for that value. That size needs to be
> documented, and it'd be helpful to know that it's longer than, say,
> the longest text representation of our fixed-length column types.

I certainly have no objection to being clear about such details in the
documentation.

> If the goal is to provide real encryption, and not just a toy, I think
> you're going to need to put a *lot* of effort into analysis. Even if
> the result of the analysis is "we don't plan to address this in v1".
>
> Crypto is inherently a cycle of
> make-it-and-break-it-and-fix-it-and-break-it-again. If that's
> considered a "wild goose chase" and not seriously pursued at some
> level, then this implementation will probably not last long in the
> face of real abuse. (That doesn't mean you have to take my advice; I'm
> just a dude with opinions -- but you will need to have real
> cryptographers look at this, and you're going to need to think about
> how the system will evolve when it's broken.)

Well, I'm just a dude with opinions, too. I fear the phenomenon where
doing anything about a problem makes you responsible for the whole
problem. If we disclaim the ability to hide the length of values,
that's clear enough. But if we start padding to try to hide the length
of values, then people might expect it to work in all cases, and I
don't see how it ever can. Moreover, I think that the padding might
need to be done in a "cryptographically intelligent" way rather than
just, say, adding trailing blanks. Now that being said, if Peter wants
to implement something around padding that he has reason to believe
will not create cryptographic weaknesses, I have no issue with that. I
just don't view it as an essential part of the feature, because hiding
such things doesn't seem like it can ever be the main point of a
feature like this.

> > Padding values to try to further obscure
> > things may be situationally useful, but if you find yourself worrying
> > too much about that sort of thing, you likely should have picked
> > stronger medicine initially.
>
> In my experience, this entire field is the application of
> situationally useful protection. That's one of the reasons it's hard,
> and designing this sort of patch is going to be hard too.

Agreed.

> Putting that
> on the user isn't quite fair when you're the ones designing the
> system; you determine what they have to worry about when you choose
> the crypto.

I guess my view on this is that, if you're trying to hide something
like a credit card number, most likely every value in the system is
the same length, and then this is a non-issue. On the other hand, if
the secret column is a person's name, then there is an issue, but
you're not going to pad every value out the maximum length of a
varlena, so you have to make an estimate of how long a name someone
might reasonably have to decide how much padding to include. You also
have to decide whether the storage cost of padding every value is
worth it to you given the potential information leakage. Only the
human user can make those decisions, so some amount of "putting that
on the user" feels inevitable. Now, if we don't have a padding system
built into the feature, then that does put even more on the user; it's
hard to argue with that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 7/26/22 13:25, Robert Haas wrote:
> I certainly have no objection to being clear about such details in the
> documentation.

Cool.

> I fear the phenomenon where
> doing anything about a problem makes you responsible for the whole
> problem. If we disclaim the ability to hide the length of values,
> that's clear enough.

I don't think disclaiming responsibility absolves you of it here, in
part because choices are being made (text format) that make length
hiding even more important than it otherwise would be. A user who
already knows that encryption doesn't hide length might still reasonably
expect a fixed-length column type like bigint to be protected in all
cases. It won't be (at least not with your 16-byte example).

And sure, you can document that caveat too, but said user might then
reasonably wonder how they're supposed to actually make it safe.

> But if we start padding to try to hide the length
> of values, then people might expect it to work in all cases, and I
> don't see how it ever can.

Well, that's where I agree with you on the value of solid documentation.
But there are other things we can do as well. In general we should

- choose a default that will protect most people out of the box,
- document the heck out of the default's limitations,
- provide guardrails that warn the user when they're outgrowing those
  limitations, and
- give people a way to tune it to their own use cases.

As an example, a naive guardrail in this instance could be to simply
have the client refuse to encrypt data past the padding maximum, if
you've gone so far as to set one up. It'd suck to hit that maximum in
production and have to rewrite the column, but you did want your
encryption to hide your data, right?

Maybe that's way too complex to think about for a v1, but it'll be
easier to maintain this into the future if there's at least a plan to
create a v2. If you declare it out of scope, instead of considering it a
potential TODO, then I think it'll be a lot harder for people to improve it.

> Moreover, I think that the padding might
> need to be done in a "cryptographically intelligent" way rather than
> just, say, adding trailing blanks.

Possibly. I think that's where AEAD comes in -- if you've authenticated
your ciphertext sufficiently, padding oracles should be prohibitively
difficult(?). (But see below; I think we also have other things to worry
about in terms of authentication and oracles.)

> Now that being said, if Peter wants
> to implement something around padding that he has reason to believe
> will not create cryptographic weaknesses, I have no issue with that. I
> just don't view it as an essential part of the feature, because hiding
> such things doesn't seem like it can ever be the main point of a
> feature like this.

I think that side channel consideration has to be an essential part of
any cryptography feature. Recent history has shown "obscure" side
channels gaining power to the point of completely breaking crypto schemes.

And it's not like TLS where we have to protect an infinite stream of
arbitrary bytes; this is going to be used on small values that probably
get repeated often and have (comparatively) very little entropy.
Cryptanalysis based on length seems to me like part and parcel of the
problem space.

> I guess my view on this is that, if you're trying to hide something
> like a credit card number, most likely every value in the system is
> the same length, and then this is a non-issue.

Agreed.

> On the other hand, if
> the secret column is a person's name, then there is an issue, but
> you're not going to pad every value out the maximum length of a
> varlena, so you have to make an estimate of how long a name someone
> might reasonably have to decide how much padding to include. You also
> have to decide whether the storage cost of padding every value is
> worth it to you given the potential information leakage. Only the
> human user can make those decisions, so some amount of "putting that
> on the user" feels inevitable.

Agreed.

> Now, if we don't have a padding system
> built into the feature, then that does put even more on the user; it's
> hard to argue with that.
Right. If they can even fix it at all. Having a well-documented padding
feature would not only help mitigate that, it would conveniently hang a
big sign on the caveats that exist.

--

Speaking of oracles and side channels. Users may want to use associated
data to further lock an encrypted value to its column type, too.
Otherwise it seems like an active DBA could feed an encrypted text blob
to a client in place of, say, an int column, to see whether or not that
text blob is a number. Seems like AD is going to be important to prevent
active attacks in general.

--Jacob

Here is an updated patch.

I mainly spent time on adding a full set of DDL commands for the keys. 
This made the patch very bulky now, but there is not really anything 
surprising in there.  It probably needs another check of permission 
handling etc., but it's got everything there to try it out.  Along with 
the DDL commands, the pg_dump side is now fully implemented.

Secondly, I isolated the protocol changes into a protocol extension with 
the name _pq_.column_encryption.  So by default there are no protocol 
changes and this feature is disabled.  AFAICT, we haven't actually ever 
used the _pq_ protocol extension mechanism, so it would be good to 
review whether this was done here in the intended way.

At this point, the patch is sort of feature complete, meaning it has all 
the concepts, commands, and interfaces that I had in mind.  I have a 
long list of things to recheck and tighten up, based on earlier feedback 
and some things I found along the way.  But I don't currently plan any 
more major architectural or design changes, pending feedback.  (Also, 
the patch is now very big, so anything additional might be better for a 
future separate patch.)

On 20.07.22 08:12, Masahiko Sawada wrote:
> ---
> Regarding the documentation, I'd like to have a page that describes
> the generic information of the transparent column encryption for users
> such as what this feature actually does, what can be achieved by this
> feature, CMK rotation, and its known limitations. The patch has
> "Transparent Column Encryption" section in protocol.sgml but it seems
> to be more internal information.

I have added more documentation in the v6 patch.

> ---
> In datatype.sgml, it says "Thus, clients that don't support
> transparent column encryption or have disabled it will see the
> encrypted values as byte arrays." but I got an error rather than
> encrypted values when I tried to connect to the server using by
> clients that don't support the encryption:
> 
> postgres(1:6040)=# select * from tbl;
> no CMK lookup found for realm ""

This has now been improved in v6.  The protocol changes need to be 
activated explicitly at connection time, so if you use a client that 
doesn't support it or activates it, you get the described behavior.

> ---
> In single-user mode, the user cannot decrypt the encrypted value but
> probably it's fine in practice.

Yes, there is nothing really to do about that.

> ---
> Regarding the column master key rotation, would it be useful if we
> provide a tool for that? For example, it takes old and new CMK as
> input, re-encrypt all CEKs realted to the CMK, and registers them to
> the server.

I imagine users using a variety of key management systems, so I don't 
see how a single tool would work.  But it's something we can think about 
in the future.

> ---
> Is there any convenient way to load a large amount of test data to the
> encrypted columns? I tried to use generate_series() but it seems not
> to work as it generates the data on the server side:

No, that doesn't work, by design.  You'd have to write a client program 
to generate the data.

> I've also tried to load the data from a file on the client by using
> \copy command, but it seems not to work:
> 
> postgres(1:80556)=# copy (select generate_series(1, 1000)::text) to
> '/tmp/tmp.dat';
> COPY 1000
> postgres(1:80556)=# \copy a from '/tmp/tmp.dat'
> COPY 1000
> postgres(1:80556)=# select * from a;
> out out memory

This was a bug that I have fixed.

> ---
> I got SEGV in the following two situations:

I have fixed these.

On 27.07.22 01:19, Jacob Champion wrote:
>> Now, if we don't have a padding system
>> built into the feature, then that does put even more on the user; it's
>> hard to argue with that.
> Right. If they can even fix it at all. Having a well-documented padding
> feature would not only help mitigate that, it would conveniently hang a
> big sign on the caveats that exist.

I would be interested in learning more about such padding systems.  I 
have done a lot of reading for this development project, and I have 
never come across a cryptographic approach to hide length differences by 
padding.  Of course, padding to the block cipher's block size is already 
part of the process, but that is done out of necessity, not because you 
want to disguise the length.  Are there any other methods?  I'm 
interested to learn more.

On Tue, Aug 30, 2022 at 4:53 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I would be interested in learning more about such padding systems.  I
> have done a lot of reading for this development project, and I have
> never come across a cryptographic approach to hide length differences by
> padding.  Of course, padding to the block cipher's block size is already
> part of the process, but that is done out of necessity, not because you
> want to disguise the length.  Are there any other methods?  I'm
> interested to learn more.

TLS 1.3 has one example. Here is a description from GnuTLS:
https://gnutls.org/manual/html_node/On-Record-Padding.html (Note the
option to turn on constant-time padding; that may not be a good
tradeoff for us if we're focusing on offline attacks.)

Here's a recent paper that claims to formally characterize length
hiding, but it's behind a wall and I haven't read it:
https://dl.acm.org/doi/abs/10.1145/3460120.3484590

I'll try to find more when I get the chance.

--Jacob

Here is an updated patch that resolves some merge conflicts; no 
functionality changes over v6.

On 30.08.22 13:35, Peter Eisentraut wrote:
> Here is an updated patch.
> 
> I mainly spent time on adding a full set of DDL commands for the keys. 
> This made the patch very bulky now, but there is not really anything 
> surprising in there.  It probably needs another check of permission 
> handling etc., but it's got everything there to try it out.  Along with 
> the DDL commands, the pg_dump side is now fully implemented.
> 
> Secondly, I isolated the protocol changes into a protocol extension with 
> the name _pq_.column_encryption.  So by default there are no protocol 
> changes and this feature is disabled.  AFAICT, we haven't actually ever 
> used the _pq_ protocol extension mechanism, so it would be good to 
> review whether this was done here in the intended way.
> 
> At this point, the patch is sort of feature complete, meaning it has all 
> the concepts, commands, and interfaces that I had in mind.  I have a 
> long list of things to recheck and tighten up, based on earlier feedback 
> and some things I found along the way.  But I don't currently plan any 
> more major architectural or design changes, pending feedback.  (Also, 
> the patch is now very big, so anything additional might be better for a 
> future separate patch.)

New version with some merge conflicts resolved, and I have worked to 
resolve several "TODO" items that I had noted in the code.

On 13.09.22 10:27, Peter Eisentraut wrote:
> Here is an updated patch that resolves some merge conflicts; no 
> functionality changes over v6.
> 
> On 30.08.22 13:35, Peter Eisentraut wrote:
>> Here is an updated patch.
>>
>> I mainly spent time on adding a full set of DDL commands for the keys. 
>> This made the patch very bulky now, but there is not really anything 
>> surprising in there.  It probably needs another check of permission 
>> handling etc., but it's got everything there to try it out.  Along 
>> with the DDL commands, the pg_dump side is now fully implemented.
>>
>> Secondly, I isolated the protocol changes into a protocol extension 
>> with the name _pq_.column_encryption.  So by default there are no 
>> protocol changes and this feature is disabled.  AFAICT, we haven't 
>> actually ever used the _pq_ protocol extension mechanism, so it would 
>> be good to review whether this was done here in the intended way.
>>
>> At this point, the patch is sort of feature complete, meaning it has 
>> all the concepts, commands, and interfaces that I had in mind.  I have 
>> a long list of things to recheck and tighten up, based on earlier 
>> feedback and some things I found along the way.  But I don't currently 
>> plan any more major architectural or design changes, pending 
>> feedback.  (Also, the patch is now very big, so anything additional 
>> might be better for a future separate patch.)

Updated version with meson build system support added (for added files 
and new tests).

On 21.09.22 23:37, Peter Eisentraut wrote:
> New version with some merge conflicts resolved, and I have worked to 
> resolve several "TODO" items that I had noted in the code.
> 
> On 13.09.22 10:27, Peter Eisentraut wrote:
>> Here is an updated patch that resolves some merge conflicts; no 
>> functionality changes over v6.
>>
>> On 30.08.22 13:35, Peter Eisentraut wrote:
>>> Here is an updated patch.
>>>
>>> I mainly spent time on adding a full set of DDL commands for the 
>>> keys. This made the patch very bulky now, but there is not really 
>>> anything surprising in there.  It probably needs another check of 
>>> permission handling etc., but it's got everything there to try it 
>>> out.  Along with the DDL commands, the pg_dump side is now fully 
>>> implemented.
>>>
>>> Secondly, I isolated the protocol changes into a protocol extension 
>>> with the name _pq_.column_encryption.  So by default there are no 
>>> protocol changes and this feature is disabled.  AFAICT, we haven't 
>>> actually ever used the _pq_ protocol extension mechanism, so it would 
>>> be good to review whether this was done here in the intended way.
>>>
>>> At this point, the patch is sort of feature complete, meaning it has 
>>> all the concepts, commands, and interfaces that I had in mind.  I 
>>> have a long list of things to recheck and tighten up, based on 
>>> earlier feedback and some things I found along the way.  But I don't 
>>> currently plan any more major architectural or design changes, 
>>> pending feedback.  (Also, the patch is now very big, so anything 
>>> additional might be better for a future separate patch.)

Hi,

On 2022-09-27 15:51:25 +0200, Peter Eisentraut wrote:
> Updated version with meson build system support added (for added files and
> new tests).

This fails on windows: https://cirrus-ci.com/task/6151847080624128


https://api.cirrus-ci.com/v1/artifact/task/6151847080624128/testrun/build/testrun/column_encryption/001_column_encryption/log/regress_log_001_column_encryption

psql error: stderr: 'OPENSSL_Uplink(00007FFC165CBD50,08): no OPENSSL_Applink'

Greetings,

Andres Freund

On 02.10.22 09:16, Andres Freund wrote:
> On 2022-09-27 15:51:25 +0200, Peter Eisentraut wrote:
>> Updated version with meson build system support added (for added files and
>> new tests).
> 
> This fails on windows: https://cirrus-ci.com/task/6151847080624128
> 
>
https://api.cirrus-ci.com/v1/artifact/task/6151847080624128/testrun/build/testrun/column_encryption/001_column_encryption/log/regress_log_001_column_encryption
> 
> psql error: stderr: 'OPENSSL_Uplink(00007FFC165CBD50,08): no OPENSSL_Applink'

What in the world is that about?  What scant information I could find 
suggests that it has something to do with building a "release" build 
against an "debug" build of the openssl library, or vice versa.  But 
this patch doesn't introduce any use of openssl that we haven't seen before.

Hi,

On 2022-10-06 16:25:51 +0200, Peter Eisentraut wrote:
> On 02.10.22 09:16, Andres Freund wrote:
> > On 2022-09-27 15:51:25 +0200, Peter Eisentraut wrote:
> > > Updated version with meson build system support added (for added files and
> > > new tests).
> > 
> > This fails on windows: https://cirrus-ci.com/task/6151847080624128
> > 
> >
https://api.cirrus-ci.com/v1/artifact/task/6151847080624128/testrun/build/testrun/column_encryption/001_column_encryption/log/regress_log_001_column_encryption
> > 
> > psql error: stderr: 'OPENSSL_Uplink(00007FFC165CBD50,08): no OPENSSL_Applink'
> 
> What in the world is that about?  What scant information I could find
> suggests that it has something to do with building a "release" build against
> an "debug" build of the openssl library, or vice versa.  But this patch
> doesn't introduce any use of openssl that we haven't seen before.

It looks to me that one needs to compile, in some form, openssl/applink.c and
link it to the application. No idea why that'd be required now and not
earlier.

Greetings,

Andres Freund

On 06.10.22 17:19, Andres Freund wrote:
>>> psql error: stderr: 'OPENSSL_Uplink(00007FFC165CBD50,08): no OPENSSL_Applink'
>> What in the world is that about?  What scant information I could find
>> suggests that it has something to do with building a "release" build against
>> an "debug" build of the openssl library, or vice versa.  But this patch
>> doesn't introduce any use of openssl that we haven't seen before.
> It looks to me that one needs to compile, in some form, openssl/applink.c and
> link it to the application. No idea why that'd be required now and not
> earlier.

I have figured this out.  The problem is that on Windows you can't 
reliably pass stdio FILE * handles between the application and OpenSSL. 
To give the helpful places I found some Google juice, I'll mention them 
here:

- https://github.com/edenhill/librdkafka/pull/3602
- https://github.com/edenhill/librdkafka/issues/3554
- https://www.mail-archive.com/openssl-users@openssl.org/msg91029.html

Here is an updated version with the tests on Windows working again, and 
some typos fixed.

On 27.09.22 15:51, Peter Eisentraut wrote:
> Updated version with meson build system support added (for added files 
> and new tests).
> 
> On 21.09.22 23:37, Peter Eisentraut wrote:
>> New version with some merge conflicts resolved, and I have worked to 
>> resolve several "TODO" items that I had noted in the code.
>>
>> On 13.09.22 10:27, Peter Eisentraut wrote:
>>> Here is an updated patch that resolves some merge conflicts; no 
>>> functionality changes over v6.
>>>
>>> On 30.08.22 13:35, Peter Eisentraut wrote:
>>>> Here is an updated patch.
>>>>
>>>> I mainly spent time on adding a full set of DDL commands for the 
>>>> keys. This made the patch very bulky now, but there is not really 
>>>> anything surprising in there.  It probably needs another check of 
>>>> permission handling etc., but it's got everything there to try it 
>>>> out.  Along with the DDL commands, the pg_dump side is now fully 
>>>> implemented.
>>>>
>>>> Secondly, I isolated the protocol changes into a protocol extension 
>>>> with the name _pq_.column_encryption.  So by default there are no 
>>>> protocol changes and this feature is disabled.  AFAICT, we haven't 
>>>> actually ever used the _pq_ protocol extension mechanism, so it 
>>>> would be good to review whether this was done here in the intended way.
>>>>
>>>> At this point, the patch is sort of feature complete, meaning it has 
>>>> all the concepts, commands, and interfaces that I had in mind.  I 
>>>> have a long list of things to recheck and tighten up, based on 
>>>> earlier feedback and some things I found along the way.  But I don't 
>>>> currently plan any more major architectural or design changes, 
>>>> pending feedback.  (Also, the patch is now very big, so anything 
>>>> additional might be better for a future separate patch.)

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:

    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;
     }

# 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.

  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.

# 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 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...

# 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 the
    known CEK/CMK might be useful as well.

  * about write queries using psql, would it be possible to use psql
    parameters? Eg.:

      => \set c1 myval
      => INSERT INTO mytable VALUES (:'c1') \gencr

# 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.

  * 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

  * 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?

  * 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,

Hi,

Here are a few more things I noticed :

If a CEK is encrypted with cmk1 and cmk2, but cmk1 isn't found on the 
client,the following error is printed twice for the very first SELECT 
statement:

   could not open file "/path/to/cmk1.pem": No such file or directory

...and nothing is returned. The next queries in the same session would 
work  correctly (cmk2 is used for the decryption of the CEK). An INSERT 
statement si handled properly, though : one (and only one) error 
message, and line actually inserted in all cases).

For example :

   postgres=# SELECT * FROM customers ;
   could not open file "/path/to/cmk1.pem": No such file or directory

   could not open file "/path/to/cmk1.pem": No such file or directory

   postgres=# SELECT * FROM customers ;
    id | name  | creditcard_num
   ----+-------+-----------------
     1 | toto  | 546843351354245
     2 | babar | 546843351354245

<close and open new psql session>

   postgres=# INSERT INTO customers (id, name, creditcard_num) VALUES 
  ($1, $2, $3) \gencr '3' 'toto' '546888351354245';
   could not open file "/path/to/cmk1.pem": No such file or directory

   INSERT 0 1
   postgres=# SELECT * FROM customers ;
    id | name  | creditcard_num
   ----+-------+-----------------
     1 | toto  | 546843351354245
     2 | babar | 546843351354245
     3 | toto  | 546888351354245


 From the documentation of CREATE COLUMN MASTER KEY, it looks like the 
REALM is optional, but both
   CREATE COLUMN MASTER KEY cmk1;
and
   CREATE COLUMN MASTER KEY cmk1 WITH ();
returns a syntax error.


About AEAD, the documentation says :
 > The “associated data” in these algorithms consists of 4 bytes: The 
ASCII letters P and G (byte values 80 and 71), followed by the algorithm 
ID as a 16-bit unsigned integer in network byte order.

My guess is that it serves no real purpose, did I misunderstand ?

Here is another updated patch.  Some preliminary work was committed, 
which allowed this patch to get a bit smaller.  I have incorporated some 
recent reviews, and also fixed some issues pointed out by recent CI 
additions (address sanitizer etc.).

The psql situation in this patch is temporary: It still has the \gencr 
command from previous versions, but I plan to fold this into the new 
\bind command.


On 14.10.22 08:27, Peter Eisentraut wrote:
> Here is an updated version with the tests on Windows working again, and 
> some typos fixed.
> 
> On 27.09.22 15:51, Peter Eisentraut wrote:
>> Updated version with meson build system support added (for added files 
>> and new tests).
>>
>> On 21.09.22 23:37, Peter Eisentraut wrote:
>>> New version with some merge conflicts resolved, and I have worked to 
>>> resolve several "TODO" items that I had noted in the code.
>>>
>>> On 13.09.22 10:27, Peter Eisentraut wrote:
>>>> Here is an updated patch that resolves some merge conflicts; no 
>>>> functionality changes over v6.
>>>>
>>>> On 30.08.22 13:35, Peter Eisentraut wrote:
>>>>> Here is an updated patch.
>>>>>
>>>>> I mainly spent time on adding a full set of DDL commands for the 
>>>>> keys. This made the patch very bulky now, but there is not really 
>>>>> anything surprising in there.  It probably needs another check of 
>>>>> permission handling etc., but it's got everything there to try it 
>>>>> out.  Along with the DDL commands, the pg_dump side is now fully 
>>>>> implemented.
>>>>>
>>>>> Secondly, I isolated the protocol changes into a protocol extension 
>>>>> with the name _pq_.column_encryption.  So by default there are no 
>>>>> protocol changes and this feature is disabled.  AFAICT, we haven't 
>>>>> actually ever used the _pq_ protocol extension mechanism, so it 
>>>>> would be good to review whether this was done here in the intended 
>>>>> way.
>>>>>
>>>>> At this point, the patch is sort of feature complete, meaning it 
>>>>> has all the concepts, commands, and interfaces that I had in mind.  
>>>>> I have a long list of things to recheck and tighten up, based on 
>>>>> earlier feedback and some things I found along the way.  But I 
>>>>> don't currently plan any more major architectural or design 
>>>>> changes, pending feedback.  (Also, the patch is now very big, so 
>>>>> anything additional might be better for a future separate patch.)

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.

On Wed, 23 Nov 2022 19:45:06 +0100
Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> On 28.10.22 12:16, Jehan-Guillaume de Rorthais wrote:
[...]

> >    * 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?

I meant even when column_encryption is turned on.

Regards,

On 23.11.22 19:39, Peter Eisentraut wrote:
> Here is another updated patch.  Some preliminary work was committed, 
> which allowed this patch to get a bit smaller.  I have incorporated some 
> recent reviews, and also fixed some issues pointed out by recent CI 
> additions (address sanitizer etc.).
> 
> The psql situation in this patch is temporary: It still has the \gencr 
> command from previous versions, but I plan to fold this into the new 
> \bind command.

I made a bit of progress with this now, based on recent reviews:

- Cleaned up the libpq API.  PQexecParams() now supports column 
encryption transparently.
- psql \bind can be used; \gencr is removed.
- Added psql \dcek and \dcmk commands.
- ALTER COLUMN MASTER KEY to alter realm.

On 28.11.22 15:05, Peter Eisentraut wrote:
> On 23.11.22 19:39, Peter Eisentraut wrote:
>> Here is another updated patch.  Some preliminary work was committed, 
>> which allowed this patch to get a bit smaller.  I have incorporated 
>> some recent reviews, and also fixed some issues pointed out by recent 
>> CI additions (address sanitizer etc.).
>>
>> The psql situation in this patch is temporary: It still has the \gencr 
>> command from previous versions, but I plan to fold this into the new 
>> \bind command.
> 
> I made a bit of progress with this now, based on recent reviews:
> 
> - Cleaned up the libpq API.  PQexecParams() now supports column 
> encryption transparently.
> - psql \bind can be used; \gencr is removed.
> - Added psql \dcek and \dcmk commands.
> - ALTER COLUMN MASTER KEY to alter realm.

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.

According to my local todo list, this patch is now complete.

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.
> 
> According to my local todo list, this patch is now complete.

Another update, with some merge conflicts resolved.  I also fixed up the 
remaining TODO markers in the code, which had something to do with Perl 
and Windows.  I did some more work on schema handling, e.g., CREATE 
TABLE / LIKE, views, partitioning etc. on top of encrypted columns, 
mostly tedious and repetitive, nothing interesting.  I also rewrote the 
code that extracts the underlying tables and columns corresponding to 
query parameters.  It's now much simpler and better encapsulated.

"ON (CASE WHEN a.attrealtypid <> 0 THEN a.attrealtypid ELSE a.atttypid END = t.oid)\n"

This breaks interoperability with older servers:
ERROR:  column a.attrealtypid does not exist

Same in describe.c

Find attached some typos and bad indentation.  I'm sending this off now
as I've already sat on it for 2 weeks since starting to look at the
patch.

-- 
Justin

> On Dec 31, 2022, at 6:17 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> Another update, with some merge conflicts resolved.

Hi Peter, thanks for the patch!

I wonder if logical replication could be made to work more easily with this feature.  Specifically, subscribers of
encryptedcolumns will need the encrypted column encryption key (CEK) and the name of the column master key (CMD) as
existson the publisher, but getting access to that is not automated as far as I can see. It doesn't come through
automaticallyas part of a subscription, and publisher's can't publish the pg_catalog tables where the keys are kept
(becausepublishing system tables is not supported.)  Is it reasonable to make available the CEK and CMK to subscribers
inan automated fashion, to facilitate setting up logical replication with less manual distribution of key information?
Isthis already done, and I'm just not recognizing that you've done it? 


Can we do anything about the attack vector wherein a malicious DBA simply copies the encrypted datum from one row to
another? Imagine the DBA Alice wants to murder a hospital patient Bob by removing the fact that Bob is deathly allergic
tolatex.  She cannot modify the Bob's encrypted and authenticated record, but she can easily update Bob's record with
theencrypted record of a different patient Charlie.  Likewise, if she want's Bob to pay Charlie's bill, she can replace
Charlie'sencrypted credit card number with Bob's, and once Bob is dead, he won't dispute the charges. 

An encrypted-and-authenticated column value should be connected with its row in some way that Alice cannot circumvent.
Inthe patch as you have it written, the client application can include row information in the patient record
(specifically,the patient's name, ssn, etc) and verify when any patient record is retrieved that this information
matches. But that's hardly "transparent" to the client.  It's something all clients will have to do, and easy to forget
todo in some code path.  Also, for encrypted fixed-width columns, it is not an option.  So it seems the client needs to
"salt"(maybe not the right term for what I have in mind) the encryption with some relevant other columns, and that's
somethingthe libpq client would need to understand, and something the patch's syntax needs to support.  Something like: 

CREATE TABLE patient_records (
    -- Cryptographically connected to the encrypted record
    patient_id  BIGINT NOT NULL,
    patient_ssn CHAR(11),

    -- The encrypted record
    patient_record TEXT ENCRYPTED WITH (column_encryption_key = cek1,
                                        column_encryption_salt = (patient_id, patient_ssn)),

    -- Extra stuff, not cryptographically connected to anything
    next_of_kin TEXT,
    phone_number BIGINT,
    ...
);

I have not selected any algorithms that include such "salt"ing (again, maybe the wrong word) because I'm just trying to
discussthe general feature, not get into the weeds about which cryptographic algorithm to select. 

Thoughts?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Jan 10, 2023, at 9:26 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>    -- Cryptographically connected to the encrypted record
>    patient_id  BIGINT NOT NULL,
>    patient_ssn CHAR(11),
>
>    -- The encrypted record
>    patient_record TEXT ENCRYPTED WITH (column_encryption_key = cek1,
>                                        column_encryption_salt = (patient_id, patient_ssn)),

As you mention upthread, tying columns together creates problems for statements that only operate on a subset of
columns. Allowing schema designers a choice about tying the encrypted column to zero or more other columns allows them
tochoose which works best for their security needs. 

The example above would make a statement like "UPDATE patient_record SET patient_record = $1 \bind '{some json
whatever}'"raise an exception at the libpq client level, but maybe that's what schema designers wants it to do.  If
not,they should omit the column_encryption_salt option in the create table statement; but if so, they should expect to
haveto specify the other columns as part of the update statement, possibly as part of the where clause, like 

    UPDATE patient_record
        SET patient_record = $1
        WHERE patient_id = 12345
          AND patient_ssn = '111-11-1111'
        \bind '{some json record}'

and have the libpq get the salt column values from the where clause (which may be tricky to implement), or perhaps use
somenew bind syntax like 

    UPDATE patient_record
        SET patient_record = ($1:$2,$3)   -- new, wonky syntax
        WHERE patient_id = $2
          AND patient_ssn = $3
        \bind '{some json record}' 12345 '111-11-1111'

which would be error prone, since the sql statement could specify the ($1:$2,$3) inconsistently with the where clause,
orperhaps specify the "new" salt columns even when not changed, like 

    UPDATE patient_record
        SET patient_record = $1, patient_id = 12345, patient_ssn = "111-11-1111"
        WHERE patient_id = 12345
          AND patient_ssn = "111-11-1111"
        \bind '{some json record}'

which looks kind of nuts at first glance, but is grammatically consistent with cases where one or both of the
patient_idor patient_ssn are also being changed, like 

    UPDATE patient_record
        SET patient_record = $1, patient_id = 98765, patient_ssn = "999-99-9999"
        WHERE patient_id = 12345
          AND patient_ssn = "111-11-1111"
        \bind '{some json record}'

Or, of course, you can ignore these suggestions or punt them to some future patch that extends the current work, rather
thantrying to get it all done in the first column encryption commit.  But it seems useful to think about what future
directionswould be, to avoid coding ourselves into a corner, making such future work harder. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, 31 Dec 2022 at 19:47, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> 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.
> >
> > According to my local todo list, this patch is now complete.
>
> Another update, with some merge conflicts resolved.  I also fixed up the
> remaining TODO markers in the code, which had something to do with Perl
> and Windows.  I did some more work on schema handling, e.g., CREATE
> TABLE / LIKE, views, partitioning etc. on top of encrypted columns,
> mostly tedious and repetitive, nothing interesting.  I also rewrote the
> code that extracts the underlying tables and columns corresponding to
> query parameters.  It's now much simpler and better encapsulated.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
5f6401f81cb24bd3930e0dc589fc4aa8b5424cdc ===
=== applying patch ./v14-0001-Transparent-column-encryption.patch
....
Hunk #1 FAILED at 1109.
....
1 out of 5 hunks FAILED -- saving rejects to file doc/src/sgml/protocol.sgml.rej
....
patching file doc/src/sgml/ref/create_table.sgml
Hunk #3 FAILED at 351.
Hunk #4 FAILED at 704.
2 out of 4 hunks FAILED -- saving rejects to file
doc/src/sgml/ref/create_table.sgml.rej
....
Hunk #1 FAILED at 1420.
Hunk #2 FAILED at 4022.
2 out of 2 hunks FAILED -- saving rejects to file
doc/src/sgml/ref/psql-ref.sgml.rej

[1] - http://cfbot.cputube.org/patch_41_3718.log

Regards,
Vignesh

On 10.01.23 18:26, Mark Dilger wrote:
> I wonder if logical replication could be made to work more easily with this feature.  Specifically, subscribers of
encryptedcolumns will need the encrypted column encryption key (CEK) and the name of the column master key (CMD) as
existson the publisher, but getting access to that is not automated as far as I can see. It doesn't come through
automaticallyas part of a subscription, and publisher's can't publish the pg_catalog tables where the keys are kept
(becausepublishing system tables is not supported.)  Is it reasonable to make available the CEK and CMK to subscribers
inan automated fashion, to facilitate setting up logical replication with less manual distribution of key information?
Isthis already done, and I'm just not recognizing that you've done it?
 

This would be done as part of DDL replication.

> Can we do anything about the attack vector wherein a malicious DBA simply copies the encrypted datum from one row to
another?

We discussed this earlier [0].  This patch is not that feature.  We 
could get there eventually, but it would appear to be an immense amount 
of additional work.  We have to start somewhere.


[0]: 
https://www.postgresql.org/message-id/4fbcf5540633699fc3d81ffb59cb0ac884673a7c.camel@vmware.com

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

On 11.01.23 17:46, vignesh C wrote:
> On Sat, 31 Dec 2022 at 19:47, Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> 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.
>>>
>>> According to my local todo list, this patch is now complete.
>>
>> Another update, with some merge conflicts resolved.  I also fixed up the
>> remaining TODO markers in the code, which had something to do with Perl
>> and Windows.  I did some more work on schema handling, e.g., CREATE
>> TABLE / LIKE, views, partitioning etc. on top of encrypted columns,
>> mostly tedious and repetitive, nothing interesting.  I also rewrote the
>> code that extracts the underlying tables and columns corresponding to
>> query parameters.  It's now much simpler and better encapsulated.
> 
> The patch does not apply on top of HEAD as in [1], please post a rebased patch:

Here is a new patch.  Changes since v14:

- Fixed some typos (review by Justin Pryzby)
- Fixed backward compat. psql and pg_dump (review by Justin Pryzby)
- Doc additions (review by Jacob Champion)
- Validate column_encryption option in libpq (review by Jacob Champion)
- Handle column encryption in inheritance
- Change CEKs and CMKs to live inside schemas

On 07.01.23 01:34, Justin Pryzby wrote:
> "ON (CASE WHEN a.attrealtypid <> 0 THEN a.attrealtypid ELSE a.atttypid END = t.oid)\n"
> 
> This breaks interoperability with older servers:
> ERROR:  column a.attrealtypid does not exist
> 
> Same in describe.c
> 
> Find attached some typos and bad indentation.  I'm sending this off now
> as I've already sat on it for 2 weeks since starting to look at the
> patch.

Thanks, I have integrated all that into the v15 patch I just posted.

On 12.01.23 17:32, Peter Eisentraut wrote:
>> Can we do anything about the attack vector wherein a malicious DBA 
>> simply copies the encrypted datum from one row to another?
> 
> We discussed this earlier [0].  This patch is not that feature.  We 
> could get there eventually, but it would appear to be an immense amount 
> of additional work.  We have to start somewhere.

I've been thinking, this could be done as a "version 2" of the currently 
proposed feature, within the same framework.  We'd extend the 
RowDescription and ParameterDescription messages to provide primary key 
information, some flags, then the client would have enough to know what 
to do.  As you wrote in your follow-up message, a challenge would be to 
handle statements that do not touch all the columns.  We'd need to work 
through this and consider all the details.

On 19.01.23 21:48, Jacob Champion wrote:
> 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.)

I have added those in the v15 patch I just posted.

> 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.

I had started coding that, but one problem was that the openssl CLI 
doesn't really provide any means to work with those kinds of keys.  The 
"openssl enc" command always wants to mix in a password.  Without that, 
there is no way to write a test case, and more crucially no way for 
users to set up these kinds of keys.  Unless we write our own tooling 
for this, which, you know, the patch just passed 400k in size.

> 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.

Heh.  I have removed that parenthetical remark.

> 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?

Not really.  I was also initially confused by this setup, but that's how 
other similar systems are set up, so I thought it would be confusing to 
do it differently.

> 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?

\gencr didn't do that either.  We could do it.  The libpq API supports 
it.  We just need to come up with some syntax for psql.

> 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?

fixed in v15

> 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.

This is documented in the protocol chapter, which I thought was the 
right place.  Did you want more documentation, or in a different place?

> 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?

Well, we can ask them to, but we can't really require them, in a 
cryptographic sense.  I'm not sure what more we can do.

> 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?

I don't completely follow this.

On Wed, Jan 25, 2023 at 11:00 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> > 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.
>
> I had started coding that, but one problem was that the openssl CLI
> doesn't really provide any means to work with those kinds of keys.  The
> "openssl enc" command always wants to mix in a password.  Without that,
> there is no way to write a test case, and more crucially no way for
> users to set up these kinds of keys.  Unless we write our own tooling
> for this, which, you know, the patch just passed 400k in size.

Arrgh: https://github.com/openssl/openssl/issues/10605

> > 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?
>
> Not really.  I was also initially confused by this setup, but that's how
> other similar systems are set up, so I thought it would be confusing to
> do it differently.

Which systems let you mix and match keys and algorithms this way? I'd
like to take a look at them.

> > 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?
>
> \gencr didn't do that either.  We could do it.  The libpq API supports
> it.  We just need to come up with some syntax for psql.

Do you think people would rather set encryption for all parameters at
once -- something like \encbind -- or have the ability to mix
encrypted and unencrypted parameters?

> > 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.
>
> This is documented in the protocol chapter, which I thought was the
> right place.  Did you want more documentation, or in a different place?

I just missed it; sorry.

> > 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?
>
> Well, we can ask them to, but we can't really require them, in a
> cryptographic sense.  I'm not sure what more we can do.

Right -- I just mean that clients need to pay more attention to it
now, whereas before they may have delegated correctness to the server.
The problem is documented in the context of deterministic encryption,
but I think it applies to randomized as well.

More concretely: should psql allow you to push arbitrary text into an
encrypted \bind parameter, like it does now?

> > 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?
>
> I don't completely follow this.

Yeah, I'm not expressing it very well. My feeling is that the
organization system here -- a realm "contains" multiple CMKs, a CMK
encrypts multiple CEKs -- is so general and flexible that it may need
some suggested guardrails for people to use it sanely. I just don't
know what those guardrails should be. I was motivated by the
realization that CEKs don't even need to be keys.

Thanks,
--Jacob

On 30.01.23 23:30, Jacob Champion wrote:
>>> 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?
>>
>> Not really.  I was also initially confused by this setup, but that's how
>> other similar systems are set up, so I thought it would be confusing to
>> do it differently.
> 
> Which systems let you mix and match keys and algorithms this way? I'd
> like to take a look at them.

See here for example: 

https://learn.microsoft.com/en-us/sql/relational-databases/security/encryption/always-encrypted-database-engine?view=sql-server-ver15

>>> 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?
>>
>> \gencr didn't do that either.  We could do it.  The libpq API supports
>> it.  We just need to come up with some syntax for psql.
> 
> Do you think people would rather set encryption for all parameters at
> once -- something like \encbind -- or have the ability to mix
> encrypted and unencrypted parameters?

For pg_dump, I'd like a mode that makes all values parameters of an 
INSERT statement.  But obviously not all of those will be encrypted.  So 
I think we'd want a per-parameter syntax.

> More concretely: should psql allow you to push arbitrary text into an
> encrypted \bind parameter, like it does now?

We don't have any data type awareness like that now in psql or libpq. 
It would be quite a change to start now.  How would that deal with data 
type extensibility, is an obvious question to start with.  Don't know.

On Tue, Jan 31, 2023 at 5:26 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> See here for example:
>
https://learn.microsoft.com/en-us/sql/relational-databases/security/encryption/always-encrypted-database-engine?view=sql-server-ver15

Hm. I notice they haven't implemented more than one algorithm, so I
wonder if they're going to be happy with their decision to
mix-and-match when number two arrives.

> For pg_dump, I'd like a mode that makes all values parameters of an
> INSERT statement.  But obviously not all of those will be encrypted.  So
> I think we'd want a per-parameter syntax.

Makes sense. Maybe something that defaults to encrypted with opt-out
per parameter?

    UPDATE t SET name = $1 WHERE id = $2
        \encbind "Jacob" plaintext(24)

> > More concretely: should psql allow you to push arbitrary text into an
> > encrypted \bind parameter, like it does now?
>
> We don't have any data type awareness like that now in psql or libpq.
> It would be quite a change to start now.

I agree. It just feels weird that a misbehaving client can "attack"
the other client implementations using it, and we don't make any
attempt to mitigate it.

--Jacob

> On Jan 25, 2023, at 10:44 AM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> Here is a new patch.  Changes since v14:
>
> - Fixed some typos (review by Justin Pryzby)
> - Fixed backward compat. psql and pg_dump (review by Justin Pryzby)
> - Doc additions (review by Jacob Champion)
> - Validate column_encryption option in libpq (review by Jacob Champion)
> - Handle column encryption in inheritance
> - Change CEKs and CMKs to live inside schemas<v15-0001-Transparent-column-encryption.patch>

Thanks Peter.  Here are some observations about the documentation in patch version 15.

In acronyms.sgml, the CEK and CMK entries should link to documentation, perhaps
linkend="glossary-column-encryption-key"and linkend="glossary-column-master-key".  These glossary entries should in
turnlink to linkend="ddl-column-encryption". 

In ddl.sgml, the sentence "forcing encryption of certain parameters in the client library (see its documentation)"
shouldlink to linkend="libpq-connect-column-encryption". 

Did you intend the use of "transparent data encryption" (rather than "transparent column encryption") in datatype.sgml?
If so, what's the difference? 

Is this feature intended to be available from ecpg?  If so, can we maybe include an example in 36.3.4. Prepared
Statementsshowing how to pass the encrypted values securely.  If not, can we include verbiage about that limitation, so
folksdon't waste time trying to figure out how to do it? 

The documentation for pg_dump (and pg_dumpall) now includes a --decrypt-encrypted-columns option, which I suppose
requirescmklookup to first be configured, and for PGCMKLOOKUP to be exported.  There isn't anything in the pg_dump docs
aboutthis, though, so maybe a link to section 5.5.3 with a warning about not running pg_dump this way on the database
serveritself? 

How does a psql user mark a parameter as having forced encryption?  A libpq user can specify this in the paramFormats
array,but I don't see any syntax for doing this from psql.  None of the perl tap tests you've included appear to do
this(except indirectly when calling test_client); grep'ing for the libpq error message "parameter with forced
encryptionis not to be encrypted" in the tests has no matches.  Is it just not possible?  I thought you'd mentioned
somesyntax for this when we spoke in person, but I don't see it now. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

> On Feb 11, 2023, at 1:54 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> Here are some observations

I should mention, src/sgml/html/libpq-exec.html needs clarification:

> paramFormats[]Specifies whether parameters are text (put a zero in the array entry for the corresponding parameter)
orbinary (put a one in the array entry for the corresponding parameter). If the array pointer is null then all
parametersare presumed to be text strings. 

Perhaps you should edit this last sentence to say that all parameters are presumed to be test strings without forced
encryption.

> Values passed in binary format require knowledge of the internal representation expected by the backend. For example,
integersmust be passed in network byte order. Passing numeric values requires knowledge of the server storage format,
asimplemented in src/backend/utils/adt/numeric.c::numeric_send() and src/backend/utils/adt/numeric.c::numeric_recv(). 

> When column encryption is enabled, the second-least-significant byte of this parameter specifies whether encryption
shouldbe forced for a parameter. 

The value 0x10 has a one as its second-least-significant *nibble*, but that's a really strange way to describe the
high-ordernibble, and perhaps not what you mean.  Could you clarify? 

> Set this byte to one to force encryption.

I think setting the byte to one (0x01) means "binary unencrypted", not "force encryption".  Don't you mean to set this
byteto 16?  

> For example, use the C code literal 0x10 to specify text format with forced encryption. If the array pointer is null
thenencryption is not forced for any parameter. 
> If encryption is forced for a parameter but the parameter does not correspond to an encrypted column on the server,
thenthe call will fail and the parameter will not be sent. This can be used for additional security against a
compromisedserver. (The drawback is that application code then needs to be kept up to date with knowledge about which
columnsare encrypted rather than letting the server specify this.) 

 I think you should say something about how specifying 0x11 will behave -- in other words, asking for encrypted binary
data. I believe that this is will draw a "format must be text for encrypted parameter" error, and that the docs should
clearlysay so. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

New patch.

Per some feedback, I have renamed this feature.  People didn't like the 
"transparent", for various reasons.  The new name I came up with is 
"automatic client-side column-level encryption".  This also matches the 
terminology used in other products better.  (Maybe the acronym ACSCLE -- 
pronounced "a chuckle" -- will catch on.)  I'm also using various 
subsets of that name when the context is clear.

Other changes since v15:

- CEKs and CMKs now have USAGE privileges.  (There are some TODO markers 
where I got too bored with boilerplate.  I will fill those in, but the 
idea should be clear.)

- Renamed attrealtypid to attusertypid.  (It wasn't really "real".)
- Added corresponding attusertypmod.
- Removed attencalg, it's now stored in atttypmod.
(The last three together make the whole attribute storage work more 
sensibly and smoothly.)

- Various documentation changes (review by Mark Dilger)
- Added more explicit documentation that this feature is not to protect 
against an "evil DBA".

On 11.02.23 22:54, Mark Dilger wrote:
> Thanks Peter.  Here are some observations about the documentation in patch version 15.
> 
> In acronyms.sgml, the CEK and CMK entries should link to documentation, perhaps
linkend="glossary-column-encryption-key"and linkend="glossary-column-master-key".  These glossary entries should in
turnlink to linkend="ddl-column-encryption".
 
> 
> In ddl.sgml, the sentence "forcing encryption of certain parameters in the client library (see its documentation)"
shouldlink to linkend="libpq-connect-column-encryption".
 
> 
> Did you intend the use of "transparent data encryption" (rather than "transparent column encryption") in
datatype.sgml? If so, what's the difference?
 

There are all addressed in the v16 patch I just posted.

> Is this feature intended to be available from ecpg?  If so, can we maybe include an example in 36.3.4. Prepared
Statementsshowing how to pass the encrypted values securely.  If not, can we include verbiage about that limitation, so
folksdon't waste time trying to figure out how to do it?
 

It should "just work".  I will give this a try sometime, but I don't see 
why it wouldn't work.

> The documentation for pg_dump (and pg_dumpall) now includes a --decrypt-encrypted-columns option, which I suppose
requirescmklookup to first be configured, and for PGCMKLOOKUP to be exported.  There isn't anything in the pg_dump docs
aboutthis, though, so maybe a link to section 5.5.3 with a warning about not running pg_dump this way on the database
serveritself?
 

Also addressed in v16.

> How does a psql user mark a parameter as having forced encryption?  A libpq user can specify this in the paramFormats
array,but I don't see any syntax for doing this from psql.  None of the perl tap tests you've included appear to do
this(except indirectly when calling test_client); grep'ing for the libpq error message "parameter with forced
encryptionis not to be encrypted" in the tests has no matches.  Is it just not possible?  I thought you'd mentioned
somesyntax for this when we spoke in person, but I don't see it now.
 

This has been asked about before.  We just need to come up with a syntax 
for it.  The issue is contained inside psql.

On 12.02.23 15:48, Mark Dilger wrote:
> I should mention, src/sgml/html/libpq-exec.html needs clarification:
> 
>> paramFormats[]Specifies whether parameters are text (put a zero in the array entry for the corresponding parameter)
orbinary (put a one in the array entry for the corresponding parameter). If the array pointer is null then all
parametersare presumed to be text strings.
 
> 
> Perhaps you should edit this last sentence to say that all parameters are presumed to be test strings without forced
encryption.

This is actually already mentioned later in that section.

>> When column encryption is enabled, the second-least-significant byte of this parameter specifies whether encryption
shouldbe forced for a parameter.
 
> 
> The value 0x10 has a one as its second-least-significant *nibble*, but that's a really strange way to describe the
high-ordernibble, and perhaps not what you mean.  Could you clarify?
 
> 
>> Set this byte to one to force encryption.
> 
> I think setting the byte to one (0x01) means "binary unencrypted", not "force encryption".  Don't you mean to set
thisbyte to 16?
 
> 
>> For example, use the C code literal 0x10 to specify text format with forced encryption. If the array pointer is null
thenencryption is not forced for any parameter.
 
>> If encryption is forced for a parameter but the parameter does not correspond to an encrypted column on the server,
thenthe call will fail and the parameter will not be sent. This can be used for additional security against a
compromisedserver. (The drawback is that application code then needs to be kept up to date with knowledge about which
columnsare encrypted rather than letting the server specify this.)
 

This was me being confused.  I adjusted the text to use "half-byte".

>   I think you should say something about how specifying 0x11 will behave -- in other words, asking for encrypted
binarydata.  I believe that this is will draw a "format must be text for encrypted parameter" error, and that the docs
shouldclearly say so.
 

done

On 22.02.23 11:25, Peter Eisentraut wrote:
> Other changes since v15:
> 
> - CEKs and CMKs now have USAGE privileges.  (There are some TODO markers 
> where I got too bored with boilerplate.  I will fill those in, but the 
> idea should be clear.)

New patch.  The above is all filled in now.

I also figured we need support in the DISCARD command to clear the 
session state of what keys have already been sent, for the benefit of 
connection poolers, so I added an option there.

The only thing left on my list for this whole thing is some syntax in 
psql to force encryption for a parameter.  But that could also be done 
as a separate patch.

Here is an updated patch.  I have done some cosmetic polishing and fixed 
a minor Windows-related bug.

In my mind, the patch is complete.

If someone wants to do some in-depth code review, I suggest focusing on 
the following files:

* src/backend/access/common/printtup.c
* src/backend/commands/colenccmds.c
* src/backend/commands/tablecmds.c
* src/backend/parser/parse_param.c
* src/interfaces/libpq/fe-exec.c
* src/interfaces/libpq/fe-protocol3.c
* src/interfaces/libpq/libpq-fe.h

(Most other files are DDL boilerplate or otherwise not too interesting.)

> On Mar 9, 2023, at 11:18 PM, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> Here is an updated patch.

Thanks, Peter.  The patch appears to be in pretty good shape, but I have a few comments and concerns.

CEKIsVisible() and CMKIsVisible() are obviously copied from TSParserIsVisible(), and the code comments weren't fully
updated. Specifically, the phrase "hidden by another parser of the same name" should be updated to not mention
"parser".

Why does get_cmkalg_name() return the string "unspecified" for PG_CMK_UNSPECIFIED, but the next function
get_cmkalg_jwa_name()returns NULL for PG_CMK_UNSPECIFIED?  It seems they would both return NULL, or both return
"unspecified". If there's a reason for the divergence, could you add a code comment to clarify?  BTW,
get_cmkalg_jwa_name()has no test coverage. 

Looking further at code coverage, the new conditional in printsimple_startup() is never tested with
(MyProcPort->column_encryption_enabled),so the block is never entered.  This would seem to be a consequence of backends
likewalsender not using column encryption, which is not terribly surprising, but it got me wondering if you had a
particularclient use case in mind when you added this block? 

The new function pg_encrypted_in() appears totally untested, but I have to wonder if that's because we're not
round-triptesting pg_dump with column encryption...?  The code coverage in pg_dump looks fairly decent, but some column
encryptioncode is not covered. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

On 2023-03-10 08:18:34 +0100, Peter Eisentraut wrote:
> Here is an updated patch.  I have done some cosmetic polishing and fixed a
> minor Windows-related bug.
> 
> In my mind, the patch is complete.
> 
> If someone wants to do some in-depth code review, I suggest focusing on the
> following files:
> 
> * src/backend/access/common/printtup.c

Have you done benchmarks of some simple workloads to verify this doesn't cause
slowdowns (when not using encryption, obviously)? printtup.c is a performance
sensitive portion for simple queries, particularly when they return multiple
columns.

And making tupledescs even wider is likely to have some price, both due to the
increase in memory usage, and due to the lower cache density - and that's code
where we're already hurting noticeably.

Greetings,

Andres Freund

On 11.03.23 19:08, Mark Dilger wrote:
> CEKIsVisible() and CMKIsVisible() are obviously copied from TSParserIsVisible(), and the code comments weren't fully
updated. Specifically, the phrase "hidden by another parser of the same name" should be updated to not mention
"parser".

fixed

> 
> Why does get_cmkalg_name() return the string "unspecified" for PG_CMK_UNSPECIFIED, but the next function
get_cmkalg_jwa_name()returns NULL for PG_CMK_UNSPECIFIED?  It seems they would both return NULL, or both return
"unspecified". If there's a reason for the divergence, could you add a code comment to clarify?
 

Added a comment.

> BTW, get_cmkalg_jwa_name() has no test coverage.

Ok, I'll look into it.

> Looking further at code coverage, the new conditional in printsimple_startup() is never tested with
(MyProcPort->column_encryption_enabled),so the block is never entered.  This would seem to be a consequence of backends
likewalsender not using column encryption, which is not terribly surprising, but it got me wondering if you had a
particularclient use case in mind when you added this block?
 

AFAICT, the relationship between printsimple.c and the replicaton 
protocol is not actually firmly defined anywhere, it just happens that 
they are used together.  So I feel the column encryption mode needs to 
be supported, technically, even if nothing is using it right now.

> The new function pg_encrypted_in() appears totally untested, but I have to wonder if that's because we're not
round-triptesting pg_dump with column encryption...?  The code coverage in pg_dump looks fairly decent, but some column
encryptioncode is not covered.
 

I have added test coverage for pg_encrypted_in() (via a COPY round-trip 
test in under src/test/column_encryption), as well as additional 
coverage in pg_dump and some DDL commands.  I didn't find any obvious 
gaps in test coverage elsewhere.

On 12.03.23 01:11, Andres Freund wrote:
> Have you done benchmarks of some simple workloads to verify this doesn't cause
> slowdowns (when not using encryption, obviously)? printtup.c is a performance
> sensitive portion for simple queries, particularly when they return multiple
> columns.

The additional code isn't used when column encryption is off, so there 
shouldn't be any impact.

Hi,

On 2023-03-13 21:22:29 +0100, Peter Eisentraut wrote:
> On 12.03.23 01:11, Andres Freund wrote:
> > Have you done benchmarks of some simple workloads to verify this doesn't cause
> > slowdowns (when not using encryption, obviously)? printtup.c is a performance
> > sensitive portion for simple queries, particularly when they return multiple
> > columns.
> 
> The additional code isn't used when column encryption is off, so there
> shouldn't be any impact.

It adds branches, and it makes tupledescs wider. In tight spots, such as
printtup, that can hurt, even if the branches aren't ever entered.

Greetings,

Andres Freund

Hi,

On 2023-03-13 13:41:19 -0700, Andres Freund wrote:
> On 2023-03-13 21:22:29 +0100, Peter Eisentraut wrote:
> > On 12.03.23 01:11, Andres Freund wrote:
> > > Have you done benchmarks of some simple workloads to verify this doesn't cause
> > > slowdowns (when not using encryption, obviously)? printtup.c is a performance
> > > sensitive portion for simple queries, particularly when they return multiple
> > > columns.
> >
> > The additional code isn't used when column encryption is off, so there
> > shouldn't be any impact.
>
> It adds branches, and it makes tupledescs wider. In tight spots, such as
> printtup, that can hurt, even if the branches aren't ever entered.

In fact, I do see a noticable, but not huge, regression:

$ cat /tmp/test.sql
SELECT * FROM pg_class WHERE oid = 1247;

c=1;taskset -c 10 pgbench -n -M prepared -c$c -j$c -f /tmp/test.sql -P1 -T10

with the server also pinned to core 1, and turbo boost disabled. Nothing else
is allowed to run on the core, or its hyperthread sibling. This is my setup
for comparing performance with the least noise in general, not related to this
patch.

head:  28495.858509 28823.055643 28731.074311
patch: 28298.498851 28285.426532 28489.359569

A ~1.1% loss.

pipelined 50 statements (pgbench pinned to a different otherwise unused core)
head:  1147.404506 1147.587475 1151.976547
patch: 1126.525708 1122.375337 1119.088734

A ~2.2% loss.

That might not be prohibitive, but it does seem worth analyzing.

Greetings,

Andres Freund

On 13.03.23 22:11, Andres Freund wrote:
>> It adds branches, and it makes tupledescs wider. In tight spots, such as
>> printtup, that can hurt, even if the branches aren't ever entered.
> In fact, I do see a noticable, but not huge, regression:

I tried to reproduce your measurements, but I don't have the CPU 
affinity tools like that on macOS, so the differences were lost in the 
noise.  (The absolute numbers looked very similar to yours.)

I can reproduce a regression if I keep adding more columns to 
pg_attribute, like 8 OID columns does start to show.

I investigated whether I could move some columns to the 
"variable-length" part outside the tuple descriptor, but that would 
require major surgery in heap.c and tablecmds.c (MergeAttributes() ... 
shudder), and also elsewhere, where you currently only have a tuple 
descriptor for looking up stuff.

How do we proceed here?  A lot of user-facing table management stuff 
like compression, statistics, collation, dropped columns, and probably 
future features like column reordering or periods, have to be 
represented in pg_attribute somehow, at least in the current 
architecture.  Do we hope that hardware keeps up with the pace at which 
we add new features?  Do we need to decouple tuple descriptors from 
pg_attribute altogether?  How do we decide what goes into the tuple 
descriptor and what does not?  I'm interested in addressing this, 
because obviously we do want the ability to add more features in the 
future, but I don't know what the direction should be.

Hi,

On 2023-03-16 11:26:46 +0100, Peter Eisentraut wrote:
> On 13.03.23 22:11, Andres Freund wrote:
> > > It adds branches, and it makes tupledescs wider. In tight spots, such as
> > > printtup, that can hurt, even if the branches aren't ever entered.
> > In fact, I do see a noticable, but not huge, regression:
> 
> I tried to reproduce your measurements, but I don't have the CPU affinity
> tools like that on macOS, so the differences were lost in the noise.  (The
> absolute numbers looked very similar to yours.)
> 
> I can reproduce a regression if I keep adding more columns to pg_attribute,
> like 8 OID columns does start to show.
>
> [...]
> How do we proceed here?

Maybe a daft question, but why do we need a separate type and typmod for
encrypted columns? Why isn't the fact that the column is encrypted exactly one
new field, and we use the existing type/typmod fields?


> A lot of user-facing table management stuff like compression, statistics,
> collation, dropped columns, and probably future features like column
> reordering or periods, have to be represented in pg_attribute somehow, at
> least in the current architecture.  Do we hope that hardware keeps up with
> the pace at which we add new features?

Yea, it's not great as is. I think it's also OK to decide that the slowdown is
worth it in some cases - it just has to be a conscious decision, in the open.


> Do we need to decouple tuple descriptors from pg_attribute altogether?

Yes. Very clearly. The amount of memory and runtime we spent on tupledescs is
disproportionate. A second angle is that we build tupledescs way way too
frequently. The executor infers them everywhere, so not even prepared
statements protect against that.


> How do we decide what goes into the tuple descriptor and what does not?  I'm
> interested in addressing this, because obviously we do want the ability to
> add more features in the future, but I don't know what the direction should
> be.

We've had some prior discussion around this, see e.g.
https://postgr.es/m/20210819114435.6r532qbadcsyfscp%40alap3.anarazel.de

Greetings,

Andres Freund

On 16.03.23 17:36, Andres Freund wrote:
> Maybe a daft question, but why do we need a separate type and typmod for
> encrypted columns? Why isn't the fact that the column is encrypted exactly one
> new field, and we use the existing type/typmod fields?

The way this is implemented is that for an encrypted column, the real 
atttypid and atttypmod are one of the encrypted special types 
(pg_encrypted_*).  That way, most of the system doesn't need to care 
about the details of encryption or whatnot, it just unpacks tuples etc. 
by looking at atttypid, atttyplen, etc., and queries on encrypted data 
behave normally by just looking at what operators etc. those types have. 
  This approach heavily contains the number of places that need to know 
about this feature at all.

>> Do we need to decouple tuple descriptors from pg_attribute altogether?
> 
> Yes. Very clearly. The amount of memory and runtime we spent on tupledescs is
> disproportionate. A second angle is that we build tupledescs way way too
> frequently. The executor infers them everywhere, so not even prepared
> statements protect against that.
> 
> 
>> How do we decide what goes into the tuple descriptor and what does not?  I'm
>> interested in addressing this, because obviously we do want the ability to
>> add more features in the future, but I don't know what the direction should
>> be.
> 
> We've had some prior discussion around this, see e.g.
> https://postgr.es/m/20210819114435.6r532qbadcsyfscp%40alap3.anarazel.de

This sounds like a good plan.

Hi,

On 2023-03-21 18:05:15 +0100, Peter Eisentraut wrote:
> On 16.03.23 17:36, Andres Freund wrote:
> > Maybe a daft question, but why do we need a separate type and typmod for
> > encrypted columns? Why isn't the fact that the column is encrypted exactly one
> > new field, and we use the existing type/typmod fields?
> 
> The way this is implemented is that for an encrypted column, the real
> atttypid and atttypmod are one of the encrypted special types
> (pg_encrypted_*).  That way, most of the system doesn't need to care about
> the details of encryption or whatnot, it just unpacks tuples etc. by looking
> at atttypid, atttyplen, etc., and queries on encrypted data behave normally
> by just looking at what operators etc. those types have.  This approach
> heavily contains the number of places that need to know about this feature
> at all.

I get that for the type, but why do we need the typmod duplicated as well?

Greetings,

Andres Freund

On 21.03.23 18:47, Andres Freund wrote:
> On 2023-03-21 18:05:15 +0100, Peter Eisentraut wrote:
>> On 16.03.23 17:36, Andres Freund wrote:
>>> Maybe a daft question, but why do we need a separate type and typmod for
>>> encrypted columns? Why isn't the fact that the column is encrypted exactly one
>>> new field, and we use the existing type/typmod fields?
>>
>> The way this is implemented is that for an encrypted column, the real
>> atttypid and atttypmod are one of the encrypted special types
>> (pg_encrypted_*).  That way, most of the system doesn't need to care about
>> the details of encryption or whatnot, it just unpacks tuples etc. by looking
>> at atttypid, atttyplen, etc., and queries on encrypted data behave normally
>> by just looking at what operators etc. those types have.  This approach
>> heavily contains the number of places that need to know about this feature
>> at all.
> 
> I get that for the type, but why do we need the typmod duplicated as well?

Earlier patch versions didn't do that, but that got really confusing 
about which type the typmod really belonged to, since code currently 
assumes that typid+typmod makes sense.  Earlier patch versions had three 
fields (usertypid, keyid, encalg), and then I changed it to (usertypid, 
usertypmod, keyid) and instead placed the encalg into the real typmod, 
which made everything much cleaner.

On 22.03.23 10:00, Peter Eisentraut wrote:
>> I get that for the type, but why do we need the typmod duplicated as 
>> well?
> 
> Earlier patch versions didn't do that, but that got really confusing 
> about which type the typmod really belonged to, since code currently 
> assumes that typid+typmod makes sense.  Earlier patch versions had three 
> fields (usertypid, keyid, encalg), and then I changed it to (usertypid, 
> usertypmod, keyid) and instead placed the encalg into the real typmod, 
> which made everything much cleaner.

I thought about this some more.  I think we could get rid of 
attusertypmod and just hardcode it as -1.  The idea would be that if you 
ask for an encrypted column of type, say, varchar(500), the server isn't 
able to enforce that anyway, so we could just prohibit specifying a 
nondefault typmod for encrypted columns.

I'm not sure if there are weird types that use typmods in some way where 
this wouldn't work.  But so far I could not think of anything.

I'll look into this some more.

On Thu, Mar 23, 2023 at 9:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> I thought about this some more.  I think we could get rid of
> attusertypmod and just hardcode it as -1.  The idea would be that if you
> ask for an encrypted column of type, say, varchar(500), the server isn't
> able to enforce that anyway, so we could just prohibit specifying a
> nondefault typmod for encrypted columns.
>
> I'm not sure if there are weird types that use typmods in some way where
> this wouldn't work.  But so far I could not think of anything.
>
> I'll look into this some more.

I thought we often treated atttypid, atttypmod, and attcollation as a
trio, these days. It seems a bit surprising that you'd end up adding
columns for two out of the three.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 23.03.23 16:55, Robert Haas wrote:
> On Thu, Mar 23, 2023 at 9:55 AM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> I thought about this some more.  I think we could get rid of
>> attusertypmod and just hardcode it as -1.  The idea would be that if you
>> ask for an encrypted column of type, say, varchar(500), the server isn't
>> able to enforce that anyway, so we could just prohibit specifying a
>> nondefault typmod for encrypted columns.
>>
>> I'm not sure if there are weird types that use typmods in some way where
>> this wouldn't work.  But so far I could not think of anything.
>>
>> I'll look into this some more.
> 
> I thought we often treated atttypid, atttypmod, and attcollation as a
> trio, these days. It seems a bit surprising that you'd end up adding
> columns for two out of the three.

Internally, we use all three.  But for reporting to the client 
(RowDescription message), we only have slots for type and typmod.  We 
could in theory extend the protocol to report the collation as well, but 
it's probably not too interesting.

Hi,

On 2023-03-23 14:54:48 +0100, Peter Eisentraut wrote:
> On 22.03.23 10:00, Peter Eisentraut wrote:
> > > I get that for the type, but why do we need the typmod duplicated as
> > > well?
> > 
> > Earlier patch versions didn't do that, but that got really confusing
> > about which type the typmod really belonged to, since code currently
> > assumes that typid+typmod makes sense.  Earlier patch versions had three
> > fields (usertypid, keyid, encalg), and then I changed it to (usertypid,
> > usertypmod, keyid) and instead placed the encalg into the real typmod,
> > which made everything much cleaner.
> 
> I thought about this some more.  I think we could get rid of attusertypmod
> and just hardcode it as -1.  The idea would be that if you ask for an
> encrypted column of type, say, varchar(500), the server isn't able to
> enforce that anyway, so we could just prohibit specifying a nondefault
> typmod for encrypted columns.

Why not just use typmod for the underlying typmod? It doesn't seem like
encrypted datums will need that? Or are you using it for something important there?

Greetings,

Andres Freund

On 24.03.23 19:12, Andres Freund wrote:
>> I thought about this some more.  I think we could get rid of attusertypmod
>> and just hardcode it as -1.  The idea would be that if you ask for an
>> encrypted column of type, say, varchar(500), the server isn't able to
>> enforce that anyway, so we could just prohibit specifying a nondefault
>> typmod for encrypted columns.
> 
> Why not just use typmod for the underlying typmod? It doesn't seem like
> encrypted datums will need that? Or are you using it for something important there?

Yes, the typmod of encrypted types stores the encryption algorithm.

(Also, mixing a type with the typmod of another type is weird in a 
variety of ways, so this is a quite clean solution.)

Hi,

On 2023-03-29 18:08:29 +0200, Peter Eisentraut wrote:
> On 24.03.23 19:12, Andres Freund wrote:
> > > I thought about this some more.  I think we could get rid of attusertypmod
> > > and just hardcode it as -1.  The idea would be that if you ask for an
> > > encrypted column of type, say, varchar(500), the server isn't able to
> > > enforce that anyway, so we could just prohibit specifying a nondefault
> > > typmod for encrypted columns.
> >
> > Why not just use typmod for the underlying typmod? It doesn't seem like
> > encrypted datums will need that? Or are you using it for something important there?
>
> Yes, the typmod of encrypted types stores the encryption algorithm.

Why isn't that an attribute of pg_colenckey, given that attcek has been added
to pg_attribute?


> (Also, mixing a type with the typmod of another type is weird in a variety
> of ways, so this is a quite clean solution.)

It's not an unrelated type though. It's the actual typmod of the column we're
talking about. I find it a lot less clean to make all non-CEK uses of
pg_attribute pay the price of storing three new fields.

Greetings,

Andres Freund

On 29.03.23 18:24, Andres Freund wrote:
> Hi,
> 
> On 2023-03-29 18:08:29 +0200, Peter Eisentraut wrote:
>> On 24.03.23 19:12, Andres Freund wrote:
>>>> I thought about this some more.  I think we could get rid of attusertypmod
>>>> and just hardcode it as -1.  The idea would be that if you ask for an
>>>> encrypted column of type, say, varchar(500), the server isn't able to
>>>> enforce that anyway, so we could just prohibit specifying a nondefault
>>>> typmod for encrypted columns.
>>>
>>> Why not just use typmod for the underlying typmod? It doesn't seem like
>>> encrypted datums will need that? Or are you using it for something important there?
>>
>> Yes, the typmod of encrypted types stores the encryption algorithm.
> 
> Why isn't that an attribute of pg_colenckey, given that attcek has been added
> to pg_attribute?

One might think that, but the precedent in other equivalent systems is 
that you reference the key and the algorithm separately.  There is some 
(admittedly not very conclusive) discussion about this near [0].

[0]: 

https://www.postgresql.org/message-id/flat/00b0c4f3-0d9f-dcfd-2ba0-eee5109b4963%40enterprisedb.com#147ad6faafe8cdd2c0d2fd56ec972a40

>> (Also, mixing a type with the typmod of another type is weird in a variety
>> of ways, so this is a quite clean solution.)
> 
> It's not an unrelated type though. It's the actual typmod of the column we're
> talking about.

What I mean is that various parts of the system think that typid+typmod 
make sense together.  If the typmod actually refers to usertypid, well, 
the code doesn't know that, so who knows what happens.

Also, with the current proposal, the system is internally consistent: 
pg_encrypted_* can actually look at their own typmod and verify their 
own input values that way, which is what a typmod is for.  This just 
works out of the box.

> I find it a lot less clean to make all non-CEK uses of
> pg_attribute pay the price of storing three new fields.

With the proposed removal of usertypmod, it's only two fields: the link 
to the key and the user-facing type.

Hi,

On 2023-03-29 19:08:25 +0200, Peter Eisentraut wrote:
> On 29.03.23 18:24, Andres Freund wrote:
> > On 2023-03-29 18:08:29 +0200, Peter Eisentraut wrote:
> > > On 24.03.23 19:12, Andres Freund wrote:
> > > > > I thought about this some more.  I think we could get rid of attusertypmod
> > > > > and just hardcode it as -1.  The idea would be that if you ask for an
> > > > > encrypted column of type, say, varchar(500), the server isn't able to
> > > > > enforce that anyway, so we could just prohibit specifying a nondefault
> > > > > typmod for encrypted columns.
> > > > 
> > > > Why not just use typmod for the underlying typmod? It doesn't seem like
> > > > encrypted datums will need that? Or are you using it for something important there?
> > > 
> > > Yes, the typmod of encrypted types stores the encryption algorithm.
> > 
> > Why isn't that an attribute of pg_colenckey, given that attcek has been added
> > to pg_attribute?
> 
> One might think that, but the precedent in other equivalent systems is that
> you reference the key and the algorithm separately.  There is some
> (admittedly not very conclusive) discussion about this near [0].
> 
> [0]:
https://www.postgresql.org/message-id/flat/00b0c4f3-0d9f-dcfd-2ba0-eee5109b4963%40enterprisedb.com#147ad6faafe8cdd2c0d2fd56ec972a40

I'm very much not convinced by that. Either way, there at least there should
be a comment mentioning that we intentionally try to allow that.

Even if this feature is something we want (why?), ISTM that this should not be
implemented by having multiple fields in pg_attribute, but instead by a table
referenced by by pg_attribute.attcek.


> > > (Also, mixing a type with the typmod of another type is weird in a variety
> > > of ways, so this is a quite clean solution.)
> > 
> > It's not an unrelated type though. It's the actual typmod of the column we're
> > talking about.
> 
> What I mean is that various parts of the system think that typid+typmod make
> sense together.  If the typmod actually refers to usertypid, well, the code
> doesn't know that, so who knows what happens.

You control what the typmod for encrypted columns does. So I don't see what
problems that could be.

I seems quite likely that having a separate typmod for the encrypted type will
cause problems down the line, because you'll end up having to copy around
typid+typmod for the encrypted datum and then also separately for the
unencrypted one.


> Also, with the current proposal, the system is internally consistent:
> pg_encrypted_* can actually look at their own typmod and verify their own
> input values that way, which is what a typmod is for.  This just works out
> of the box.
> 
> > I find it a lot less clean to make all non-CEK uses of
> > pg_attribute pay the price of storing three new fields.
> 
> With the proposed removal of usertypmod, it's only two fields: the link to
> the key and the user-facing type.

That feels far less clean. I think loosing the ability to set the precision of
a numeric, or the SRID for postgis datums won't be received very well?

Greetings,

Andres Freund

On 30.03.23 03:29, Andres Freund wrote:
>> One might think that, but the precedent in other equivalent systems is that
>> you reference the key and the algorithm separately.  There is some
>> (admittedly not very conclusive) discussion about this near [0].
>>
>> [0]:
https://www.postgresql.org/message-id/flat/00b0c4f3-0d9f-dcfd-2ba0-eee5109b4963%40enterprisedb.com#147ad6faafe8cdd2c0d2fd56ec972a40
> 
> I'm very much not convinced by that. Either way, there at least there should
> be a comment mentioning that we intentionally try to allow that.
> 
> Even if this feature is something we want (why?), ISTM that this should not be
> implemented by having multiple fields in pg_attribute, but instead by a table
> referenced by by pg_attribute.attcek.

I don't know if it is clear to everyone here, but the key data model and 
the surrounding DDL are exact copies of the equivalent MS SQL Server 
feature.  When I was first studying it, I had the exact same doubts 
about this.  But as I was learning more about it, it does make sense, 
because this matches a common pattern in key management systems, which 
is relevant because these keys ultimately map into KMS-managed keys in a 
deployment.  Moreover, 1) it is plausible that those people knew what 
they were doing, and 2) it would be preferable to maintain alignment and 
not create something that looks the same but is different in some small 
but important details.

>> With the proposed removal of usertypmod, it's only two fields: the link to
>> the key and the user-facing type.
> 
> That feels far less clean. I think loosing the ability to set the precision of
> a numeric, or the SRID for postgis datums won't be received very well?

In my mind, and I probably wasn't explicit about this, I'm thinking 
about what can be done now versus later.

The feature is arguably useful without typmod support, e.g., for text. 
We could ship it like that, then do some work to reorganize pg_attribute 
and tuple descriptors to relieve some pressure on each byte, and then 
add the typmod support back in in a future release.  I think that is a 
workable compromise.

Hi,

On 2023-03-30 16:01:46 +0200, Peter Eisentraut wrote:
> On 30.03.23 03:29, Andres Freund wrote:
> > > One might think that, but the precedent in other equivalent systems is that
> > > you reference the key and the algorithm separately.  There is some
> > > (admittedly not very conclusive) discussion about this near [0].
> > > 
> > > [0]:
https://www.postgresql.org/message-id/flat/00b0c4f3-0d9f-dcfd-2ba0-eee5109b4963%40enterprisedb.com#147ad6faafe8cdd2c0d2fd56ec972a40
> > 
> > I'm very much not convinced by that. Either way, there at least there should
> > be a comment mentioning that we intentionally try to allow that.
> > 
> > Even if this feature is something we want (why?), ISTM that this should not be
> > implemented by having multiple fields in pg_attribute, but instead by a table
> > referenced by by pg_attribute.attcek.
> 
> I don't know if it is clear to everyone here, but the key data model and the
> surrounding DDL are exact copies of the equivalent MS SQL Server feature.
> When I was first studying it, I had the exact same doubts about this.  But
> as I was learning more about it, it does make sense, because this matches a
> common pattern in key management systems, which is relevant because these
> keys ultimately map into KMS-managed keys in a deployment.  Moreover, 1) it
> is plausible that those people knew what they were doing, and 2) it would be
> preferable to maintain alignment and not create something that looks the
> same but is different in some small but important details.

I find it very hard to belief that details of the catalog representation like
this will matter to users. How would would it conceivably affect users that we
store (key, encryption method) in pg_attribute vs storing an oid that's
effectively a foreign key reference to (key, encryption method)?


> > > With the proposed removal of usertypmod, it's only two fields: the link to
> > > the key and the user-facing type.
> > 
> > That feels far less clean. I think loosing the ability to set the precision of
> > a numeric, or the SRID for postgis datums won't be received very well?
> 
> In my mind, and I probably wasn't explicit about this, I'm thinking about
> what can be done now versus later.
> 
> The feature is arguably useful without typmod support, e.g., for text. We
> could ship it like that, then do some work to reorganize pg_attribute and
> tuple descriptors to relieve some pressure on each byte, and then add the
> typmod support back in in a future release.  I think that is a workable
> compromise.

I doubt that shipping a version of column encryption that breaks our type
system is a good idea.

Greetings,

Andres Freund

Greetings,

* Andres Freund (andres@anarazel.de) wrote:
> On 2023-03-30 16:01:46 +0200, Peter Eisentraut wrote:
> > On 30.03.23 03:29, Andres Freund wrote:
> > > > One might think that, but the precedent in other equivalent systems is that
> > > > you reference the key and the algorithm separately.  There is some
> > > > (admittedly not very conclusive) discussion about this near [0].
> > > >
> > > > [0]:
https://www.postgresql.org/message-id/flat/00b0c4f3-0d9f-dcfd-2ba0-eee5109b4963%40enterprisedb.com#147ad6faafe8cdd2c0d2fd56ec972a40
> > >
> > > I'm very much not convinced by that. Either way, there at least there should
> > > be a comment mentioning that we intentionally try to allow that.
> > >
> > > Even if this feature is something we want (why?), ISTM that this should not be
> > > implemented by having multiple fields in pg_attribute, but instead by a table
> > > referenced by by pg_attribute.attcek.
> >
> > I don't know if it is clear to everyone here, but the key data model and the
> > surrounding DDL are exact copies of the equivalent MS SQL Server feature.
> > When I was first studying it, I had the exact same doubts about this.  But
> > as I was learning more about it, it does make sense, because this matches a
> > common pattern in key management systems, which is relevant because these
> > keys ultimately map into KMS-managed keys in a deployment.  Moreover, 1) it
> > is plausible that those people knew what they were doing, and 2) it would be
> > preferable to maintain alignment and not create something that looks the
> > same but is different in some small but important details.

I was wondering about this- is it really exactly the same, down to the
point that there's zero checking of what the data returned actually is
after it's decrypted and given to the application, and if it actually
matches the claimed data type?

> I find it very hard to belief that details of the catalog representation like
> this will matter to users. How would would it conceivably affect users that we
> store (key, encryption method) in pg_attribute vs storing an oid that's
> effectively a foreign key reference to (key, encryption method)?

I do agree with this.

> > > > With the proposed removal of usertypmod, it's only two fields: the link to
> > > > the key and the user-facing type.
> > >
> > > That feels far less clean. I think loosing the ability to set the precision of
> > > a numeric, or the SRID for postgis datums won't be received very well?
> >
> > In my mind, and I probably wasn't explicit about this, I'm thinking about
> > what can be done now versus later.
> >
> > The feature is arguably useful without typmod support, e.g., for text. We
> > could ship it like that, then do some work to reorganize pg_attribute and
> > tuple descriptors to relieve some pressure on each byte, and then add the
> > typmod support back in in a future release.  I think that is a workable
> > compromise.
>
> I doubt that shipping a version of column encryption that breaks our type
> system is a good idea.

And this.

I do feel that column encryption is a useful capability and there's
large parts of this approach that I agree with, but I dislike the idea
of having our clients be able to depend on what gets returned for
non-encrypted columns while not being able to trust what encrypted
column results are and then trying to say it's 'transparent'.  To that
end, it seems like just saying they get back a bytea and making it clear
that they have to provide the validation would be clear, while keeping
much of the rest.  Expanding out from that I'd imagine, pie-in-the-sky
and in some far off land, having our data type in/out validation
functions moved to the common library and then adding client-side
validation of the data going in/out of the encrypted columns would allow
application developers to be able to trust what we're returning (as long
as they're using libpq- and we'd have to document that independent
implementations of the protocol have to provide this or just continue to
return bytea's).

Not sure how we'd manage to provide support for extensions though.

Thanks,

Stephen

On 30.03.23 17:55, Andres Freund wrote:
> I find it very hard to belief that details of the catalog representation like
> this will matter to users. How would would it conceivably affect users that we
> store (key, encryption method) in pg_attribute vs storing an oid that's
> effectively a foreign key reference to (key, encryption method)?

The change you are alluding to would also affect how the DDL commands 
work and interoperate, so it affects the user.

But also, let's not drive this design decision bottom up.  Let's go from 
how we want the data model and the DDL to work and then figure out 
suitable ways to record that.  I don't really know if you are just 
worried about the catalog size, or you find an actual fault with the 
data model, or you just find it subjectively odd.

>> The feature is arguably useful without typmod support, e.g., for text. We
>> could ship it like that, then do some work to reorganize pg_attribute and
>> tuple descriptors to relieve some pressure on each byte, and then add the
>> typmod support back in in a future release.  I think that is a workable
>> compromise.
> 
> I doubt that shipping a version of column encryption that breaks our type
> system is a good idea.

I don't follow how you get from that to claiming that it breaks the type 
system.  Please provide more details.

On 30.03.23 20:35, Stephen Frost wrote:
> I do feel that column encryption is a useful capability and there's
> large parts of this approach that I agree with, but I dislike the idea
> of having our clients be able to depend on what gets returned for
> non-encrypted columns while not being able to trust what encrypted
> column results are and then trying to say it's 'transparent'.  To that
> end, it seems like just saying they get back a bytea and making it clear
> that they have to provide the validation would be clear, while keeping
> much of the rest.

[Note that the word "transparent" has been removed from the feature 
name.  I just didn't change the email thread name.]

These thoughts are reasonable, but I think there is a tradeoff to be 
made between having featureful data validation and enhanced security. 
If you want your database system to validate your data, you have to send 
it in plain text.  If you want to have your database system not see the 
plain text, then it cannot validate it.  But there is still utility in it.

You can't really depend on what gets returned even in the non-encrypted 
case, unless you cryptographically sign the schema against modification 
or something like that.  So realistically, a client that cares strongly 
about the data it receives has to do some kind of client-side validation 
anyway.

Note also that higher-level client libraries like JDBC effectively do 
client-side data validation, for example when you call 
ResultSet.getInt() etc.

This is also one of the reasons why the user facing type declaration 
exists.  You could just make all encrypted columns of type "opaque" or 
something and not make any promises about what's inside.  But client 
APIs sort or rely on the application being able to ask the result set 
for what's inside a column value.  If we just say, we don't know, then 
applications (or driver APIs) will have to be changed to accommodate 
that, but the intention was to not require that.  So instead we say, 
it's supposed to be int, and then if it's sometimes actually not int, 
then your application throws an exception you can deal with.  This is 
arguably a better developer experience, even if it concerns the data 
type purist.

But do you have a different idea about how it should work?

> Expanding out from that I'd imagine, pie-in-the-sky
> and in some far off land, having our data type in/out validation
> functions moved to the common library and then adding client-side
> validation of the data going in/out of the encrypted columns would allow
> application developers to be able to trust what we're returning (as long
> as they're using libpq- and we'd have to document that independent
> implementations of the protocol have to provide this or just continue to
> return bytea's).

As mentioned, some client libraries effectively already do that.  But 
even if we could make this much more comprehensive, I don't see how this 
can ever actually satisfy your point.  It would require that all 
participating clients apply validation all the time, and all other 
clients can rely on that happening, which is impossible.

To kick some things off for PG18, here is an updated version of the 
patch for automatic client-side column-level encryption.  (See commit 
message included in the patch for a detailed description, if you have 
forgotten.  Also, see [0] if the thread has dropped off your local mail 
storage.)

[0]: 
https://www.postgresql.org/message-id/flat/89157929-c2b6-817b-6025-8e4b2d89d88f@enterprisedb.com

This patch got stuck around CF 2023-03 because expanding the size of the 
tuple descriptor (with new pg_attribute columns) had a noticeable 
performance impact.  Various work in PG17 has made it more manageable to 
have columns in pg_attribute that are not in the tuple descriptor, and 
this patch now takes advantage of that (and I wanted to do this merge 
soon to verify that the changes in PG17 are usable).  Otherwise, this 
version v20 is functionally unchanged over the last posted version v19. 
Obviously, it's early days, so there will be plenty of time to have 
discussions on various other aspects of this patch.  I'm keeping a keen 
eye on the discussion of protocol extensions, for example.

On Wed, 10 Apr 2024 at 12:13, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> To kick some things off for PG18, here is an updated version of the
> patch for automatic client-side column-level encryption.

I only read the docs and none of the code, but here is my feedback on
the current design:

> (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). As far as I can tell the only way to
rotate a CEK is by re-encrypting the column for all rows in a single
go at the client side, thus taking a write-lock on all rows of the
table. That seems quite problematic, because that makes key rotation
an operation that requires application downtime. Allowing online
rotation is important, otherwise almost no-one will do it preventative
at a regular interval.

One way to allow online CEK rotation is by allowing a column to be
encrypted by one of several keys and/or allow a key to have multiple
versions. And then for each row we would store which key/version it
was encrypted with. That way for new insertions/updates clients would
use the newest version. But clients would still be able to decrypt
both old rows with the old key and new rows encrypted with the new
key, because the server would give them both keys and tell which row
was encrypted with which. Then the old rows can be rewritten by a
client in small batches, so that writes to the table can keep working
while this operation takes place.

This could even be used to allow encrypting previously unencrypted
columns using something like "ALTER COLUMN mycol ENCRYPTION KEY cek1".
Then unencrypted rows could be indicated by e.g. returning something
like NULL for the CEK.

+    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.

+         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).

+   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?

+      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

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). 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? Or is
writing disallowed because the format code would never be set to 0x11.

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?

But no-matter these behavioural details, I think it would be fairly
easy to add minimal "non-support" for this feature while supporting
the new protocol messages. All they would need to do is understand
what the new protocol messages/fields mean and either ignore them or
throw a clear error. For poolers it's a different story however. For
transaction pooling there's quite a bit of work to be done. I already
mentioned the session-specific ID being a problem, but even assuming
we change that to a global ID there's still difficulties. Key
information is only sent by the server if it wasn't sent before in the
session[1], so a pooler would need to keep it's own cache and send
keys to clients that haven't received them yet.

So yeah, I think it would make sense to put this behind a protocol
extension feature flag, since it's fairly niche and would require
significant work at the pooler side to support.


[1]:
+    When automatic client-side column-level encryption is enabled, the
+    messages ColumnMasterKey and ColumnEncryptionKey can appear before
+    RowDescription and ParameterDescription messages.  Clients should collect
+    the information in these messages and keep them for the duration of the
+    connection.  A server is not required to resend the key information for
+    each statement cycle if it was already sent during this connection.

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?

On Thu, 18 Apr 2024 at 13:25, Peter Eisentraut <peter@eisentraut.org> wrote:
> Hopefully, the reason for key rotation is mainly that policies require
> key rotation, not that keys get compromised all the time.

These key rotation policies are generally in place to reduce the
impact of a key compromise by limiting the time a compromised key is
valid.

> 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.

I think the threat model for this proposal and a laptop's file system
encryption are different enough that the same choices/tradeoffs don't
automatically translate. Specifically in this proposal the unencrypted
CEK is present on all servers that need to read/write those encrypted
values. And a successful attacker would then be able to read the
encrypted values forever with this key, because it effectively cannot
be rotated. That is a much bigger attack surface and risk than a
laptop's disk encryption. So, I feel quite strongly that shipping the
proposed feature without being able to re-encrypt columns in an online
fashion would be a mistake.

> That's the
> reason for having this two-tier key system in the first place.

If we allow for online-rotation of the actual encryption key, then
maybe we don't even need this two-tier system ;)

Not having this two tier system would have a few benefits in my opinion:
1. We wouldn't need to be sending encrypted key material from the
server to every client. Which seems nice from a security, bandwidth
and client implementation perspective.
2. Asymmetric encryption of columns is suddenly an option. Allowing
certain clients to enter encrypted data into the database but not read
it.


> 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.

How about defining at column creation time if the underlying value
should be binary or not? Something like:

CREATE TABLE t(
    mytime timestamp ENCRYPTED WITH (column_encryption_key = cek1, binary=true)
);

> Even if the identifiers
> somehow were global (but OIDs can also change and are not guaranteed
> unique forever),

OIDs of existing rows can't just change while a connection is active,
right? (all I know is upgrades can change them but that seems fine)
Also they are unique within a catalog table, right?

> the state of which keys have already been sent is
> session state.

I agree that this is the case. But it's state that can be tracked
fairly easily by a transaction pooler. Similar to how prepared
statements can be tracked. And this is easier to do when at the IDs of
the same keys are the same across each session to the server, because
if they differ then you need to do re-mapping of IDs.

> 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.

Fair enough

> 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.

I see your point. But with the current approach even for queries that
don't return any encrypted columns, these useless fields would be part
of the RowDescryption. It seems quite annoying to add extra network
and parsing overhead all of your queries even if only a small
percentage use the encryption feature. Maybe we should add a new
message type instead like EncryptedRowDescription, or add some flag
field at the start of RowDescription that can be used to indicate that
there is encryption info for some of the columns.

> 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.

Okay, makes sense. But I think it would be good to document that.

> > 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?

A server has table A and B. A is encrypted with a master key realm X
and B is encrypted with master key realm Y. If libpq is only given a
key for realm X, and it then tries to read table B, an error is
thrown. While if you don't provide any realm at all, you can read from
table B just fine, only you will get bytea fields back.

On Wed, Apr 10, 2024 at 6:13 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> Obviously, it's early days, so there will be plenty of time to have
> discussions on various other aspects of this patch.  I'm keeping a keen
> eye on the discussion of protocol extensions, for example.

I think the way that you handled that is clever, and along the lines
of what I had in mind when I invented the _pq_ stuff.

More specifically, the way that the ColumnEncryptionKey and
ColumnMasterKey messages are handled is exactly the way that I was
imagining things would work. The client uses _pq_.column_encryption to
signal that it can understand those messages, and the server responds
by including them. I assume that if the client doesn't signal
understanding, then the server simply omits sending those messages. (I
have not checked the code.)

I'm less certain about the changes to the ParameterDescription and
RowDescription messages. I see a couple of potential problems. One is
that, if you say you can understand column encryption messages, the
extra fields are included even for unencrypted columns. The client
must choose at connection startup whether it ever wishes to read any
encrypted data; if so, it pays a portion of that overhead all the
time. Another potential problem is with the scalability of this
design. Suppose that we could not only encrypt columns, but also
compress, fold, mutilate, and spindle them. Then there might end up
being a dizzying array of variation in the format of what is supposed
to be the same message. Perhaps it's not so bad: as long as the
documentation is clear about in which order the additional fields will
appear in the relevant messages when more than one relevant feature is
used, it's probably not too difficult for clients to cope. And it is
probably also true that the precise size of, say, a RowDescription
message will rarely be performance-critical. But another thought is
that we might try to redesign this so that we simply add more message
types rather than mutating message types i.e. after sending the
RowDescription message, if any columns are encrypted, we additionally
send a RowEncryptionDescription message. Then this treatment becomes
symmetric with the handling of ColumnEncryptionKey and ColumnMasterKey
messages, and there's no overhead when the feature is unused.

With regard to the Bind message, I suggest that we regard the protocol
change as reserving a currently-unused bit in the message to indicate
whether the value is pre-encrypted, without reference to the protocol
extension. It could be legal for a client that can't understand
encryption message from the server to supply an encrypted value to be
inserted into a column. And I don't think we would ever want the bit
that's being reserved here to be used by some other extension for some
other purpose, even when this extension isn't used. So I don't see a
need for this to be tied into the protocol extension.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, 18 Apr 2024 at 18:46, Robert Haas <robertmhaas@gmail.com> wrote:
> With regard to the Bind message, I suggest that we regard the protocol
> change as reserving a currently-unused bit in the message to indicate
> whether the value is pre-encrypted, without reference to the protocol
> extension. It could be legal for a client that can't understand
> encryption message from the server to supply an encrypted value to be
> inserted into a column. And I don't think we would ever want the bit
> that's being reserved here to be used by some other extension for some
> other purpose, even when this extension isn't used. So I don't see a
> need for this to be tied into the protocol extension.

I think this is an interesting idea. I can indeed see use cases for
e.g. inserting a new row based on another row (where the secret is the
same).

IMHO that means that we should also bump the protocol version for this
change, because it's changing the wire protocol by adding a new
parameter format code. And it does so in a way that does not depend on
the new protocol extension.

On Thu, Apr 18, 2024 at 1:49 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> I think this is an interesting idea. I can indeed see use cases for
> e.g. inserting a new row based on another row (where the secret is the
> same).
>
> IMHO that means that we should also bump the protocol version for this
> change, because it's changing the wire protocol by adding a new
> parameter format code. And it does so in a way that does not depend on
> the new protocol extension.

I think we're more or less covering the same ground we did on the
other thread here -- in theory I don't love the fact that we never
bump the protocol version when we change stuff, but in practice if we
start bumping it every time we do anything I think it's going to just
break a bunch of stuff without any real benefit.

--
Robert Haas
EDB: http://www.enterprisedb.com