From: Bruce Momjian <bruce@momjian.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Tom Kincaid <tomjohnkincaid@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Subject: Re: Key management with tests
Date: Tue, 26 Jan 2021 17:53:01 -0500
Message-ID: <20210126225301.GD32305@momjian.us> (raw)
In-Reply-To: <CA+TgmoaiKL6q4mAXtJWjAOt-NGyF39_q08ugRk9HXdvsoprPnQ@mail.gmail.com>
References: <20210125191805.GD27081@momjian.us>
<20210125231201.GA10108@alvherre.pgsql>
<20210126000944.GA18075@momjian.us>
<20210126032718.GB18075@momjian.us>
<20210126161548.GA32305@momjian.us>
<CA+TgmoaiKL6q4mAXtJWjAOt-NGyF39_q08ugRk9HXdvsoprPnQ@mail.gmail.com>
On Tue, Jan 26, 2021 at 03:24:30PM -0500, Robert Haas wrote:
> On Tue, Jan 26, 2021 at 11:15 AM Bruce Momjian <bruce@momjian.us> wrote:
> > This version fixes OpenSSL detection and improves docs for initdb
> > interactions.
>
> Hi,
>
> I'm wondering whether you've considered storing all the keys in one
> file instead of a file per key. The reason I ask is that it seems to
> me that the key rotation procedure would be a lot simpler if it were
> all in one file. You could just create a temporary file and atomically
> rename it over the existing file. If you see a temporary file you're
> always free to remove it. This is a lot simpler than what you have
> right now. The "repair" concept pretty much goes away completely,
> which seems nice. Granted I don't know exactly how to store multiple
> keys in one file, but I bet there's some way to do it.
We envisioned allowing heap/index key rotation by having a standby with
the same WAL key as the primary but different heap/index keys so that we
can failover to the standby to change the heap/index key and then change
the WAL key. This separation allows that. We also might need some
additional keys later and this allows that. I do like simplicity, but
the complexity here seems to serve a need.
> The way in which you are posting these patches is quite unlike what
> most people do when posting patches to this list. You seem to have
> generated a bunch of patches using 'git format-patch' but then
> concatenated them all together in a single file. It would be helpful
> if you could do this more like the way that is now standard on this
> list. Not only that, but the patches don't have meaningful commit
What is the standard? You want seven separate files? I can do that.
> messages in them, and don't seem to be meaningfully split for easy
> review. They just say things like 'crypto squash commit'. Compare this
Yes, the feature is at the backend, common, /bin, and test levels. I
was able to separate out the bin, pg_alterckey and test stuff, but the
backend interactions were hard to split.
> to for example what I did on the "cleaning up a few CLOG-related
> things" thread where the commits appear in a logical sequence, and
> each one has a meaningful commit message. Or here's an example from
> someone else --
> http://postgr.es/m/be72abfa-e62e-eb81-4e70-1b57fe6dc9e2@amazon.com --
> and note the inclusion of authorship information in the commit
> messages, so that the source of the code can be easily understood.
I see. I am not sure how to do that easily for all the pieces.
> The README in src/backend/crypto does not explain how the scripts in
> that directory are intended to be used. If I want to use AWS Secrets
> Manager with this feature, I can see that I should use
> ckey_aws.sh.sample as a basis for that integration, but I don't know
> what I should do with the script because the README says nothing about
> it. I am frankly pretty doubtful about the idea of shipping a bunch of
> /bin/sh scripts as a best practice; for one thing, that's totally
> unusable on Windows, and it also means that this is dependent on
> /bin/sh existing and having the behavior we expect and on all the
> other executables in these scripts as well. But, at the very least,
> there needs to be a clearer explanation of how the scripts are
> intended to be used, which parts people are supposed to modify, what
> arguments they're going to get called with, and things like that.
I added comments to most of the scripts. I don't know what more I can
do, or what other language would be appropriate.
> The comments in cipher.c and cipher_openssl.c could be improved to
> explain that they are alternatives to each other. Perhaps the former
> could be renamed to something like cipher_failure.c or cipher_noimpl.c
> for clarity.
This follows the way cryptohash.c and cryptohash_openssl.c are done. I
did just add comments to the top of cipher.c and cipher_openssl.c to be
just like cryptohash versions.
> I believe that a StaticAssertStmt could be used to check the length of
> the encryption_methods[] array, so that if someone changes
> NUM_ENCRYPTION_METHODS without updating the array, compilation fails.
> See UserAuthName[] for an example of how to do this.
Sure, good idea, done.
> You seem to have omitted to update the documentation with the names of
> the new wait events that you added.
OK, added.
> In process_postgres_switches(), when there's a multi-line comment
> followed by a single line of actual code, I prefer to include braces
> around the whole thing. There might be some disagreement on what is
> best here, though.
OK, done.
> What are the consequences of the placement of the code in
> PostgresMain() for processes other than user backends and walsenders?
> I think that the way you have it, background workers would not have
> access to keys, nor auxiliary processes like the checkpointer ... at
Well, there are three cases, --boot mode, postmaster mode, and postgres
single-user mode. I tried to have all those cases only unwrap the keys
once and store them in shared memory, or in boot mode, in local memory.
As far as I know, the startup does it once and everyone else uses shared
memory to access it.
> least in the EXEC_BACKEND case. In the non-EXEC_BACKEND case you have
> the postmaster doing it, so then I'm not sure why it has to be redone
> for every backend. Won't they just inherit the data from the
For postgres --single.
> postmaster? Has this code been meaningfully tested on Windows? How do
No, just by the cfbot Windows machine.
> we know that it works? Maybe we need to think about adding some
> asserts that guarantee that any process that attempts to access a
> buffer has the key manager initialized; I bet such assertions would
> fail at least on Windows as the code looks now.
Are you saying we should set a global variable and throw an error if it
is accessed without the array being initialized?
> I don't think it makes sense to think about committing this to v14. I
> believe it only makes sense if we have a TDE patch that is relatively
> close to committable that can be used with it. I also don't think that
> this patch is in good enough shape to commit yet in terms of where
> it's at in terms of quality; I think it needs more review first,
> hopefully including review from people who can comment intelligently
> specifically on the cryptography aspects of it. However, the
> challenges don't seem insurmountable. There's also still some question
> in my mind about whether the design choices here (one KEK, 2 DEKs, one
> for data and one for WAL) have enough consensus. I don't have a
> considered opinion on that, partly because I'm not quite sure what the
> reasonable alternatives are, but it seems that other people had some
> questions about it, IIUC.
While I am willing to make requested adjustments to the patch, I don't
plan to work on this feaure any further, assuming your analysis above is
correct. If after years we are still not sure this is the right
direction, I don't see any point in moving forward with the later
pieces, which are even more complicated. I will join the group of
people that feel there will never be consensus on implementing this
feature in the community, so it is not worth trying.
I would also like to add a "not wanted" entry for this feature on the
TODO list, baaed on the feature's limited usefulness, but I already
asked about that and no one seems to feel we don't want it.
I now better understand why the OpenSSL project has had such serious
problems in the past.
Updated patch attached as seven attachments.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
Attachments:
0001-crypto-squash-commit.patch.gz (application/gzip, 10.8K) download
0002-common-squash-commit.patch.gz (application/gzip, 7.3K) download
0003-backend-squash-commit.patch.gz (application/gzip, 6.8K) download
0004-pg_alterckey-squash-commit.patch.gz (application/gzip, 8.1K) download
0005-bin-squash-commit.patch.gz (application/gzip, 9.3K) download
0006-test-squash-commit.patch.gz (application/gzip, 82.5K) download
0007-key-squash-commit.patch.gz (application/gzip, 3.3K) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bruce@momjian.us, robertmhaas@gmail.com, alvherre@alvh.no-ip.org, tomjohnkincaid@gmail.com, andres@anarazel.de, amit.kapila16@gmail.com, thomas.munro@gmail.com, sfrost@snowman.net, masahiko.sawada@2ndquadrant.com
Subject: Re: Key management with tests
In-Reply-To: <20210126225301.GD32305@momjian.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox