Thread: Re: [PATCH] test/ssl: rework the sslfiles Makefile target

> On 4 Mar 2021, at 01:03, Jacob Champion <pchampion@vmware.com> wrote:

> Andrew pointed out elsewhere [1] that it's pretty difficult to add new
> certificates to the test/ssl suite without blowing away the current
> state and starting over. I needed new cases for the NSS backend work,
> and ran into the same pain, so here is my attempt to improve the
> situation.

Thanks for working on this, I second the pain cited.  I've just started to look
at this, so only a few comments thus far.

> The unused server-ss certificate has been removed entirely.

Nice catch, this seems to have been unused since the original import of the SSL
test suite.  To cut down scope of the patch (even if only a small bit) I
propose to apply this separately first, as per the attached.

> - Serial number collisions are less likely, thanks to Andrew's idea to
> use the current clock time as the initial serial number in a series.

+my $serialno = `openssl x509 -serial -noout -in ssl/client.crt`;
+$serialno =~ s/^serial=//;
+$serialno = hex($serialno); # OpenSSL prints serial numbers in hexadecimal

Will that work on Windows?  We don't currently require the openssl binary to be
in PATH unless one wants to rebuild sslfiles (which it is quite likely to be
but there should at least be errorhandling covering when it's not).

> - I am making _heavy_ use of GNU Make-isms, which does not improve
> long-term maintainability.

GNU Make is already a requirement, I don't see this shifting the needle in any
direction.

--
Daniel Gustafsson        https://vmware.com/

On Wed, 2021-07-28 at 00:24 +0200, Daniel Gustafsson wrote:
> > On 4 Mar 2021, at 01:03, Jacob Champion <pchampion@vmware.com> wrote:
> > Andrew pointed out elsewhere [1] that it's pretty difficult to add new
> > certificates to the test/ssl suite without blowing away the current
> > state and starting over. I needed new cases for the NSS backend work,
> > and ran into the same pain, so here is my attempt to improve the
> > situation.
> 
> Thanks for working on this, I second the pain cited.  I've just started to look
> at this, so only a few comments thus far.
> 
> > The unused server-ss certificate has been removed entirely.
> 
> Nice catch, this seems to have been unused since the original import of the SSL
> test suite.  To cut down scope of the patch (even if only a small bit) I
> propose to apply this separately first, as per the attached.

LGTM.

> > - Serial number collisions are less likely, thanks to Andrew's idea to
> > use the current clock time as the initial serial number in a series.
> 
> +my $serialno = `openssl x509 -serial -noout -in ssl/client.crt`;
> +$serialno =~ s/^serial=//;
> +$serialno = hex($serialno); # OpenSSL prints serial numbers in hexadecimal
> 
> Will that work on Windows?  We don't currently require the openssl binary to be
> in PATH unless one wants to rebuild sslfiles (which it is quite likely to be
> but there should at least be errorhandling covering when it's not).

Hm, that's a good point. It should be easy enough for me to add a
fallback if the invocation fails; I'll give it a shot tomorrow.

> > - I am making _heavy_ use of GNU Make-isms, which does not improve
> > long-term maintainability.
> 
> GNU Make is already a requirement, I don't see this shifting the needle in any
> direction.

As long as the .SECONDARYEXPANSION magic is clear enough to others, I'm
happy.

Thanks!
--Jacob

On 7/28/21 4:10 PM, Jacob Champion wrote:
>
>>> - I am making _heavy_ use of GNU Make-isms, which does not improve
>>> long-term maintainability.
>> GNU Make is already a requirement, I don't see this shifting the needle in any
>> direction.
> As long as the .SECONDARYEXPANSION magic is clear enough to others, I'm
> happy.
>

We don't currently have any, and so many of us (including me) will have
to learn to understand it. But that's not to say it's unacceptable. If
there's no new infrastructure requirement then I'm OK with it.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Jacob Champion <pchampion@vmware.com> writes:
> On Wed, 2021-07-28 at 00:24 +0200, Daniel Gustafsson wrote:
>> GNU Make is already a requirement, I don't see this shifting the needle in any
>> direction.

Um ... the existing requirement is for gmake 3.80 or newer;
if you want to use newer features we'd have to have a discussion
about whether it's worthwhile to move that goalpost.

> As long as the .SECONDARYEXPANSION magic is clear enough to others, I'm
> happy.

After reading the gmake docs about that, I'd have to say it's
likely to be next door to unmaintainable.  Do we really have
to be that cute?  And, AFAICT, it's not in 3.80.

            regards, tom lane

> On 28 Jul 2021, at 23:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Jacob Champion <pchampion@vmware.com> writes:

>> As long as the .SECONDARYEXPANSION magic is clear enough to others, I'm
>> happy.
>
> After reading the gmake docs about that, I'd have to say it's likely to be next
> door to unmaintainable.


Personally, I don’t think it’s that bad, but mileage varies.  It’s obviously a
show-stopper if maintainers don’t feel comfortable with it.

> And, AFAICT, it's not in 3.80.

That however, is a very good point that I missed.  I think it’s a good tool,
but probably not enough to bump the requirement.

--
Daniel Gustafsson        https://vmware.com/

On Wed, 2021-07-28 at 23:09 +0200, Daniel Gustafsson wrote:
> > On 28 Jul 2021, at 23:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Jacob Champion <pchampion@vmware.com> writes:
> > > As long as the .SECONDARYEXPANSION magic is clear enough to others, I'm
> > > happy.
> > 
> > After reading the gmake docs about that, I'd have to say it's likely to be next
> > door to unmaintainable.
> 
> Personally, I don’t think it’s that bad, but mileage varies.  It’s obviously a
> show-stopper if maintainers don’t feel comfortable with it.
> 
> > And, AFAICT, it's not in 3.80.
> 
> That however, is a very good point that I missed.  I think it’s a good tool,
> but probably not enough to bump the requirement.

No worries, it's easy enough to unroll the expansion manually. The
annoyances without secondary expansion are the duplicated lines for
each individual CA and the need to introduce .INTERMEDIATE targets so
that cleanup works as intended.

Attached is a v3 that does that, and introduces a fallback in case
openssl isn't on the PATH. I also missed a Makefile dependency on
cas.config the first time through, which has been fixed. The patch you
pulled out earlier is 0001 in the set.

--Jacob

On Fri, Jul 30, 2021 at 03:11:49PM +0000, Jacob Champion wrote:
> No worries, it's easy enough to unroll the expansion manually. The
> annoyances without secondary expansion are the duplicated lines for
> each individual CA and the need to introduce .INTERMEDIATE targets so
> that cleanup works as intended.
>
> Attached is a v3 that does that, and introduces a fallback in case
> openssl isn't on the PATH. I also missed a Makefile dependency on
> cas.config the first time through, which has been fixed. The patch you
> pulled out earlier is 0001 in the set.

Patch 0001 is a good cleanup.  Daniel, are you planning to apply that?

Regarding 0002, I am not sure.  Even if this reduces a lot of
duplication, which is really nice, enforcing .SECONDARY to not trigger
with a change impacting Makefile.global.in does not sound very
appealing to me in the long-run, TBH.
--
Michael

> On 10 Aug 2021, at 09:22, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jul 30, 2021 at 03:11:49PM +0000, Jacob Champion wrote:
>> No worries, it's easy enough to unroll the expansion manually. The
>> annoyances without secondary expansion are the duplicated lines for
>> each individual CA and the need to introduce .INTERMEDIATE targets so
>> that cleanup works as intended.
>>
>> Attached is a v3 that does that, and introduces a fallback in case
>> openssl isn't on the PATH. I also missed a Makefile dependency on
>> cas.config the first time through, which has been fixed. The patch you
>> pulled out earlier is 0001 in the set.
>
> Patch 0001 is a good cleanup.  Daniel, are you planning to apply that?

Yes, it’s on my todo for today.

> Regarding 0002, I am not sure.  Even if this reduces a lot of
> duplication, which is really nice, enforcing .SECONDARY to not trigger
> with a change impacting Makefile.global.in does not sound very
> appealing to me in the long-run, TBH.

I personally think the increased readability and usability from what we have
today warrants the changes.  Is it the use of .SECONDARY or the change in the
global Makefile you object to (or both)?

--
Daniel Gustafsson        https://vmware.com/

Michael Paquier <michael@paquier.xyz> writes:
> Regarding 0002, I am not sure.  Even if this reduces a lot of
> duplication, which is really nice, enforcing .SECONDARY to not trigger
> with a change impacting Makefile.global.in does not sound very
> appealing to me in the long-run, TBH.

Yeah, I don't like that change either.  It would be one thing if
we had several places in which suppressing .SECONDARY was useful,
but if there's only one then it seems better to design around it.

As a concrete example of why this might be a bad idea, how sure
are you that noplace in Makefile.global depends on that behavior?

            regards, tom lane

On Tue, Aug 10, 2021 at 09:36:14AM +0200, Daniel Gustafsson wrote:
> I personally think the increased readability and usability from what we have
> today warrants the changes.  Is it the use of .SECONDARY or the change in the
> global Makefile you object to (or both)?

The part I am mainly objecting to is the change in Makefile.global.in,
but I have to admit after thinking about it that enforcing SECONDARY
may not be a good idea if other parts of the system rely on that, so
encouraging the use of clean_intermediates may be dangerous (Tom's
point from upthread).

I have not tried so I am not sure, but perhaps we should just focus on
reducing the number of openssl commands rather than making easier the
integration of new files?  It could be possible to close the gap with
the addition of new files with some more documentation for future
hackers then?
--
Michael

On Tue, 2021-08-10 at 16:22 +0900, Michael Paquier wrote:
> Regarding 0002, I am not sure.  Even if this reduces a lot of
> duplication, which is really nice, enforcing .SECONDARY to not trigger
> with a change impacting Makefile.global.in does not sound very
> appealing to me in the long-run, TBH.

De-duplication isn't the primary goal of the .SECONDARY change. It
definitely helps with that, but the major improvement is that Make can
maintain the CA state with less hair-pulling:

1. Developer updates an arbitrary number of certificates and runs
   `make sslfiles`.
2. Make sees that the CA state is missing, and creates it once at the
   start of the run.
3. Make runs multiple `openssl ca` commands, depending on the
   certificates being changed, which modify the CA state as they go.
4. After Make is finished, it removes all the CA files, putting your
   local state back the way it was before you ran Make.

Doing it this way has several advantages:

- The magic state files don't persist to influence a future Make run,
  so there's less chance of "I generated with local changes, then
  pulled in the changes you made, and now everything's busted in weird
  ways because my CA state disagrees with what's in the tree".

- Order-only intermediates do The Right Thing in this case -- create
  once when needed, accumulate state during the run, remove at the end
  -- whether you add a single certificate, or regenerate the entire
  tree, or even Ctrl-C halfway through. That's going to be very hard to
  imitate by sprinkling `rm`s like the current Makefile does, though
  I've been the weeds long enough that maybe I'm missing an obvious
  workaround.

- If, after all that, something still goes wrong (your machine crashes
  so Make can't clean up), `git status` will now help you debug
  dependency problems because it's no longer "normal" to carry the
  intermediate litter in your source tree.

What it doesn't fix is the fact that we're still checking in generated
files that have interdependencies, so the timestamps Make is looking at
are still going to be wrong during initial checkout. That problem
existed before and will persist after this change.

On Wed, 2021-08-11 at 09:47 +0900, Michael Paquier wrote:
> The part I am mainly objecting to is the change in Makefile.global.in,
> but I have to admit after thinking about it that enforcing SECONDARY
> may not be a good idea if other parts of the system rely on that, so
> encouraging the use of clean_intermediates may be dangerous (Tom's
> point from upthread).

I don't really want to encourage the use of clean_intermediates. I just
want Make to have its default, useful behavior for this one Makefile.

> I have not tried so I am not sure, but perhaps we should just focus on
> reducing the number of openssl commands rather than making easier the
> integration of new files?  It could be possible to close the gap with
> the addition of new files with some more documentation for future
> hackers then?

I'd rather fix the dependency/state bugs than document how to work
around them. I know the workarounds; it doesn't make working with this
Makefile any less maddening.

--Jacob

On Tue, 2021-08-10 at 11:26 -0400, Tom Lane wrote:
> Yeah, I don't like that change either.  It would be one thing if
> we had several places in which suppressing .SECONDARY was useful,
> but if there's only one then it seems better to design around it.

Maybe. The current Makefile already tried to design around it, with
`rm`s inserted various places. That strategy won't work for the CA
state, and my personal interest in trying to manually replicate built-
in Make features is... low.

> As a concrete example of why this might be a bad idea, how sure
> are you that noplace in Makefile.global depends on that behavior?

I was hoping that, by scoping the change to only a single Makefile with
the clean_intermediates flag, I could simplify that question to "does
any place in that one Makefile rely on an affected rule from
Makefile.global?" And the answer to that appears to be "no" at the
moment, because that Makefile doesn't really need the globals for
anything but the prove_ macros.

(Things would get hairier if someone included the SSL Makefile
somewhere else, but I don't see anyone doing that now and I don't know
why someone would.)

But -- if I do spend the time to answer your broader question, does it
actually help my case? Someone could always add more stuff to
Makefile.global. It sounds like the actual fear is that we don't
understand what might be interacting with a very broad global target,
and that fear is too great to try a scoped change, in a niche Makefile,
early in a release cycle, to improve a development issue multiple
committers have now complained about.

If _that's_ the case, then our build system is holding all of us
hostage. Which is frustrating to me. Should I shift focus to help with
that, first?

--Jacob

On Fri, Aug 13, 2021 at 12:08:16AM +0000, Jacob Champion wrote:
> (Things would get hairier if someone included the SSL Makefile
> somewhere else, but I don't see anyone doing that now and I don't know
> why someone would.)

That would not happen.  Hopefully.

> But -- if I do spend the time to answer your broader question, does it
> actually help my case? Someone could always add more stuff to
> Makefile.global. It sounds like the actual fear is that we don't
> understand what might be interacting with a very broad global target,
> and that fear is too great to try a scoped change, in a niche Makefile,
> early in a release cycle, to improve a development issue multiple
> committers have now complained about.
>
> If _that's_ the case, then our build system is holding all of us
> hostage. Which is frustrating to me. Should I shift focus to help with
> that, first?

Fresh ideas in this area are welcome, yes.  FWIW, I'll try to spend a
couple of hours on what you had upthread in 0002 for the
simplification of SSL stuff generation and see if I can come up with
something.
--
Michael

On Fri, 2021-08-27 at 15:02 +0900, Michael Paquier wrote:
> On Fri, Aug 13, 2021 at 12:08:16AM +0000, Jacob Champion wrote:
> > (Things would get hairier if someone included the SSL Makefile
> > somewhere else, but I don't see anyone doing that now and I don't know
> > why someone would.)
> 
> That would not happen.  Hopefully.

:)

> FWIW, I'll try to spend a
> couple of hours on what you had upthread in 0002 for the
> simplification of SSL stuff generation and see if I can come up with
> something.

Thanks! The two-patch v3 no longer applies so I've attached a v4 to
make the cfbot happy.

--Jacob

On Fri, 2021-08-27 at 15:02 +0900, Michael Paquier wrote:
> On Fri, Aug 13, 2021 at 12:08:16AM +0000, Jacob Champion wrote:
> > If _that's_ the case, then our build system is holding all of us
> > hostage. Which is frustrating to me. Should I shift focus to help with
> > that, first?
> 
> Fresh ideas in this area are welcome, yes.

Since the sslfiles target is its own little island in the dependency
graph (it doesn't need anything from Makefile.global), would it be
acceptable to just move it to a standalone sslfiles.mk that the
Makefile defers to? E.g.

    sslfiles:
        $(MAKE) -f sslfiles.mk
Then we wouldn't have to touch Makefile.global at all, because
sslfiles.mk wouldn't need to include it. This also reduces .NOTPARALLEL
pollution as a bonus.

--Jacob

On 9/1/21 8:09 PM, Jacob Champion wrote:
> On Fri, 2021-08-27 at 15:02 +0900, Michael Paquier wrote:
>> On Fri, Aug 13, 2021 at 12:08:16AM +0000, Jacob Champion wrote:
>>> If _that's_ the case, then our build system is holding all of us
>>> hostage. Which is frustrating to me. Should I shift focus to help with
>>> that, first?
>> Fresh ideas in this area are welcome, yes.
> Since the sslfiles target is its own little island in the dependency
> graph (it doesn't need anything from Makefile.global), would it be
> acceptable to just move it to a standalone sslfiles.mk that the
> Makefile defers to? E.g.
>
>     sslfiles:
>         $(MAKE) -f sslfiles.mk
> Then we wouldn't have to touch Makefile.global at all, because
> sslfiles.mk wouldn't need to include it. This also reduces .NOTPARALLEL
> pollution as a bonus.
>

I had he same thought yesterday, so I like the idea :-)


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 2021-09-02 at 07:09 -0400, Andrew Dunstan wrote:
> 
> I had he same thought yesterday, so I like the idea :-)

Done that way in v5. It's a lot of moved code, so I've kept it as two
commits for review purposes.

Thanks!
--Jacob

On Thu, Sep 02, 2021 at 04:42:14PM +0000, Jacob Champion wrote:
> Done that way in v5. It's a lot of moved code, so I've kept it as two
> commits for review purposes.

Nice.  This is neat.  The split helps a lot to understand how you've
changed things from the original implementation.  As a whole, this
looks rather committable to me.

One small-ish comment that I have is about all the .config files we
have at the root of src/test/ssl/, bloating the whole.  I think that
it would be a bit cleaner to put all of them in a different
sub-directory, say just config/ or conf/.
--
Michael

On Fri, 2021-09-03 at 09:46 +0900, Michael Paquier wrote:
> Nice.  This is neat.  The split helps a lot to understand how you've
> changed things from the original implementation.  As a whole, this
> looks rather committable to me.

Great!

> One small-ish comment that I have is about all the .config files we
> have at the root of src/test/ssl/, bloating the whole.  I think that
> it would be a bit cleaner to put all of them in a different
> sub-directory, say just config/ or conf/.

That sounds reasonable. I won't be able to get to it before the holiday
weekend, but I can put up a patch sometime next week.

Thanks,
--Jacob

On Fri, 2021-09-03 at 23:21 +0000, Jacob Champion wrote:
> > One small-ish comment that I have is about all the .config files we
> > have at the root of src/test/ssl/, bloating the whole.  I think that
> > it would be a bit cleaner to put all of them in a different
> > sub-directory, say just config/ or conf/.
> 
> That sounds reasonable. I won't be able to get to it before the holiday
> weekend, but I can put up a patch sometime next week.

Done in v6, a three-patch squashable set. I chose conf/ as the
directory.

--Jacob

On Wed, Sep 08, 2021 at 07:32:03PM +0000, Jacob Champion wrote:
> On Fri, 2021-09-03 at 23:21 +0000, Jacob Champion wrote:
> > > One small-ish comment that I have is about all the .config files we
> > > have at the root of src/test/ssl/, bloating the whole.  I think that
> > > it would be a bit cleaner to put all of them in a different
> > > sub-directory, say just config/ or conf/.
> >
> > That sounds reasonable. I won't be able to get to it before the holiday
> > weekend, but I can put up a patch sometime next week.
>
> Done in v6, a three-patch squashable set. I chose conf/ as the
> directory.

Looks sensible to me.  One thing I can see, while poking at it, is
that the README mentions sslfiles to recreate the set of files.  But
it is necessary to do sslfiles-clean once, as sslfiles is a no-op if
the set of files exists.

I have not been able to check that this is compatible across all the
versions of OpenSSL we support on HEAD.  By looking at the code, that
should be fine but it would be good to be sure.

Daniel, you are registered as a reviewer of this thread
(https://commitfest.postgresql.org/34/3029/).  So I guess that you
would prefer to look at that by yourself and perhaps take care of the
commit?
--
Michael

> On 9 Sep 2021, at 03:32, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 08, 2021 at 07:32:03PM +0000, Jacob Champion wrote:
>> On Fri, 2021-09-03 at 23:21 +0000, Jacob Champion wrote:
>>>> One small-ish comment that I have is about all the .config files we
>>>> have at the root of src/test/ssl/, bloating the whole.  I think that
>>>> it would be a bit cleaner to put all of them in a different
>>>> sub-directory, say just config/ or conf/.
>>>
>>> That sounds reasonable. I won't be able to get to it before the holiday
>>> weekend, but I can put up a patch sometime next week.
>>
>> Done in v6, a three-patch squashable set. I chose conf/ as the
>> directory.
>
> Looks sensible to me.

I concur, I like this more readable approach and it makes adding new keys and
certificates easier.

>  One thing I can see, while poking at it, is
> that the README mentions sslfiles to recreate the set of files.  But
> it is necessary to do sslfiles-clean once, as sslfiles is a no-op if
> the set of files exists.

A few things noted (and fixed as per the attached, which is v6 squashed and
rebased on HEAD; commitmessage hasn't been addressed yet) while reviewing:

* Various comment reflowing to fit within 79 characters

* Pass through the clean targets into sslfiles.mk rather than rewrite them to
make clean (even if they are the same in sslfiles.mk).

* Moved the lists of keys/certs to be one object per line to match the style
introduced in 01368e5d9.  The previous Makefile was violating that as well, but
when we're fixing this file for other things we might as well fix that too.

* Bumped the password protected key output to AES256 to match the server files,
since it's more realistic to see than AES128 (and I was fiddling around here
anyways testing different things, see below).

> I have not been able to check that this is compatible across all the
> versions of OpenSSL we support on HEAD.  By looking at the code, that
> should be fine but it would be good to be sure.

The submitted patch works for 1.0.1, 1.0.2 and 1.1.1 when running the below
sequence:

    make check
    make ssfiles-clean
    make sslfiles
    make check

Testing what's in the tree, recreating the keys/certs and testing against the
new ones.

In OpenSSL 3.0.0, the final make check on the generated files breaks on the
encrypted DER key.  The key generates fine, and running "openssl rsa ..
-check" validates it, but it fails to load into postgres.  Removing the
explicit selection of cipher makes the test pass again (also included in the
attached).  I haven't been able to track down exactly what the issue is, but I
have a suspicion that it's in OpenSSL rather than libpq.  This issue is present
in master too, so fixing it is orthogonal to this work, but it needs to be
figured out.

Another point of interest here is that 3.0.0 put "openssl rsa" in maintenance
mode, so maybe we'll have to look at supporting "openssl pkeyutl" as well for
some parts should future bugs remain unfixed in the rsa command.

> Daniel, you are registered as a reviewer of this thread
> (https://commitfest.postgresql.org/34/3029/).  So I guess that you
> would prefer to look at that by yourself and perhaps take care of the
> commit?

Sure, I can take care of it.

--
Daniel Gustafsson        https://vmware.com/

On Mon, 2021-09-13 at 15:04 +0200, Daniel Gustafsson wrote:
> A few things noted (and fixed as per the attached, which is v6 squashed and
> rebased on HEAD; commitmessage hasn't been addressed yet) while reviewing:
> 
> * Various comment reflowing to fit within 79 characters
> 
> * Pass through the clean targets into sslfiles.mk rather than rewrite them to
> make clean (even if they are the same in sslfiles.mk).
> 
> * Moved the lists of keys/certs to be one object per line to match the style
> introduced in 01368e5d9.  The previous Makefile was violating that as well, but
> when we're fixing this file for other things we might as well fix that too.

Looks good, thanks!

> * Bumped the password protected key output to AES256 to match the server files,
> since it's more realistic to see than AES128 (and I was fiddling around here
> anyways testing different things, see below).

Few thoughts about this part of the diff:

> -# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1) formats
> -# to test libpq's support for the sslpassword= option.
> -ssl/client-encrypted-pem.key: outform := PEM
> -ssl/client-encrypted-der.key: outform := DER
> +# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1)
> +# formats to test libpq's support for the sslpassword= option.
>  ssl/client-encrypted-pem.key ssl/client-encrypted-der.key: ssl/client.key
> -       openssl rsa -in $< -outform $(outform) -aes128 -passout 'pass:dUmmyP^#+' -out $@
> +       openssl rsa -in $< -outform PEM -aes256 -passout 'pass:dUmmyP^#+' -out $@
> +ssl/client-encrypted-der.key: ssl/client.key
> +       openssl rsa -in $< -outform DER -passout 'pass:dUmmyP^#+' -out $@

1. Should the DER key be AES256 as well?
2. The ssl/client-encrypted-der.key target for the first recipe should
be removed; I get a duplication warning from Make.
3. The new client key will need to be included in the patch; the one
there now is still the AES128 version.

And one doc comment:

>  ssl/ subdirectory. The Makefile also contains a rule, "make sslfiles", to
> -recreate them if you need to make changes.
> +recreate them if you need to make changes. "make sslfiles-clean" is required
> +in order to recreate.

This is only true if you need to rebuild the entire tree; if you just
want to recreate a single cert pair, you can just touch the config file
for it (or remove the key, if you want to regenerate the pair) and
`make sslfiles` again.

> The submitted patch works for 1.0.1, 1.0.2 and 1.1.1 when running the below
> sequence:
> 
>         make check
>         make ssfiles-clean
>         make sslfiles
>         make check
> 
> Testing what's in the tree, recreating the keys/certs and testing against the
> new ones.

Great, thanks!

> In OpenSSL 3.0.0, the final make check on the generated files breaks on the
> encrypted DER key.  The key generates fine, and running "openssl rsa ..
> -check" validates it, but it fails to load into postgres.  Removing the
> explicit selection of cipher makes the test pass again (also included in the
> attached).  I haven't been able to track down exactly what the issue is, but I
> have a suspicion that it's in OpenSSL rather than libpq.  This issue is present
> in master too, so fixing it is orthogonal to this work, but it needs to be
> figured out.
> 
> Another point of interest here is that 3.0.0 put "openssl rsa" in maintenance
> mode, so maybe we'll have to look at supporting "openssl pkeyutl" as well for
> some parts should future bugs remain unfixed in the rsa command.

Good to know. Agreed that it should be a separate patch.

--Jacob

> On 15 Sep 2021, at 00:14, Jacob Champion <pchampion@vmware.com> wrote:
> On Mon, 2021-09-13 at 15:04 +0200, Daniel Gustafsson wrote:

>> -# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1) formats
>> -# to test libpq's support for the sslpassword= option.
>> -ssl/client-encrypted-pem.key: outform := PEM
>> -ssl/client-encrypted-der.key: outform := DER
>> +# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1)
>> +# formats to test libpq's support for the sslpassword= option.
>> ssl/client-encrypted-pem.key ssl/client-encrypted-der.key: ssl/client.key
>> -       openssl rsa -in $< -outform $(outform) -aes128 -passout 'pass:dUmmyP^#+' -out $@
>> +       openssl rsa -in $< -outform PEM -aes256 -passout 'pass:dUmmyP^#+' -out $@
>> +ssl/client-encrypted-der.key: ssl/client.key
>> +       openssl rsa -in $< -outform DER -passout 'pass:dUmmyP^#+' -out $@
>
> 1. Should the DER key be AES256 as well?

It should, but then it fails to load by postgres, my email wasn't clear about
this, sorry.  The diff to revert from aes256 (and aes128 for that matter) is to
make the key load at all.

> 2. The ssl/client-encrypted-der.key target for the first recipe should
> be removed; I get a duplication warning from Make.

Interesting, I didn't see that, will check.

> 3. The new client key will need to be included in the patch; the one
> there now is still the AES128 version.

Good point, that's a reason to keep it aes128 until the encrypter DER key in
3.0.0 issue has been fixed.

> And one doc comment:
>
>> ssl/ subdirectory. The Makefile also contains a rule, "make sslfiles", to
>> -recreate them if you need to make changes.
>> +recreate them if you need to make changes. "make sslfiles-clean" is required
>> +in order to recreate.
>
> This is only true if you need to rebuild the entire tree; if you just
> want to recreate a single cert pair, you can just touch the config file
> for it (or remove the key, if you want to regenerate the pair) and
> `make sslfiles` again.

Correct.  In my head, "rebuild" is when dealing with individually changed files
and "recreate" means rebuild everything regardless.  Thats just my in my head
though, so clearly the wording should be expanded.  Will do.

--
Daniel Gustafsson        https://vmware.com/

On Wed, Sep 15, 2021 at 12:31:31AM +0200, Daniel Gustafsson wrote:
> Correct.  In my head, "rebuild" is when dealing with individually changed files
> and "recreate" means rebuild everything regardless.  Thats just my in my head
> though, so clearly the wording should be expanded.  Will do.

So this will be addressed and perhaps merged into the tree, right?
--
Michael

> On 1 Oct 2021, at 08:59, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Sep 15, 2021 at 12:31:31AM +0200, Daniel Gustafsson wrote:
>> Correct.  In my head, "rebuild" is when dealing with individually changed files
>> and "recreate" means rebuild everything regardless.  Thats just my in my head
>> though, so clearly the wording should be expanded.  Will do.
>
> So this will be addressed and perhaps merged into the tree, right?

Yes, I'm finalizing testing of this patch across platforms and OpenSSL versions.

--
Daniel Gustafsson        https://vmware.com/

> On 1 Oct 2021, at 09:02, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 1 Oct 2021, at 08:59, Michael Paquier <michael@paquier.xyz> wrote:
>>
>> On Wed, Sep 15, 2021 at 12:31:31AM +0200, Daniel Gustafsson wrote:
>>> Correct.  In my head, "rebuild" is when dealing with individually changed files
>>> and "recreate" means rebuild everything regardless.  Thats just my in my head
>>> though, so clearly the wording should be expanded.  Will do.
>>
>> So this will be addressed and perhaps merged into the tree, right?
>
> Yes, I'm finalizing testing of this patch across platforms and OpenSSL versions.

The attached contains the requested fixes, and has been tested on all (by us)
supported versions of OpenSSL.  Unless there are objections I will apply this
to master shortly.

--
Daniel Gustafsson        https://vmware.com/

> On 15 Oct 2021, at 14:01, Daniel Gustafsson <daniel@yesql.se> wrote:

> The attached contains the requested fixes, and has been tested on all (by us)
> supported versions of OpenSSL.  Unless there are objections I will apply this
> to master shortly.

Which is now done.

--
Daniel Gustafsson        https://vmware.com/

On Tue, 2021-10-19 at 20:21 +0200, Daniel Gustafsson wrote:
> > On 15 Oct 2021, at 14:01, Daniel Gustafsson <daniel@yesql.se> wrote:
> > The attached contains the requested fixes, and has been tested on all (by us)
> > supported versions of OpenSSL.  Unless there are objections I will apply this
> > to master shortly.
> 
> Which is now done.

Thanks very much! Hopefully this makes that area of the code easier on
everyone.

--Jacob