Add regression testing for https_port certificate loading

[yadij]

Since our unit testing cannot yet handle FATAL exits of Squid as a
successful test result, some https_port settings are commented out for
now. They should be checked manually if altering the relevant code.

Likewise, ssl-bump testing cannot be done yet because the OpenSSL
specific code is not always built (minimal build test layer).

This test creates some temporary CA certificates and keys as-needed. The
script checks for certtool or openssl CLI existence.

[yadij]

@chtsanti , in PRF#144 michaeladm mentioned the certtool created certificates did not work for bumping. You might want to test the bumping process with the certs created by the script here

[rousskov]

In PR #144 michaeladm mentioned the certtool created certificates did not work for bumping. You might want to test the bumping process with the certs created by the script here

I am not sure it is important that the test certificates work outside of their test cases, but requiring certtool availability for running these tests is one concern that did not make it into my review (IIRC). How about generating certificates during bootstrapping, so that Squid tarball users can run full tests without relying on any particular certificate manipulation program being installed?

[yadij]

I'm being a bit paranoid here with the timing and cert creation. Having them only be built when actually needed avoids any issues that might crop up with our tarballs containing crypto keys (might confuse some people or tools that check for such things).

[rousskov]

Great progress, thank you. I left a few questions and a couple of should-be-easy-to-address suggestions.

[rousskov]

Most of these change requests require changes in more places than just code they are attached to, going beyond that code-specific suggestions. Please interpret them accordingly.

P.S. I am ignoring CI test failures in this review iteration.

[rousskov]

Feels wrong to use a domain that can be reached on the internet and has a "real" trusted certificate. This code is not "documentation" that example.org is meant for. Can we use a .test TLD instead? I think such usage would follow RFC 2606 recommendations and be compatible with RFC 6761 requirements on other agents.

With the transition to .test TLD, we can make this name more specific to this certificate meaning. For example:

Suggested change

	organizationName= Example.Org
	organizationName = Root.Test

Similar changes would need to be done elsewhere, of course. They would make associated names easier to grok in triage (than, for example, trying to remember the role difference between example.org and example.net and example.com!).

[rousskov]

Please use a more telling/specific name for this and similar files. Do not force us to remember the difference between "example.org", "example.com", and "example.net" when looking through artifacts or triage logs! And please use a filename extension to hint an how this configuration file is used. For example:

Suggested change

	" > example.org-ca
	" > root-ca.conf

or

Suggested change

	" > example.org-ca
	" > ca-root.conf

Other names would need to be adjusted accordingly, of course.

[rousskov]

For readability and consistency sake, please use the same assignment style throughout configuration files, with spaces around =. The change below is just an illustration -- more lines will need to be adjusted:

Suggested change

	prompt= no
	prompt = no

[rousskov]

This is a (fatal) error for this script, not just a warning:

Suggested change

	echo "WARNING: cannot find a tool to generate certificates"
	echo "ERROR: Cannot find a tool to generate certificates"

[rousskov]

The other files are following a reasonable naming scheme, but the last two use a different scheme that names the same kind of file differently and starts using a vague word "chain". Let's apply the earlier naming scheme/pattern to the last two entries as well:

Suggested change

	cat leaf-rsa.pkey leaf-rsa.crt > leaf-key-rsa.pem
	cat leaf-rsa.crt ca-root-rsa.crt > leaf-chain-nokey-rsa.pem
	cat leaf-rsa.pkey leaf-rsa.crt > leaf-rsa.pem
	cat leaf-rsa.crt ca-root-rsa.crt > leaf-root-nokey-rsa.pem

[rousskov]

There are other incomplete chains here IMO. And this one does not really chain anything. Let's be specific and more consistent:

Suggested change

	# 4) incomplete chain tls-cert=./ca-mid-rsa.*
	# 4) intermediate CA only tls-cert=./ca-mid-rsa.*

[rousskov]

Please drop this invalid setting/variation (C5): This PR focuses on testing "certificate loading". The corresponding tests are not about testing our ability to parse boolean options like tls-generate-host-certificates. This PR does not even focus on parsing options that do specify certificate filenames.

Suggested change

# 5) FILE tls-generate-host-certificates=./ca-mid-root-rsa.pem

If there is enough value in testing tls-generate-host-certificates=garbage, a different PR should add a different test. This "certificate loading" PR is best without such testing.

[rousskov]

For consistency with "leaf only" in item 1.

Suggested change

	# 3) root CA tls-cert=./ca-root-rsa.*
	# 3) root CA only tls-cert=./ca-root-rsa.*

Alternatively, we can drop "only" from all entries.

[rousskov]

Please move this line down and visually separate it from OpenSSL-specific code. It is not specific to OpenSSL. It covers both GnuTLS and OpenSSL cases (handled above).

Suggested change

	AM_CONDITIONAL(ENABLE_CERT_TESTS,[ test "x$with_openssl" = "xyes" -o "x$with_gnutls" != "xno"])
	AC_SUBST(SSLLIB)
	AC_SUBST(SSLLIB)
	AM_CONDITIONAL(ENABLE_CERT_TESTS,[ test "x$with_openssl" = "xyes" -o "x$with_gnutls" != "xno"])

[rousskov]

Do we really want to ENABLE_CERT_TESTS (below) when neither openssl nor certool binaries are not found (i.e., neither OPENSSL_BIN nor CERTTOOL_BIN variables are defined)?

Should not we ENABLE_CERT_TESTS (below) when Squid is built with GnuTLS but only openssl binary is found (i.e., OPENSSL_BIN is defined but CERTTOOL_BIN is not)? It is currently not enabled in that case AFAICT. Same for OpenSSL+certool combination.

It feels like AC_PATH_PROG() tests for OPENSSL_BIN and CERTTOOL_BIN should be made independent from the corresponding libraries and ENABLE_CERT_TESTS preconditions adjusted to enable testing in more environments where such testing should work.