On Wed, 19 Feb 2020 at 09:29, Cary Huang <cary.hu...@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_init to initialise context
> for encryption and key wrapping. In ossl_aes256_encrypt_init, the cipher
> method always initialises to aes-256-cbc, which is ok for keywrap because
> under CBC block cipher mode, we only had to supply one unique IV as initial
> value. But for actual WAL and buffer encryption that will come in later, I
> think the discussion is to use CTR block cipher mode, which requires one
> unique IV for each block, and the sequence id from WAL and buffer can be used
> to derive unique IV for each block for better security? I think it would be
> better to allow caller to 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 performing the encryption and decryption. So when we call
> "ossl_aes256_encrypt_init", we can include the new enum as input parameter
> and it will initialise the EVP_CIPHER_CTX with either EVP_aes_256_cbc() or
> EVP_aes_256_ctr() for different purposes (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. This is because the result length is not
> set long enough to accommodate the 4 byte VARHDRSZ header used by the
> multi-type variable.
>
> the len variable in SET_VARSIZE(res, len) should include also the variable
> header VARHDRSZ. Now it is 4 byte short so it 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 obtain the wrapped key from the server
> in order to use these wrap/unwrap functions. I personally don't think it is a
> good idea to expose these functions to user
Thank you for testing. I'm going to include regression tests and
documentation in the next version patch.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
