Horton

Thread

Commits

  1. Refactor the sslfiles Makefile target for ease of use

  2. Remove unused regression test certificate server-ss

  1. [PATCH] test/ssl: rework the sslfiles Makefile target

    Jacob Champion <pchampion@vmware.com> — 2021-03-04T00:03:36Z

    Hello all,

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.

For the common case -- adding a new certificate/key pair -- all you
have to do now is drop one new .config file into the test/ssl
directory, add it to either the CLIENTS or SERVERS list, and run `make
sslfiles`. No cleaning necessary.

The core architectural addition: by making use of both order-only
dependencies and intermediate file cleanup, the CA state will be
recreated (exactly once) on demand for each Make run, assign serial
numbers to new certificates in increasing order, and then be
automatically removed at the end of the Make run. So it should be much
harder to accumulate junk state during incremental development.

== Improvements ==

- The sslfiles target no longer needs to be preceded by sslfiles-clean
to work correctly.

- I've removed some incorrect dependencies, added missing ones, and
moved others to order-only (such as the CA state files -- we need them
to exist, but the changes they accumulate should not force other
certificates to be regenerated).

- Most of the copy-paste recipes have been consolidated, and some
existing copy-paste cruft has disappeared as a result. The unused
server-ss certificate has been removed entirely.

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

- All the .config files are now self-contained (i.e. they contain all
the required extension information), which simplifies the OpenSSL
recipes significantly. No more -extfile wrangling.

== Downsides ==

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

== Possible Future Work ==

- I haven't quite fixed the dependency situation for the CRL hash
directories -- there are situations where they could be incorrectly
remade. (Relying on directories' timestamps is perilous.) But I think I
have not made the situation worse than it is today.

- Because all of these generated files are still checked in, if you run
`make sslfiles` after checking out the ssl artifacts directory for the
first time, Make may decide to regenerate some files due to the more
recent timestamps. I don't see an easy way around this. You can reset
Make's view of things with a `touch ssl/*`, but it'd be nice if it
didn't happen to begin with.

I recommend using a diff driver for the new certificates and CRLs so
that you can see the actual changes -- the only things that should have
changed are the serial numbers, the timestamps, and the signature
blobs.

WDYT? I missed the boat slightly for the current commitfest, so I'll
add this patch to the next one.

--Jacob

[1] https://www.postgresql.org/message-id/fd96ae76-a8e3-ef8e-a642-a592f5b76771%40dunslane.net

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

    Jacob Champion <pchampion@vmware.com> — 2021-06-29T20:14:12Z

    On Thu, 2021-03-04 at 00:03 +0000, Jacob Champion wrote:
> Hello all,
> 
> 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.

v2 is a rebase to resolve conflicts around SSL compression and the new
client-dn test case.

--Jacob

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-27T22:24:36Z

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

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

    Jacob Champion <pchampion@vmware.com> — 2021-07-28T20:10:53Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2021-07-28T20:45:07Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-28T21:02:54Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-07-28T21:09:57Z

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

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

    Jacob Champion <pchampion@vmware.com> — 2021-07-30T15:11:49Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-08-10T07:22:55Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-08-10T07:36:14Z

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-10T15:26:30Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-08-11T00:47:38Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-08-13T00:08:05Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-08-13T00:08:16Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-08-27T06:02:27Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-01T16:12:44Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-02T00:09:49Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2021-09-02T11:09:08Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-02T16:42:14Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-09-03T00:46:29Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-03T23:21:59Z

    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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-08T19:32:03Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2021-09-09T01:32:00Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-13T13:04:28Z

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

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

    Jacob Champion <pchampion@vmware.com> — 2021-09-14T22:14:16Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-09-14T22:31:31Z

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

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

    Michael Paquier <michael@paquier.xyz> — 2021-10-01T06:59:53Z

    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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-10-01T07:02:46Z

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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-10-15T12:01:30Z

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

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

    Daniel Gustafsson <daniel@yesql.se> — 2021-10-19T18:21:31Z

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

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

    Jacob Champion <pchampion@vmware.com> — 2021-10-19T18:25:01Z

    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