Key management with tests
I have completed the key management patch with tests created by Stephen
Frost. Original patch by Masahiko Sawada. It requires the hex
reorganization patch first. The key patch is now 2.1MB because of the
tests, so attaching it here seems unwise:
https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
I will add it to the commitfest. I think we need to figure out how much
of the tests we want to add.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote:
I have completed the key management patch with tests created by Stephen
Frost. Original patch by Masahiko Sawada. It requires the hex
reorganization patch first. The key patch is now 2.1MB because of the
tests, so attaching it here seems unwise:https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
https://github.com/postgres/postgres/compare/master...bmomjian:key.diffI will add it to the commitfest. I think we need to figure out how much
of the tests we want to add.
I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019
with zero-length input data (no -p), while Stephen is able for those
tests to pass. This needs more research, plus I think higher-level
tests.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Fri, Jan 1, 2021 at 01:07:50AM -0500, Bruce Momjian wrote:
On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote:
I have completed the key management patch with tests created by Stephen
Frost. Original patch by Masahiko Sawada. It requires the hex
reorganization patch first. The key patch is now 2.1MB because of the
tests, so attaching it here seems unwise:https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
https://github.com/postgres/postgres/compare/master...bmomjian:key.diffI will add it to the commitfest. I think we need to figure out how much
of the tests we want to add.I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019
with zero-length input data (no -p), while Stephen is able for those
tests to pass. This needs more research, plus I think higher-level
tests.
I have found the cause of the failure, which I added as a C comment:
/*
* OpenSSL 1.1.1d and earlier crashes on some zero-length plaintext
* and ciphertext strings. It crashes on an encryption call to
* EVP_EncryptFinal_ex(() in GCM mode of zero-length strings if
* plaintext is NULL, even though plaintext_len is zero. Setting
* plaintext to non-NULL allows it to work. In KW/KWP mode,
* zero-length strings fail if plaintext_len = 0 and plaintext is
* non-NULL (the opposite). OpenSSL 1.1.1e+ is fine with all options.
*/
else if (cipher == PG_CIPHER_AES_GCM)
{
plaintext_len = 0;
plaintext = pg_malloc0(1);
}
All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)
My next step is to add the high-level tests.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On 2021-Jan-07, Bruce Momjian wrote:
All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)
So the tests are about 95% of the patch ... do we really need that many
tests?
--
�lvaro Herrera
On Thu, Jan 7, 2021 at 04:08:49PM -0300, �lvaro Herrera wrote:
On 2021-Jan-07, Bruce Momjian wrote:
All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)So the tests are about 95% of the patch ... do we really need that many
tests?
No, I don't think so. Stephen imported the entire NIST test suite. It
was so comperhensive, it detected several OpenSSL bugs for zero-length
strings, which I already reported, but we would never be encrypting
zero-length strings, so there wasn't a lot of value to it.
Anyway, I think we need to figure out how to trim. The first part would
be to figure out whether we need 128 _and_ 256-bit tests, and then see
what items are really useful. Stephen, do you have any ideas on that?
We currently have 10296 tests, and I think we could get away with 100.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On Thu, Jan 7, 2021 at 10:02:14AM -0500, Bruce Momjian wrote:
My next step is to add the high-level tests.
Here is the high-level script, and the log output. I used the
pg_upgrade test.sh as a model.
It uses "CFE DEBUG" lines that are already in the code to compare the
initdb encryption with the other initdb decryption and pg_ctl
decryption. It was easier than I thought.
What it does not do is to test the file descriptor passing from
/dev/tty, or the sample scripts. This seems acceptable to me since I
test them and they rarely change.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Greetings,
* Bruce Momjian (bruce@momjian.us) wrote:
On Thu, Jan 7, 2021 at 04:08:49PM -0300, Álvaro Herrera wrote:
On 2021-Jan-07, Bruce Momjian wrote:
All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)So the tests are about 95% of the patch ... do we really need that many
tests?No, I don't think so. Stephen imported the entire NIST test suite. It
was so comperhensive, it detected several OpenSSL bugs for zero-length
strings, which I already reported, but we would never be encrypting
zero-length strings, so there wasn't a lot of value to it.
I ran the entire test suite locally to ensure everything worked, but I
didn't actually include all of it in the PR which you merged- I had
already reduced it quite a bit by removing all 'additional
authenticated data' test cases (which the tests will automatically skip
and which we haven't implemented support for in the common library
wrappers) and by removing the 192-bit cases. This reduced the overall
test set by about 2/3rd's or so, as I recall.
Anyway, I think we need to figure out how to trim. The first part would
be to figure out whether we need 128 _and_ 256-bit tests, and then see
what items are really useful. Stephen, do you have any ideas on that?
We currently have 10296 tests, and I think we could get away with 100.
Yeah, it's probably still too much, but I don't have any particularly
justifiable suggestions as to exactly what we should remove or what we
should keep.
Perhaps it'd make sense to try and cover the cases that are more likely
to be issues between our wrapper functions and OpenSSL, and not stress
too much about constantly testing cases that should really be up to
OpenSSL. As such, I'd propose:
- Add back in some 192-bit tests, so we cover all three bit lengths.
- Add back in some additional authenticated test cases, just to make
sure that, until/unless we implement support, the test code properly
skips over those.
- Keep tests for various length plaintext/ciphertext (including 0-byte
cases, so we make sure those work, since they really should).
- Keep at least one test for each length of tag that's included in the
test suite.
I'm not sure how many tests we'd end up with from that, but my swag /
gut feeling is that it'd probably be on the order of 100ish and a small
enough set that it won't dwarf the rest of the patch.
Would be nice if we had a way for some buildfarm animal or something to
pull in the entire suite and test it, imv.. If anyone wants to
volunteer, I'd be happy to explain how to make that happen (it's not
hard though- download/unzip the files, drop them in the directory,
update the test script to add all the files into the array).
Thanks,
Stephen
Greetings Bruce,
* Bruce Momjian (bruce@momjian.us) wrote:
On Fri, Jan 1, 2021 at 01:07:50AM -0500, Bruce Momjian wrote:
On Thu, Dec 31, 2020 at 11:50:47PM -0500, Bruce Momjian wrote:
I have completed the key management patch with tests created by Stephen
Frost. Original patch by Masahiko Sawada. It requires the hex
reorganization patch first. The key patch is now 2.1MB because of the
tests, so attaching it here seems unwise:https://github.com/postgres/postgres/compare/master...bmomjian:hex.diff
https://github.com/postgres/postgres/compare/master...bmomjian:key.diffI will add it to the commitfest. I think we need to figure out how much
of the tests we want to add.I am getting regression test errors using OpenSSL 1.1.1d 10 Sep 2019
with zero-length input data (no -p), while Stephen is able for those
tests to pass. This needs more research, plus I think higher-level
tests.I have found the cause of the failure, which I added as a C comment:
/*
* OpenSSL 1.1.1d and earlier crashes on some zero-length plaintext
* and ciphertext strings. It crashes on an encryption call to
* EVP_EncryptFinal_ex(() in GCM mode of zero-length strings if
* plaintext is NULL, even though plaintext_len is zero. Setting
* plaintext to non-NULL allows it to work. In KW/KWP mode,
* zero-length strings fail if plaintext_len = 0 and plaintext is
* non-NULL (the opposite). OpenSSL 1.1.1e+ is fine with all options.
*/
else if (cipher == PG_CIPHER_AES_GCM)
{
plaintext_len = 0;
plaintext = pg_malloc0(1);
}All the tests pass now. The current src/test directory is 19MB, and
adding these tests takes it to 23MB, or a 20% increase. That seems like
a lot. It is testing 128-bit and 256-bit keys --- should we do fewer
tests, or just test 256, or use gzip to compress the tests by 50%?
(Does every platform have gzip?)
Thanks a lot for working on this and figuring out what the issue was and
fixing it! That's great that we got all those cases passing for you
too.
Thanks again,
Stephen
On Fri, Jan 8, 2021 at 03:33:44PM -0500, Stephen Frost wrote:
No, I don't think so. Stephen imported the entire NIST test suite. It
was so comperhensive, it detected several OpenSSL bugs for zero-length
strings, which I already reported, but we would never be encrypting
zero-length strings, so there wasn't a lot of value to it.I ran the entire test suite locally to ensure everything worked, but I
didn't actually include all of it in the PR which you merged- I had
already reduced it quite a bit by removing all 'additional
authenticated data' test cases (which the tests will automatically skip
and which we haven't implemented support for in the common library
wrappers) and by removing the 192-bit cases. This reduced the overall
test set by about 2/3rd's or so, as I recall.
Wow, so that was reduced!
Anyway, I think we need to figure out how to trim. The first part would
be to figure out whether we need 128 _and_ 256-bit tests, and then see
what items are really useful. Stephen, do you have any ideas on that?
We currently have 10296 tests, and I think we could get away with 100.Yeah, it's probably still too much, but I don't have any particularly
justifiable suggestions as to exactly what we should remove or what we
should keep.Perhaps it'd make sense to try and cover the cases that are more likely
to be issues between our wrapper functions and OpenSSL, and not stress
too much about constantly testing cases that should really be up to
OpenSSL. As such, I'd propose:- Add back in some 192-bit tests, so we cover all three bit lengths.
- Add back in some additional authenticated test cases, just to make
sure that, until/unless we implement support, the test code properly
skips over those.
- Keep tests for various length plaintext/ciphertext (including 0-byte
cases, so we make sure those work, since they really should).
- Keep at least one test for each length of tag that's included in the
test suite.
Makes sense. I did a simplistic trim-down to 90 tests but it still was
40% of the patch; attached. The hex strings are very long.
I'm not sure how many tests we'd end up with from that, but my swag /
gut feeling is that it'd probably be on the order of 100ish and a small
enough set that it won't dwarf the rest of the patch.Would be nice if we had a way for some buildfarm animal or something to
pull in the entire suite and test it, imv.. If anyone wants to
volunteer, I'd be happy to explain how to make that happen (it's not
hard though- download/unzip the files, drop them in the directory,
update the test script to add all the files into the array).
Yes, do we have a place to store more comprehensive tests outside of our
git tree? Has this been done before?
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
cryptotest.tgzapplication/x-gtar-compressedDownload
� ���_ ��]�v;v���~E9������@�G�H�m�j�� H.�:�
IFw���u�}���U��$����J�k��y�{-~�99��z�/�x�?�_��_���W������W�����{,��������Zx�?�Ok5F����s����o�Y~���9����������_��?����������_��o����������������?�����r����O�����k�������[�o�r��_��O����?�_�_����3~���������������?���������}�oc����Q��M!������o���?�?������~�������B��������������c��d�������7��G�����_�'�������z�q>9�6r��}e�'����+�<�~Fj)���w��a�6�s�.��y�������i-��5���|�w�|v�e��~���o���o=�����������9�b%�z��g��Z!����1����yy�r�'��vY1���������rr�<�s��Bm�?a���lm|�)+��j\�9�<q����y���u�-���X��G�(��<�$6�(�[%�1*���{�3w�%��<���}�w���j��� ����g�o���ZjxOg
��������0{|�o�7�����<�o��O�������:�w��o���h���������^,�yF��(_�%�������w~=�5j��S{;������z��������X���;��/s��t>o��URx��'��n���Q�5K������W��e���s7��{w��W�^#���/��r�k0�u�Z;���Km>�r(N'��:�����4y��>"%��6��r����q���y�7jj�a!?�N��:C���-~����U��|�cx�g1��j�5�7��c�� �a���+?VIz���k���y��;�n9�����m�2�3v8%�0c��&w/�}vde�}�����d��>Z���w��U��'�����������K�l��gn_�x����o����b����o�Z��`�&�t|��e��o�%�� ���OJ�����>�}�������/��L��0���WG*��|��Q}��XA���vM�k��Q�R�����&;����;~���[Yk~%�)�,�SXi��W)y�_"����|d�M�d������d�V��e��S��������V9Oa}�����v�zs�F�|x,�����g�%��"`�����7�TtfY�DN~�����x�����;����c���t��W�k�#�A�����v��k�WI���C*��~?�����g�T&�;b;�=,�E�yf����%����*y~������!�Q�7��l�R��sZ��J��$B�d����<�y��#�'F�)a�������O,�����Y�,���Y�0��c����`�����x��
��0Y���)���H����q�}�u�B��"�9}��3XE,���h�>u��!�UF������\�I��Ur/u�-x����=��L�$�b���J����~u0K�/����M&�?dTJ%�D�k����y����Y��oF|���� Ie�5���,ubW��U�>8?�W�<���o���a��";#�F$�O�����L�0��_R�����
��Og���S`��BD�������3+[���E;5��>
a�t"_�Va$W �,���L���+a����lo}�~�
������)��\�M<��O,|������`�%����&&���v�;;�`�I4���%gLbl`C�h���K��43����c���1Wc
V����q�Bj�$�'���������Zu���&�W2���,�����KNe��gt�Ig��������@<�W�W~�����X����>���N�Y���s�����d�@��e��I���LH6ba��l���c�@�,jz�h�q��O"
1�3M0O�p{�c���7����:;��D��H)��?���gX�6��:��� �|�|s0~�9��������:?�d��W��_�J.���&"�4�`�e#ib /@�Y/?S���A�j$�x�'���;��&��N�e�y��kb�����f$>�� �#�l���`$�g~��W@�T������>b
�Li* la�&��t�8i�����!%d��m�B���WR��H�;��\���)�0��� ����"���HP-`O��K��o�[`�K���� fn�W����]=����&