Re: Key management with tests

On Fri, Jan 15, 2021 at 02:37:56PM -0800, Andres Freund wrote:
> On 2021-01-15 16:47:19 -0500, Bruce Momjian wrote:
> > > I am not even sure there is a consensus on the design, without which
> > > any commit is always premature.
> >
> > If people want changes, I need to hear about it here. I have address
> > everything people have mentioned in these threads so far.
>
> I don't even know how anybody is supposed to realistically review the
> design or the patch:
>
> This thread started at
> https://postgr.es/m/20210101045047.GB30966%40momjian.us - there's no
> reference to any discussion of the design at all and the supposed links
> to code are dead.

You have to understand cryptography and Postgres internals to understand
the design, and I don't think it is realistic to explain that all to the
community. We did much of this in voice calls over months because it
was too much of a burden to explain all the cryptographic details so
everyone could follow along.

> The last version of the code that I see posted ([1]), has the useless
> commit message of "key squash commit" - nothing else. There's no design
> documentation included in the patch either, as far as I can tell.
>
> Manually searching for the topic brings me to
> https://www.postgresql.org/message-id/20201202213814.GG20285%40momjian.us
> , a thread of 52 messages, which provides a bit more context, but
> largely just references another thread and a wiki article. The link to
> the other thread is into the middle of a 112 message thread.
>
> The wiki page doesn't really describe a design either. It has a very
> long todo, a bunch of implementation details, but no design.

I am not sure what design document you are requesting. I thought the
TODO was that.

> Nor did 978f869b99 include much in the way of design description.
>
> You cannot expect anybody to review a patch if developing some basic
> understanding of the intended design requires reading hundreds of
> messages in which the design evolved. And I don't think it's acceptable
> to push it due to lack of further feedback, given this situation - the
> lack of design description is a blocker in itself.

OK, I will just move on to something else then. It is not worth the
feature to go into that kind of discussion again. I am willing to have
voice calls with individuals to explain the logic, but repeatedly
explaining it to the entire group I find unproductive. I don't think
another 400-email thread would help anyone.

> There's a few things that stand out on a very very brief scan:
> - the patch badly needs to be split up into independently reviewable
> pieces

I can do that, but there are enough complaints above that I feel it
would not be worth it.

> - tests:
> - wait, a .sh test script? No, we shouldn't add any more of those,
> they're nightmare across platforms

The script originatad from pg_upgrade. I don't know how to do things
like initdb and stuff another way, at least in our code.

> - Do the tests actually do anything useful? It's not clear to me what
> they are trying to achieve. En/Decrypting test vectors doesn't seem to
> buy that much?

Uh, that's because the key manager doesn't do anything useful yet.

> - the new pg_alterckey is completely untested

Wow, I was so excited I tested the data keys that I forgot to add the
pg_alterckey tests. My tests had that already. I have added it to the
attached patch.

> - the pg_upgrade paths is untested

Uh, I was waiting until we were actually encrypting some data to test
that.

> - ..
> - Without further comment BootStrapKmgr() does "copy cluster file
> encryption keys from an old cluster?", but there's no explanation as
> to why / when that's the case. Presumably pg_upgrade, but, uh, explain
> that.

Uh, the heap/index files are, in the future, encrypted with the keys of
the old cluster, so we just copy them to the new cluster and they keep
working. Potentially we could replace the WAL key at that point since
we don't move WAL from the old cluster to the new one, but we also need
a command-line tool to do that, so I figured I would just wait for that
to be done.

> - pg_alterckey.c
> - appears to create it's own cluster lock file, using its
> own routine for doing so. How does that lock file interact with the
> running server?

pg_alterckey runs fine while the old cluster is running, which is why I
used a new lock file. The keys are only read at db boot time.

> - retrieve_cluster_keys() is missing (void).

Oops, fixed.

> I think this is at the very least a month away from being committable,
> even if the design were completely correct (which I do not know, see
> above).

Those comments were very helpful, and I could certainly use more
feedback on the patch. Updated patch attached.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EnterpriseDB https://enterprisedb.com

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