Re: Proposed patch for key management

Greetings,

* Fabien COELHO (coelho(at)cri(dot)ensmp(dot)fr) wrote:
> >>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).

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.

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.

> >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,
so it's unclear to me what the argument here is. Further, we haven't
even gotten to actual cluster encryption yet- this is all just the key
management side of things, 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.
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).
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.

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

Thanks,

Stephen