Re: Key management with tests - Mailing list pgsql-hackers
From | Bruce Momjian |
---|---|
Subject | Re: Key management with tests |
Date | |
Msg-id | 20210116002132.GG8740@momjian.us Whole thread Raw |
In response to | Re: Key management with tests (Andres Freund <andres@anarazel.de>) |
Responses |
Re: Key management with tests
|
List | pgsql-hackers |
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@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
Attachment
pgsql-hackers by date: