Re: Proposed patch for key managment

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.

> > > There are a few shell script I should include to show how to create
> > > commands. Where should they be stored? /contrib module?
> >
> > Why are these needed. Is it a matter of documentation?
>
> 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.

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.

> > > Are people okay with having the feature enabled, but invisible
> > > since the docs and --help output are missing? When we enable
> > > ssl_passphrase_command to prompt from the terminal, some of the
> > > command-line options will be useful.
> >
> > You are suggesting to enable the feature by default, make it invisible
> > to the users and leave it undocumented? Is there something I missing
> > here?
>
> 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..

> > > Do people like the command-letter choices?
> > >
> > > I called the alter passphrase utility pg_altercpass. I could
> > > have called it pg_clusterpass, but I wanted to highlight it is
> > > only for changing the passphrase, not for creating them.
> >
> > 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".

* Michael Paquier (michael(at)paquier(dot)xyz) wrote:
> On Mon, Dec 14, 2020 at 10:19:02PM -0500, Bruce Momjian wrote:
> > On Tue, Dec 15, 2020 at 10:05:49AM +0900, Michael Paquier wrote:
> >> - The cipher split also should be done in its own patch, and reviewed
> >> on its own. There is a mix of dependencies between non-OpenSSL and
> >> OpenSSL code paths, making the pluggin of a new SSL library harder to
> >> do. In short, this requires proper API design, which is not something
> >> we have here. There are also no changes in pgcrypto for that stuff.
> >
> > I am going to need someone to help me make these changes. I don't feel
> > I know enough about the crypto API to do it, and it will take me 1+ week
> > to learn it.
>
> 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..).

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.

Thanks,

Stephen