Re: Proposed patch for key management

On Thu, Dec 31, 2020 at 11:11:11AM +0100, Fabien COELHO wrote:
> > 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.

Let's unpack this logic, since I know others, like Alastair Turner (CC
added), had similar concerns.  Frankly, I feel this feature has limited
security usefulness, so if we don't feel it has sufficient value, let's
mark it as not wanted and move on.

I think there are two basic key configurations:

1.  pass data encryption keys in from an external source
2.  store the data encryption keys wrapped by a key encryption key (KEK)
    passed in from an external source

The current code implements #2 because it simplifies administration,
checking, external key (KEK) rotation, and provides good error reporting
when something goes wrong.  For example, with #1, there is no way to
rotate the externally-stored key except reencrypting the entire cluster.

When using AES256 with GCM (for verification) is #1 more secure than #2?
I don't think so.  If the Postgres account is compromised, they can grab
the data encryption keys as the come in from the external script (#1),
or when they are decrypted by the KEK (#2), or while they are in shared
memory while the server is running.  If the postgres account is not
compromised, I don't think it is any easier to get the data key wrapped
by a KEK than it is to try decrypting the actual heap/index/WAL blocks.
(Once you find the key for one, they would all decrypt.)

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

We don't store the data encryption keys raw on the server, but rather
wrap them with a KEK.  If the external keystore is compromised using #1,
you can't rotate those keys without reencrypting the entire cluster.

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

The question is whether there is a common use-case, and if so, do we
implement that first, with all the safeguards, and then allow others to
customize?  I don't want to optimize for the rare case if that makes the
common case more error-prone.

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

Well, you would want to change the external-stored key independent of
the data encryption keys, which are harder to change.

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

See above.  Please explain the security value of this complexity.

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

You are not the only one who is suggesting this, so we should decide as
a group on an acceptable direction.

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

I have already explained why I am optimizing for the common use case,
and you are not addressing my reasons here.

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

I think we need a separate command for the fully external case, rather
than trying to lay it on top of the command for the internal one.

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

  The usefulness of a cup is in its emptiness, Bruce Lee