Re: Internal key management system

On Sun, 14 Jun 2020 at 19:39, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
>
>
> Hello Masahiko-san,
>
> >>> * The external place needs to manage more encryption keys than the
> >>> current patch does.
> >>
> >> Why? If the external place is just a separate process on the same host,
> >> probably it would manage the very same amount as what your patch.
> >
> > In the current patch, the external place needs to manage only one key
> > whereas postgres needs to manages multiple DEKs. But with your idea,
> > the external place needs to manage both KEK and DEKs.
>
> Hmmm. I do not see a good use case for a "management system" which would
> only have to manage a singleton. ISTM that one point of using one KEK is
> to allows several DEKs under it. Maybe I have missed something in your
> patch, but only one key is a very restricted use case.
>
> >>> Some cloud key management services are charged by the number of active
> >>> keys and key operations. So the number of keys postgres requires affects
> >>> the charges. It'd be worse if we were to have keys per table.
> >>
> >> Possibly. Note that you do not have to use a cloud storage paid as a
> >> service. However, you could do it if there is an interface, because it
> >> would allow postgres to do so if the user wishes that. That is the point
> >> of having an interface that can be implemented differently for different
> >> use cases.
> >
> > The same is true for the current patch. The user can get KEK from
> > anywhere they want using cluster_passphrase_command.
>
> Yep. Somehow I'm proposing to have a command to get DEKs instead of just
> the KEK, otherwise it is not that far.
>
> > But as I mentioned above the number of keys that the user manages
> > outside postgres is different.
>
> Yep, and I do not think that "only one key" approach is good. I really
> missed something in the patch. From a use case point of view, I thought
> that the user could have has many keys has they see fit. Maybe one per
> cluser, or database, or table, or a row if for some reason the application
> would demand it. I do not think that the pg should decide that, among
> other things. That is why I'm constantly refering to a "key identifier",
> and in the pseudo code I added a "local id" (typically an int).

What I referred to "only one key" is KEK. In the current patch,
postgres needs to manage multiple DEKs and fetches one KEK from
somewhere. According to the recent TDE discussion, we would have one
DEK for all tables/indexes encryption and one DEK for WAL encryption
as the first step.

>
> >>> * If this approach supports only GET protocol, the user needs to
> >>> create encryption keys with appropriate ids in advance so that
> >>> postgres can get keys by id. If postgres TDE creates keys as needed,
> >>> CREATE protocol would also be required.
> >>
> >> I'm not sure. ISTM that if there is a KMS to manage keys, it could be its
> >> responsability to actually create a key, however the client (pg) would
> >> have to request it, basically say "given me a new key for this id".
> >>
> >> This could even work with a "get" command only, if the KMS is expected to
> >> create a new key when asked for a key which does not exists yet. ISTM that
> >> the client could (should?) only have to create identifiers for its keys.
> >
> > Yeah, it depends on KMS, meaning we need different extensions for
> > different KMS. A KMS might support an interface that creates key if not
> > exist during GET but some KMS might support CREATE and GET separately.
>
> I disagree that it is necessary, but this is debatable. The KMS-side
> interface code could take care of that, eg:
>
> if command is "get X"
> if (X does not exist in KMS)
> create a new key stored in KMS, return it;
> else
> return KMS-stored key;
> ...
>
> So you can still have a "GET" only interface which adapts to the "final"
> KMS. Basically, the glue code which implements the interface for the KMS
> can include some logic to adapt to the KMS point of view.

Is the above code is for the extension side, right? For example, if
users want to use a cloud KMS, say AWS KMS, to store DEKs and KEK they
need an extension that is loaded to postgres and can communicate with
AWS KMS. I imagine that such extension needs to be written in C, the
communication between the extension uses AWS KMS API, and the
communication between postgres core and the extension uses text
protocol. When postgres core needs a DEK identified by KEY-A, it asks
for the extension to get the DEK by passing something like “GET KEY-A”
message, and then the extension asks the existence of that key to AWK
KMS, creates if not exist and returns it to the postgres core. Is my
understanding right?

When we have TDE feature in the future, we would also need to change
frontend tools such as pg_waldump and pg_rewind that read database
files so that they can read encrypted files. It means that these
frond-end tools also somehow need to communicate with the external
place to get DEKs in order to decrypt encrypted database files. In
your idea, what do you think about how we can support it?

>
> >>> * If we need only GET protocol, the current approach (i.g.
>
> >> The point of a discussion is basically to present arguments.
> >
> > My point is the same as Bruce. I'm concerned about the fact that even
> > if we introduce this approach the present data could still be stolen
> > when a postgres process is compromised.
>
> Yes, sure.
>
> > It seems to me that your approach is extensible and can protect data
> > from threats in addition to threats that the current patch can protect
> > but it would bring some costs and complexity instead comparing to the
> > current patch. I'd like to hear opinions from other hackers in the
> > community.
>
> I'd like an extensible design to have anything in core. As I said in an
> other mail, if you want to handle a somehow restricted use case, just
> provide an external extension and do nothing in core, please. Put in core
> something that people with a slightly different use case or auditor can
> build on as well. The current patch makes a dozen hard-coded decisions
> which it should not, IMHO.

It might have confused you that I included key manager and encryption
SQL functions to the patches but this key manager has been designed
dedicated to only TDE. It might be better to remove both SQL interface
and SQL key we discussed from the patch set as they are actually not
necessary for TDE purposes. Aside from the security risk you
mentioned, it was a natural design decision for me that we have our
key manager component in postgres core that is responsible for
managing encryption keys for our TDE. To make the key manager and TDE
simple as much as possible, we discussed that we will have
cluster-wide TDE and key manager that manages a few encryption keys
used by TDE (e.g. one key for table/index encryption and another key
for WAL encryption), as the first step.

>
> > I think the actual code would help to explain how your approach is not
> > complexed.
>
> I provided quite some pseudo code, including some python. I'm not planning
> to redevelop the whole thing: my contribution is a review, currently about
> the overall design, then if somehow I agree on the design, I would look at
> the code more precisely.

Hmm, understood. Let's wait for comments from other members.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services