Date: Fri, 18 Dec 2020 12:02:50 -0500
From: Bruce Momjian <bruce@momjian.us>
To: Stephen Frost <sfrost@snowman.net>
Cc: Michael Paquier <michael@paquier.xyz>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Subject: Re: Proposed patch for key managment
Message-ID: <20201218170250.GE28841@momjian.us>
References: <X9lhi1ht04I+v/rV@paquier.xyz>
 <20201216220412.GB4527@momjian.us>
 <X9qXuLekePLRYrB2@paquier.xyz>
 <20201217163954.GY16415@tamriel.snowman.net>
 <20201217171022.GE23260@momjian.us>
 <X9v/Yo8sLB/GKLvt@paquier.xyz>
 <20201218031414.GA28841@momjian.us>
 <20201218131853.GA16415@tamriel.snowman.net>
 <20201218154446.GC28841@momjian.us>
 <20201218161344.GB16415@tamriel.snowman.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201218161344.GB16415@tamriel.snowman.net>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Fri, Dec 18, 2020 at 11:13:44AM -0500, Stephen Frost wrote:
> Greetings,
> 
> * Bruce Momjian (bruce@momjian.us) wrote:
> > On Fri, Dec 18, 2020 at 08:18:53AM -0500, Stephen Frost wrote:
> > > - C API in backend - we should try to at least set up the structure to
> > >   allow multiple encryption implementations, either via different
> > >   libraries or if someone feels it'd be useful to write a built-in
> > >   implementation, but as I mentioned just a moment ago, we aren't going
> > >   to get this perfect and we should accept that.
> > 
> > All my OpenSSL-specific stuff is isolated in src/common.
> 
> ... and in 'openssl' files, with 'generic' functions on top, right?
> That's what I recall from before and seems like the right approach to at
> least start with, and then we likely make adjustments as needed to the
> API when we add another encryption library.

Uh, I did it the way cryptohash_openssl.c does it. Sawada-san's patch did
it kind of like that, but had #ifdef calls to OpenSSL versions, while
the cryptohash_openssl.c method has two files with exactly the same
function names, and they are conditionally compiled in the makefile
based on makefile defines, and that is how I did it.  Michael Paquier
pointed this out, and the msvc changes needed to handle it.

> > > > I split apart my patch to create cipher{_openssl}.c and hmac{_openssl}.c
> > > > so the single HMAC function is not in the cipher file anymore, attached.
> > > 
> > > Will try to look at this soon, but in general the idea seems right.
> > 
> > Should I be using the init/update/final HMAC API that SCRAM uses so it
> > is easier to integrate into Michael's patch?  I can do that, but didn't
> > because that is going to require me to create a much larger footprint in
> > the main code, and that might be harder to integrate once Michael's API
> > is final.  It is easier for me to change one line to five lines than to
> > change five lines to five different lines.
> 
> For my 2c, ideally we'd get a review done of Michael's changes and just
> get that committed and have your work here rebased over that.  I don't

That would be nice.

> see any reason why we can't get that done in relatively short order.
> Just because it's registered in the January CF doesn't mean we actually
> have to wait for that to get done, it just needs a review from another
> committer (or self certification from Michael if he's happy with it).

Agreed.  I just don't want to wait until mid-January to get this into
the tree, and I am going to defer to Michael's timeline on this, which
is why I figured I might need to go first.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

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