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

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

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