Re: Internal key management system
От | Masahiko Sawada |
---|---|
Тема | Re: Internal key management system |
Дата | |
Msg-id | CA+fd4k4ynaF78Pc=7rQ+dPW7mxD+GpxiQGVR80EZkEmzg-A9uQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Internal key management system (Masahiko Sawada <masahiko.sawada@2ndquadrant.com>) |
Ответы |
Re: Internal key management system
(Cary Huang <cary.huang@highgo.ca>)
|
Список | pgsql-hackers |
On Thu, 20 Feb 2020 at 16:16, Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > On Wed, 19 Feb 2020 at 09:29, Cary Huang <cary.huang@highgo.ca> wrote: > > > > Hi > > > > I have tried the attached kms_v3 patch and have some comments: > > > > 1) In the comments, I think you meant hmac + iv + encrypted data instead of iv + hmac + encrypted data? > > > > ---> in kmgr_wrap_key( ): > > + /* > > + * Assemble the wrapped key. The order of the wrapped key is iv, hmac and > > + * encrypted data. > > + */ > > Right, will fix. > > > > > > > 2) I see that create_keywrap_ctx function in kmgr_utils.c and regular cipher context init will both call ossl_aes256_encrypt_initto initialise context for encryption and key wrapping. In ossl_aes256_encrypt_init, the cipher methodalways initialises to aes-256-cbc, which is ok for keywrap because under CBC block cipher mode, we only had to supplyone unique IV as initial value. But for actual WAL and buffer encryption that will come in later, I think the discussionis to use CTR block cipher mode, which requires one unique IV for each block, and the sequence id from WAL andbuffer can be used to derive unique IV for each block for better security? I think it would be better to allow callerto decide which EVP_CIPHER to initialize? EVP_aes_256_cbc(), EVP_aes_256_ctr() or others? > > > > +ossl_aes256_encrypt_init(pg_cipher_ctx *ctx, uint8 *key) > > +{ > > + if (!EVP_EncryptInit_ex(ctx, EVP_aes_256_cbc(), NULL, NULL, NULL)) > > + return false; > > + if (!EVP_CIPHER_CTX_set_key_length(ctx, PG_AES256_KEY_LEN)) > > + return false; > > + if (!EVP_EncryptInit_ex(ctx, NULL, NULL, key, NULL)) > > + return false; > > + > > + /* > > + * Always enable padding. We don't need to check the return > > + * value as EVP_CIPHER_CTX_set_padding always returns 1. > > + */ > > + EVP_CIPHER_CTX_set_padding(ctx, 1); > > + > > + return true; > > +} > > It seems good. We can expand it to make caller decide the block cipher > mode of operation and key length. I removed such code from the > previous patch to make it simple since currently we support only > AES-256 CBC. > > > > > 3) Following up point 2), I think we should enhance the enum to include not only the Encryption algorithm and key size,but also the block cipher mode as well because having all 3 pieces of information can describe exactly how KMS is performingthe encryption and decryption. So when we call "ossl_aes256_encrypt_init", we can include the new enum as inputparameter and it will initialise the EVP_CIPHER_CTX with either EVP_aes_256_cbc() or EVP_aes_256_ctr() for differentpurposes (key wrapping, or WAL encryption..etc). > > > > ---> kmgr.h > > +/* Value of key_management_cipher */ > > +enum > > +{ > > + KMGR_CIPHER_OFF = 0, > > + KMGR_CIPHER_AES256 > > +}; > > + > > > > so it would become > > +enum > > +{ > > + KMGR_CIPHER_OFF = 0, > > + KMGR_CIPHER_AES256_CBC = 1, > > + KMGR_CIPHER_AES256_CTR = 2 > > +}; > > > > If you agree with this change, several other places will need to be changed as well, such as "kmgr_cipher_string", "kmgr_cipher_value"and the initdb code.... > > KMGR_CIPHER_XXX is relevant with cipher mode used by KMS and KMS would > still use AES256 CBC even if we had TDE which would use AES256 CTR. > > After more thoughts, I think currently we can specify -e aes-256 to > initdb but actually this is not necessary. When > --cluster-passphrase-command specified, we enable the internal KMS and > always use AES256 CBC. Something like -e option will be needed after > supporting TDE with several cipher options. Thoughts? > > > > > 4) the pg_wrap_key and pg_unwrap_key SQL functions defined in kmgr.c > > I tried these new SQL functions and found that the pg_unwrap_key will produce the original key with 4 bytes less. Thisis because the result length is not set long enough to accommodate the 4 byte VARHDRSZ header used by the multi-typevariable. > > > > the len variable in SET_VARSIZE(res, len) should include also the variable header VARHDRSZ. Now it is 4 byte short soit will produce incomplete output. > > > > ---> pg_unwrap_key function in kmgr.c > > + if (!kmgr_unwrap_key(UnwrapCtx, (uint8 *) VARDATA_ANY(data), datalen, > > + (uint8 *) VARDATA(res), &len)) > > + ereport(ERROR, > > + (errmsg("could not unwrap the given secret"))); > > + > > + /* > > + * The size of unwrapped key can be smaller than the size estimated > > + * before unwrapping since the padding is removed during unwrapping. > > + */ > > + SET_VARSIZE(res, len); > > + PG_RETURN_BYTEA_P(res); > > > > I am only testing their functionalities with random key as input data. It is currently not possible for a user to obtainthe wrapped key from the server in order to use these wrap/unwrap functions. I personally don't think it is a goodidea to expose these functions to user > > Thank you for testing. I'm going to include regression tests and > documentation in the next version patch. > Attached the updated version patch. In this version, I've removed -e option of initdb that was used to specify the encryption algorithm and key length like aes-256. The cipher algorithm and key length used by KMS is fixed, aes-256, so it's no longer necessary as long as we support only KMS. When we introduce transparent data encryption and we'd like to support multiple options we will have such option. Therefore, the internal KMS is enabled when PostgreSQL is built with --with-openssl and --cluster-passphrase-command is specified to initdb. The patch includes minimal doc and regression tests. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: Michael PaquierДата:
Сообщение: Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions?