Re: Proposed patch for key management

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Bruce Momjian <bruce(at)momjian(dot)us>
Cc: Alastair Turner <minion(at)decodable(dot)me>, Stephen Frost <sfrost(at)snowman(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>
Subject: Re: Proposed patch for key management
Date: 2020-12-31 10:11:11
Message-ID: alpine.DEB.2.22.394.2012311023150.3573723@pseudo
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


Hello Bruce,

>> The API should NOT make assumptions about the cryptographic design, what
>> depends about what, where things are stored… ISTM that Pg should only care
>> about naming keys, holding them when created/retrieved (but not create
>> them), basically interacting with the key manager, passing the stuff to
>> functions for encryption/decryption seen as black boxes.
>>
>> I may have suggested something along these lines at the beginning of the key
>> management thread, probably. Not going this way implicitely implies making
>> some assumptions which may or may not suit other use cases, so makes them
>> specific not generic. I do not think pg should do that.
>
> I am not sure what else I can add to this discussion. Having something
> that is completely external might be a nice option, but I don't think it
> is the common use-case, and will make the most common use cases more
> complex.

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

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

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

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

> and I would not want to be associated with it.

You do as you want. I'm no one, you can ignore me and proceed to commit
whatever you want. I'm only advising to the best of my ability, what I
think should be the level of API pursued for such a feature in pg, at the
design level.

I feel that the current proposal is built around one particular use case
with many implicit and unnecessary assumptions without giving much
thoughts to the generic API which should really be pg concern, and I do
not think that it can really be corrected once the ship has sailed, hence
my attempt at having some thoughts given to the matter before that.

IMHO, the *minimum* should be to have the API clearly designed, and the
implementation compatible with it at some level, including not having
assumptions about underlying cryptographic functions and key derivations
(I mean at the API level, the code itself can do it obviously).

If you want a special "key_mgt_command = internal" because you feel that
processes are fragile and unreliable and do not bring security, so be it,
but I think that the API should be designed beforehand nevertheless.

Note that if people think that I'm wrong, then I'm wrong by definition.

--
Fabien.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Etsuro Fujita 2020-12-31 10:15:48 Re: Asynchronous Append on postgres_fdw nodes.
Previous Message Noah Misch 2020-12-31 07:45:47 Re: Buildfarm's cross-version-upgrade tests