Re: Internal key management system

Hello Masahiko-san,

> What I referred to "only one key" is KEK.

Ok, sorry, I misunderstood.

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

Such a code could be in the command with which pg communicates (eg through 
its stdin/stdout, or whatever) to get keys.

pg talks to the command, the command can do anything, such as storing keys 
or communicating with an external service to retrieve them, anything 
really, that is the point.

I'm advocating defining the pg/command protocol, something along "GET xxx" 
as you wrote, and possibly provide a possible/reasonable command 
implementation, which would be part of the code you put in your patch, 
only it would be in the command instead of postgres.

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

Why? I could write it in bash, probably. Ok, maybe not so good for suid, 
but in principle it could be anything. I'd probably write it in C, though.

> the communication between the extension uses AWS KMS API, and the 
> communication between postgres core and the extension uses text 
> protocol.

I'm not sure of the word "extension" above. For me the postgres side could 
be an extension as in "CREATE EXTENSION". The command itself could be 
provided in the extension code, but would not be in the "CREATE 
EXTENSION", it would be something run independently.

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

Yes. The command in the use-case you outline would just be an 
intermediary, but for another use-case it would do the storing. The point 
of aiming at extensibility if that from pg point of view the external 
commands provide keys, but what these commands really do to do this can be 
anything.

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

Hmmm. My idea was that the natural interface would be to get things 
through postgres. For a debug tool such as pg_waldump, probably it needs 
to be adapted if it needs to decrypt data to operate.

Now I'm not sure I understood, because of the DEK are managed by postgres 
in your patch, waldump and other external commands would have no access to 
the decrypted data anyway, so the issue would be the same?

With file-level encryption, obviously all commands which have to read and 
understand the files have to be adapted if they are to still work, which 
is another argument to have some interface rather than integrated 
server-side stuff, because these external commands would need to be able 
to get keys and use them as well.

Or I misunderstood something.

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

Hmmm. This is NOT AT ALL what the patch does. The documentation in your 
patch talks about "column level encryption", which is an application 
thing. Now you seem to say that it does not matter and can be removed 
because the use case is elsewhere.

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

The documentation part of the patch, at no point, talks about TDE 
(transparent data encryption), which is a file-level encryption as far as 
I understand it, i.e. whole files are encrypted.

I'm lost, because if you want to do that you cannot easily use 
padding/HMAC and so because they would change block sizes, and probably 
you would use CRT instead of CBC to be able to decrypt data selectively.

So you certainly succeeded in confusing me deeply:-)

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

The patch really needs a README to explain what it really does, and why, 
and how, and what is the thread model, what are the choices (there should 
be as few as possible), how it can/could be extended.

I've looked at the whole patch, and I could not find the place where files 
are actually encrypted/decrypted at a low level, that I would expect for 
file encryption implementation.

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

Hmmm. Ok. So in fact all that is for TDE, *but* the patch does not do TDE, 
but provides a column-oriented SQL-level encryption, which is unrelated to 
your actual objective, which is to do file-level encryption in the end.

However, for TDE, it may that you cannot do it with a pg extension because 
for the extension to work the database must work, which would require some 
"data" files not to be encrypted in the first place. That seems like a 
good argument to actually have something in core.

Probably for TDE you only want the configuration file not to be encrypted.

I'd still advocate to have the key management system possibly outside of 
pg, and have pg interact with it to get keys when needed. Probably key ids 
would be the relative file names in that case. The approach of 
externalizing encryption/decryption would be totally impractical for 
performance reasons, though.

I see value in Cary Huang suggestion on the thread to have dynamically 
loaded functions implement an interface. That would at least allow to 
remove some hardcoded choices such as what cypher is actually used, key 
sizes, and so on. One possible implementation would be to manage things 
more or less internally as you do, another to fork an external command and 
talk with it to do the same.

However, I do not share the actual interface briefly outlined: I do not 
thinkpg should have to care about key management functions such as 
rotation, generation or derivation, storage… the interest of pg should be 
limited to retrieving the keys it needs. That does not mean such functions 
do not have security value and should not be implemented, I'd say that it 
should not be visible/hardcoded in the pg/kms interface, especially if
this interface is expected to be generic.

As I see it, a pg/kms C-level loadable interface would provide the 
following function:

// options would be supplied by a guc and allow to initialize the 
// interface with the relevant data, whatever the underlying 
// implementation needs.
error kms_init(char *options);

// associate opaque key identifier to a local id
error kms_key(local_id int, int key_id_len, byte *key_id);

or maybe something like:

// would return the local id attributed to the key
error/int kms_key(key_id_len, key_id);

// the actual functions should be clarified
// for TDE file-level, probably the encrypted length is the same as the 
// input, you cannot have padding, hmac or whatever added.
// for SQL app-level, the rules could be different
error kms_(en|de)crypt(local_id int, int mode, int len,
                        byte *in, byte *out);

// maybe
error kms_key_forget(local_id int);
error kms_destroy(…);

// maybe, to allow extensibility and genericity
// eg kms_command("rotate keys with new kek=123");
error kms_command(char *cmd);

I'm a little bit unsure that there should be only one KMS active ever, 
though: a file-level vs app-level encryption could have quite different 
constraints. Also, should the app-level encryption be able to access keys
loaded for file-level encryption?

-- 
Fabien.