Re: Proposed patch for key managment

On Tue, Dec 15, 2020 at 02:09:09PM -0500, Stephen Frost wrote:
> Greetings,
>
> * Bruce Momjian (bruce(at)momjian(dot)us) wrote:
> > On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
> > > On Mon, Dec 14, 2020 at 06:06:15PM -0500, Bruce Momjian wrote:
> > > > I am getting close to applying these patches, probably this week. The
> > > > patches are cumulative:
> > > >
> > > > https://github.com/postgres/postgres/compare/master...bmomjian:key.diff
> > > > https://github.com/bmomjian/postgres/compare/key...bmomjian:key-alter.diff
> > >
> > > I strongly object to a commit based on the current state of the patch.
>
> Yeah, I agree that this could use some work though I don't think it's
> all that far away from being something we can get in, to allow us to
> move forward with the other work around supporting TDE.

Glad to hear that. Michael Paquier was right that the common/cipher*.c
API was wrong and should not be committed until fixed, which I think it
has been. If there are other messy parts of this patch, I would like to
fix those too.

> > I have posted some of the scripts. They involved complex shell
> > scripting that I doubt the average user can do. The scripts are for
> > prompting for a passphrase from the user's terminal, or using the
> > Yubikey, with our without typing a pin. I can put them in the docs and
> > then users can copy them into a file. Is that the preferred method?
>
> If we are going to include these in core anywhere, I would think a new
> directory under contrib would make the most sense. I'd hope that we
> could find a way to automate the testing of them though, so that we have
> some idea when they break because otherwise I'd be concerned that
> they'll break somewhere down the line and we won't notice for quite a
> while.

I am doing automated testing here, but I have a Yubikey. Michael
Paquier recommended TAP tests, I guess like we do for pg_upgrade, and I
will look into that.

> This doesn't seem like something that would make sense to only have in
> the documentation, which isn't a terribly convenient way to make use of
> them.

Yeah, it is hard to cut-paste 60 lines. The /contrib directory would be
for SSL and cluster file encryption passphrase control, so it should
have more general usefulness. Would those file be copied to the install
directory somewhere if you install /contrib? Do we have any cases of
this?

> > The point is that the command-line options will be active, but will not
> > be documented. It will not do anything on its own unless you specify
> > that command-line option. I can comment-out the command-line options
> > from being active but the code it going to look very messy.
>
> I'm a bit concerned with what's being contemplated here.. Ideally,
> we'll actually get everything committed for v14 but if we don't and this
> doesn't serve any use-case then I'm not sure that it should be
> included in the release. I also don't like committing and reverting
> things either, but having command line options that aren't documented
> but which exist in the code isn't great either, nor is having random
> code commented out..

Agreed. Once we use this code for SSL passphrase prompting, many of the
options will actually have usefulness. What we could do is to not hide
anything, including the docs, and then hide them once we are ready to
start beta. At that point, maybe putting in the #ifdefs will be
acceptable, since we would not be working on the feature until we
branch, and then we can just remove them again. We had similar issues
with the Win32 port, but that had fewer visible knobs.

> > > I think that you should attach such patches directly to the emails
> > > sent to pgsql-hackers, if those github links disappear for some
> > > reason, it would become impossible to refer to see what has been
> > > discussed here.
> >
> > Well, the patches are changing frequently. I am attaching a version to
> > this email.
>
> I agree with having the patches posted to the list. I don't really
> agree with the argument of "they change frequently".

Sure, they are only 35k compressed. My point is that I am modifying my
git tree all day, and with github, I can easily push the changes and
when someone looks at the diff, they see the most recent version.

> > I think that designing a correct set of APIs that can be plugged with
> > any SSL library is the correct move in the long term. I have on my
> > agenda to clean up HMAC as SCRAM uses that with SHA256 and you would
> > use that with SHA512. Daniel has mentioned that he has been touching
> > this area, and I also got a patch halfly done though pgcrypto needs
> > some extra thoughts. So this is still WIP but you could reuse that
> > here.
>
> Yeah, looking at what's been done there seems like the right approach to
> me as well, ideally we'd have one set of APIs that'll support all these
> use cases (not unlike what curl does..).

I think I accomplished this in a patch I just posted.

> The other thought I had here off-hand was that we might want to randomly
> generate a key during startup and have that available and use it for
> anything that's temporary and which is going to get blown away on a
> restart, I've been thinking that doing that might allow us to have a
> relatively simpler API for transient/temporary files too.

That is easy to do because of the design. If you want it done now, I
can to it --- just let me know.

I want to thank everyone for the very helpful feedback I have received
since posting the first version of this patch.

--
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