Re: Proposed patch for key management - Mailing list pgsql-hackers

Hello Stephen,

>> I'm unsure about what is the "common use case". ISTM that having the
>> postgres process holding the master key looks like a "no" for me in any use
>> case with actual security concern: as the database must be remotely
>> accessible, a reasonable security assumption is that a pg account could be
>> compromised, and the "master key" (if any, that is just one particular
>> cryptographic design) should not be accessible in that case. The first
>> barrier would be pg admin account. With external, the options are that the
>> key is hold by another id, so the host must be compromised as well, and
>> could even be managed remotely on another host (I agree that this later
>> option would be fragile because of the implied network connection, but it
>> would be a tradeoff depending on the security concerns, but it pretty much
>> correspond to the kerberos model).
>
> No, this isn't anything like the Kerberos model and there's no case
> where the PG account won't have access to the DEK (data encryption key)
> during normal operation (except with the possibility of off-loading to a
> hardware crypto device which, while interesting, is definitely something
> that's of very limited utility and which could be added on as a
> capability later anyway.  This is also well understood by those who are
> interested in this capability and it's perfectly acceptable that PG will
> have, in memory, the DEK.

I'm sorry that I'm not very clear. My ranting is having a KEK "master key" 
in pg memory. I think I'm fine at this point with having DEKs available on 
the host to code/decode files. What I meant about kerberos is that the 
setup I was describing was making the database dependent on a remote host, 
which is somehow what keroberos does.

> The keys which are stored on disk with the proposed patch are encrypted
> using a KEK which is external to PG- that's all part of the existing
> patch and design.

Yep. My point is that the patch hardwires a cryptographic design with many 
assumptions, whereas I think it should design an API compatible with a 
larger class of designs, and possibly implement one with KEK and so on, 
I'm fine with that.

>>> Adding a pre-defined system will not prevent future work on a
>>> completely external option.
>>
>> Yes and no.
>>
>> Having two side-by-side cluster-encryption scheme in core, the first that
>> could be implemented on top of the second without much effort, would look
>> pretty weird. Moreover, required changes in core to allow an API are the
>> very same as the one in this patch. The other concern I have with doing the
>> cleaning work afterwards is that the API would be somehow in the middle of
>> the existing functions if it has not been thought of before.
>
> This has been considered and the functions which are proposed are
> intentionally structured to allow for multiple implementations already,

Does it? This is unclear to me from looking at the code, and the limited
documentation does not point to that. I can see that the "kek provider" 
and the "random provider" can be changed, but the overall cryptographic 
design seems hardwired.

> so it's unclear to me what the argument here is.

The argument is that professional cryptophers do wrong designs, and I do 
not see why you would do different. So instead of hardwiring choice that 
you think are THE good ones, ISTM that pg should design a reasonably 
flexible API, and also provide an implementation, obviously.

> Further, we haven't even gotten to actual cluster encryption yet- this 
> is all just the key management side of things,

With hardwired choices about 1 KEK and 2 DEK that are debatable, see 
below.

> which is absolutely a necessary and important part but argueing that it 
> doesn't address the cluster encryption approach in core simply doesn't 
> make any sense as that's not a part of this patch.
>
> Let's have that discussion when we actually get to the point of talking
> about cluster encryption.
>
>>> I know archive_command is completely external, but that is much simpler
>>> than what would need to be done for key management, key rotation, and key
>>> verification.
>>
>> I'm not sure of the design of the key rotation stuff as I recall it from the
>> patches I looked at, but the API design looks like the more pressing issue.
>> First, the mere existence of an "master key" is a cryptographic choice which
>> is debatable, because it creates issues if one must be able to change it,
>> hence the whole "key rotation" stuff. ISTM that what core needs to know is
>> that one particular key is changed by a new one and the underlying file is
>> rewritten. It should not need to know anything about master keys and key
>> derivation at all. It should need to know that the user wants to change file
>> keys, though.
>
> No, it's not debatable that a KEK is needed, I disagree entirely.

ISTM that there is cryptographic design which suits your needs and you 
seem to decide that it is the only possible way to do it It seems that you 
know. As a researcher, I know that I do not know:-) As a software 
engineer, I'm trying to suggest enabling more with the patch, including 
not hardwiring a three-key management scheme.

I'm advocating that you pg not decide for others what is best for them, 
but rather allow multiple choices and implementations through an API. Note 
that I'm not claiming that the 1 KEK + 2 DEK + AES… design is bad. I'm 
claiming that it is just one possible design, quite peculiar though, and 
that pg should not hardwire it, and it does not need to anyway.

The documentation included in the key management patch states: "Key zero 
is the key used to encrypt database heap and index files which are stored 
in the file system, plus temporary files created during database 
operation." Read as is, it suggests that the same key is used to encrypt 
many files. From a cryptographic point of view this looks like a bad idea. 
The mode used is unclear. If this is a stream cypher generated in counter 
mode, it could be a very bad idea. Hopefully this is not what is intended, 
but the included documentation is frankly misleading in that case. IMHO 
there should be one key per file. Also, the CTR mode should be avoided if 
possible because it has quite special properties, unless these properties 
are absolutely necessary.

From what I see in the patch, the DEKs cannot be changed, only the KEK 
which encodes de DEK. I cannot say I'm thrilled by such a property. If the 
KEK is lost, then probably the DEKs are lost as well. Changing the KEK 
does not change anything about it, so that the attacker would be able to 
retrieve the contents of later/previous state of the database if they can 
retrieve a previous/later encoded version, even if the KEK is changed 
every day. This is a bad property which somehow provides a false sense of 
security. I'm not sure that it makes much sense to change a KEK without 
changing the associated DEK, security-wise.

Anyway, I'd suggest to provide a script to re-encrypt a cluster, possibly 
while offline, with different DEKs. From an audit perspective, this could 
be a requirement.

I'd expect a very detailed README or documentation somewhere, but there 
are none in the committed/uncommitted patch.

In https://wiki.postgresql.org/wiki/Transparent_Data_Encryption, I see a 
lot of minute cryptographic design decision which are debatable, and a few 
which seem to be still opened, whereas other things are already in the 
process of being committed. I'm annoyed by the decision to reuse the same 
key for a lot of encryption, even if there are discussions about changing 
the iv in some way. If you have some clever way to build an iv, then 
please please change the key as well?

ISTM that pg at the core level should (only) directly provide:

(1) a per-file encryption scheme, with loadable (hook-replaceable 
functions??) to manage pages, maybe:

   encrypt(page_id, *key, *clear_page, *encrypted_page);
   decrypt(page_id, *key, *encrypted_page, *clear_page);

What encrypt/decrypt does is beyond pg core stuff. Ok, a reasonable
implementation should be provided, obviously, possibly in core. There may 
be some block-size issues if not full pages are encrypted, so maybe the 
interface needs to be a little more subtle.

(2) offer a key management scheme interface, to manage *per-file* keys, 
possibly loadable (hook replaceable?). If all files have the same key, 
which is stored in a directory and encoded with a KEK, this is just one 
(debatable) implementation choice. For that, ISTM that what is needed at 
this level is:

   get_key(file_id (relative name? oid? 8 or 16 bytes something?));

Maybe that should be enough to create a new key implicitely, or maybe it 
should be a different interface, eg get_new_key(file_id), not sure. There 
is the question of the key length, which may for instance include an iv. 
Maybe an information seeking function should be provided, to know about 
sizes or whatever, not sure. Maybe there should/could be a parameter to 
manage key versions, not sure.

The internal key infrastructure would interact with this/these functions 
when needed, and store the key in the file management structure. All this 
should be a black box to core pg, that is the point of an API. This is 
what I'd have expected to see in the "postgres core key management" stuff, 
not a hardwired 2-key store with a KEK.

(3) ISTM that the key management interface should be external, or at least 
it should be possible to make it external easily. I do not think that 
there is a significant performance issue because keys are needed once, and 
once loaded they are there. A simple way to do that is a separate process 
with a basic protocol on stdin/stdout to implement "get_key", which is 
basically already half implemented in the patch for retrieving the KEK.

> Perhaps we can have support for an external key store in the future for
> the DEKs, but we really need to have our own key management and the
> ability to reasonably rotate keys (which is what the KEK provides).

I think that the KEK/2-DEK store is a peculiar design which does not 
*need* to be hardwired deeply in core.

> Ideally, we'll get to a point where we can even have multiple DEKs and
> deal with rotating them too, but that, if anything, point even stronger
> to the need to have a key management system and KEK as we'll be dealing
> with that many more keys.
>
>>> I will say that if the community feels external-only should be the only
>>> option, I will stop working on this feature because I feel the result
>>> would be too fragile to be reliable,
>>
>> I'm do not see why it would be the case. I'm just arguing to have key
>> management in a separate, possibly suid something-else, process, which given
>> the security concerns which dictates the feature looks like a must have, or
>> at least must be possible. From a line count point of view, it should be a
>> small addition to the current code.
>
> All of this hand-waving really isn't helping.

I'm sorry that you feel that. I'm really trying to help improve the 
approach and design with my expertise. As I said in another mail, I'm no 
one, you can always chose to ignore me.

> If it's a small addition to the current code then it'd be fantastic if
> you'd propose a specific patch which adds what you're suggesting.

My point is to discuss an API.

Once there is some agreement the possible API outline, then 
implementations can be provided, but if people do not want an API in the 
first place, I'm not sure I can see the point of sending patches.

> I don't think either Bruce or I would have any issue with others helping 
> out on this effort, but let's be clear- we need something that *is* part 
> of core PG, even if we have an ability to have other parts exist outside 
> of PG.

Yep. I have not suggested that PG should not implement the API, on the 
contrary it definitely should.

-- 
Fabien.