Key management with tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
74 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

Key management with tests

Bruce Momjian
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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
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.diff
>
> I 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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
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.diff
> >
> > I 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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Álvaro Herrera
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


Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
In reply to this post by Bruce Momjian
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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee

Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Stephen Frost
In reply to this post by Bruce Momjian
Greetings,

* Bruce Momjian ([hidden email]) 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Stephen Frost
In reply to this post by Bruce Momjian
Greetings Bruce,

* Bruce Momjian ([hidden email]) 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.diff
> > >
> > > I 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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
In reply to this post by Stephen Frost
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  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee


cryptotest.tgz (101K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
In reply to this post by Stephen Frost
On Fri, Jan  8, 2021 at 03:34:23PM -0500, Stephen Frost 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?)
>
> 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.

Yes, I was relieved.  The pattern of when zero-length strings fail in
which modes is still very odd, but at least it reports an error, so it
isn't returning incorrect data.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Stephen Frost
In reply to this post by Bruce Momjian
Greetings,

* Bruce Momjian ([hidden email]) wrote:

> On Fri, Jan  8, 2021 at 03:33:44PM -0500, Stephen Frost wrote:
> > > 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 don't think we actually need to stress over the size of the test data
relative to the size of the patch- it's not like it's all that much perl
code.  I can appreciate that we don't want to add megabytes worth of
test data to the git repo though.

> > 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?
Not that I'm aware of.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
In reply to this post by Bruce Momjian
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.diff
> >
> > I 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 know we are still working on the hex patch (dest-len) and the crypto
tests, but I wanted to post this so people can see where we are, and we
can get some current cfbot testing.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee


hex.diff.gz (4K) Download Attachment
key.diff.gz (165K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
On Sat, Jan  9, 2021 at 01:17:36PM -0500, Bruce Momjian 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.diff
> > >
> > > I 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 know we are still working on the hex patch (dest-len) and the crypto
> tests, but I wanted to post this so people can see where we are, and we
> can get some current cfbot testing.
Here is an updated version that covers all the possible
testing/configuration options.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee


hex.diff.gz (4K) Download Attachment
hex..key.diff.gz (165K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
On Sat, Jan  9, 2021 at 08:08:16PM -0500, Bruce Momjian wrote:
> On Sat, Jan  9, 2021 at 01:17:36PM -0500, Bruce Momjian wrote:
> > I know we are still working on the hex patch (dest-len) and the crypto
> > tests, but I wanted to post this so people can see where we are, and we
> > can get some current cfbot testing.
>
> Here is an updated version that covers all the possible
> testing/configuration options.

Does anyone know why the cfbot applied the patch listed second first
here?

        http://cfbot.cputube.org/patch_31_2925.log

Specifically, it applied hex..key.diff.gz before hex.diff.gz.  I assumed
it would apply attachments in the order they appear in the email.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Thomas Munro-5
On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian <[hidden email]> wrote:
> Does anyone know why the cfbot applied the patch listed second first
> here?
>
>         http://cfbot.cputube.org/patch_31_2925.log
>
> Specifically, it applied hex..key.diff.gz before hex.diff.gz.  I assumed
> it would apply attachments in the order they appear in the email.

It sorts the filenames (in this case after decompressing step removes
the .gz endings).  That works pretty well for the patches that "git
format-patch" spits out, but it's a bit hit and miss with cases like
yours.


Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
On Sun, Jan 10, 2021 at 06:04:12PM +1300, Thomas Munro wrote:

> On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian <[hidden email]> wrote:
> > Does anyone know why the cfbot applied the patch listed second first
> > here?
> >
> >         http://cfbot.cputube.org/patch_31_2925.log
> >
> > Specifically, it applied hex..key.diff.gz before hex.diff.gz.  I assumed
> > it would apply attachments in the order they appear in the email.
>
> It sorts the filenames (in this case after decompressing step removes
> the .gz endings).  That works pretty well for the patches that "git
> format-patch" spits out, but it's a bit hit and miss with cases like
> yours.
OK, here they are with numeric prefixes.  It was actually tricky to
figure out how to create a squashed format-patch based on another branch.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee


01-hex.diff.gz (4K) Download Attachment
02-hex..key.diff.gz (165K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Masahiko Sawada
On Sun, Jan 10, 2021 at 11:51 PM Bruce Momjian <[hidden email]> wrote:

>
> On Sun, Jan 10, 2021 at 06:04:12PM +1300, Thomas Munro wrote:
> > On Sun, Jan 10, 2021 at 3:45 PM Bruce Momjian <[hidden email]> wrote:
> > > Does anyone know why the cfbot applied the patch listed second first
> > > here?
> > >
> > >         http://cfbot.cputube.org/patch_31_2925.log
> > >
> > > Specifically, it applied hex..key.diff.gz before hex.diff.gz.  I assumed
> > > it would apply attachments in the order they appear in the email.
> >
> > It sorts the filenames (in this case after decompressing step removes
> > the .gz endings).  That works pretty well for the patches that "git
> > format-patch" spits out, but it's a bit hit and miss with cases like
> > yours.
>
> OK, here they are with numeric prefixes.  It was actually tricky to
> figure out how to create a squashed format-patch based on another branch.
>
Thank you for attaching the patches. It passes all cfbot tests, great.

Looking at the patch, it supports three algorithms but only
PG_CIPHER_AES_KWP is used in the core for now:

+/*
+ * Supported symmetric encryption algorithm. These identifiers are passed
+ * to pg_cipher_ctx_create() function, and then actual encryption
+ * implementations need to initialize their context of the given encryption
+ * algorithm.
+ */
+#define PG_CIPHER_AES_GCM          0
+#define PG_CIPHER_AES_KW           1
+#define PG_CIPHER_AES_KWP          2
+#define PG_MAX_CIPHER_ID           3

Are we in the process of experimenting which algorithms are better? If
we support one algorithm that is actually used in the core, we would
reduce the tests as well.

FWIW, I've written a PoC patch for buffer encryption to make sure the
kms patch would be workable with other components using the encryption
key managed by kmgr.

Overall it’s good. While the buffer encryption patch is still PoC
quality and there are some problems regarding nonce generation we need
to deal with, it easily can use the relation key managed by the kmgr
to encrypt/decrypt buffers.

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

0003-Poc-buffer-encryption.patch (32K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
On Mon, Jan 11, 2021 at 08:12:00PM +0900, Masahiko Sawada wrote:
> On Sun, Jan 10, 2021 at 11:51 PM Bruce Momjian <[hidden email]> wrote:
> > OK, here they are with numeric prefixes.  It was actually tricky to
> > figure out how to create a squashed format-patch based on another branch.
>
> Thank you for attaching the patches. It passes all cfbot tests, great.

Yeah, I saw that.  :-)  I head to learn a lot about how to create
squashed format-patches on non-master branches.  I have now automated it
so it will be easy going forward.

> Looking at the patch, it supports three algorithms but only
> PG_CIPHER_AES_KWP is used in the core for now:
>
> +/*
> + * Supported symmetric encryption algorithm. These identifiers are passed
> + * to pg_cipher_ctx_create() function, and then actual encryption
> + * implementations need to initialize their context of the given encryption
> + * algorithm.
> + */
> +#define PG_CIPHER_AES_GCM          0
> +#define PG_CIPHER_AES_KW           1
> +#define PG_CIPHER_AES_KWP          2
> +#define PG_MAX_CIPHER_ID           3
>
> Are we in the process of experimenting which algorithms are better? If
> we support one algorithm that is actually used in the core, we would
> reduce the tests as well.

I think we are only using KWP (Key Wrap with Padding) because that is
for wrapping keys:

        https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/KWVS.pdf

I am not sure about KW.  I think we are using GCM for the WAP/heap/index
pages.  Stephen would know more.

> FWIW, I've written a PoC patch for buffer encryption to make sure the
> kms patch would be workable with other components using the encryption
> key managed by kmgr.

Wow, it is a small patch --- nice.
 
--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Stephen Frost
Greetings,

* Bruce Momjian ([hidden email]) wrote:

> On Mon, Jan 11, 2021 at 08:12:00PM +0900, Masahiko Sawada wrote:
> > Looking at the patch, it supports three algorithms but only
> > PG_CIPHER_AES_KWP is used in the core for now:
> >
> > +/*
> > + * Supported symmetric encryption algorithm. These identifiers are passed
> > + * to pg_cipher_ctx_create() function, and then actual encryption
> > + * implementations need to initialize their context of the given encryption
> > + * algorithm.
> > + */
> > +#define PG_CIPHER_AES_GCM          0
> > +#define PG_CIPHER_AES_KW           1
> > +#define PG_CIPHER_AES_KWP          2
> > +#define PG_MAX_CIPHER_ID           3
> >
> > Are we in the process of experimenting which algorithms are better? If
> > we support one algorithm that is actually used in the core, we would
> > reduce the tests as well.
>
> I think we are only using KWP (Key Wrap with Padding) because that is
> for wrapping keys:
>
> https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/KWVS.pdf
Yes.

> I am not sure about KW.  I think we are using GCM for the WAP/heap/index
> pages.  Stephen would know more.

KW was more-or-less 'for free' and there were tests for it, which is why
it was included.  Yes, GCM would be for WAL/heap/index pages, it
wouldn't be appropriate to use KW or KWP for that.  Using KW/KWP for the
key wrapping also makes the API simpler- and therefore easier for other
implementations to be written which provide the same API.

> > FWIW, I've written a PoC patch for buffer encryption to make sure the
> > kms patch would be workable with other components using the encryption
> > key managed by kmgr.
>
> Wow, it is a small patch --- nice.

I agree that the actual encryption patch, for just the main heap/index,
won't be too bad.  The larger part will be dealing with all of the
temporary files we create that have user data in them...  I've been
contemplating a way to try and make that part of the patch smaller
though and hopefully that will bear fruit and we can avoid having to
change a lof of, eg, reorderbuffer.c and pgstat.c.

There's a few places where we need to be sure to be updating the LSN for
both logged and unlogged relations properly, including dealing with
things like the magic GIST "GistBuildLSN" fake-LSN too, and we will
absolutely need to have a bit used in the IV to distinguish if it's a
real LSN or an unlogged LSN.

Although, another approach and one that I've discussed a bit with Bruce,
is to have more keys- such as a key for temporary files, and perhaps
even a key for logged relations and a different for unlogged..  Or
perhaps sets of keys for each which automatically are rotating every X
number of GB based on the LSN...  Which is a big part of why key
management is such an important part of this effort.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Key management with tests

Bruce Momjian
On Mon, Jan 11, 2021 at 12:54:49PM -0500, Stephen Frost wrote:
> Although, another approach and one that I've discussed a bit with Bruce,
> is to have more keys- such as a key for temporary files, and perhaps
> even a key for logged relations and a different for unlogged..  Or

Yes, we have to make sure the nonce (computed as LSN/pageno) is never
reused, so if we have several LSN usage "spaces", they need different
data keys.

> perhaps sets of keys for each which automatically are rotating every X
> number of GB based on the LSN...  Which is a big part of why key
> management is such an important part of this effort.

Yes, this would avoid the need to failover to a standby for data key
rotation.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

  The usefulness of a cup is in its emptiness, Bruce Lee



1234