Test internal cert generation & verification process #10350

yhabteab · 2025-02-18T17:23:49Z

This is basically kind of the same as #10329 but without using hard coded certificates!

yhabteab · 2025-09-02T13:12:17Z

That was my last push :)! Now, all of the GHAs should be fine, so pls have a look!

jschmidt-icinga

You should squash some of the changes from f74cddb back into ec26a2d. First introducing the new argument as int and then changing it to long makes the diff much harder to read.

lib/base/tlsutility.cpp

test/base-tlsutility.cpp

lib/base/tlsutility.cpp

yhabteab · 2025-09-03T16:40:01Z

As @julianbrost found out, OpenSSL has no problems with past 2038 timestamps, so the whole Normalize* function was removed, as it's no longer needed. To not overflow the time_t type when comparing the certifcate timestamps, OpenSSL has also a function ASN1_TIME_compare which takes two ASN1_TIME pointers and compare them, instead of the X509_cmp_time function, however as of writing this comment, AL2 already failed due to the ASN1_TIME_compare function being absent with such old OpenSSL version. So, I'm not sure yet, how we should handle this, maybe there's an equivalent OpenSSL function that also exists on AL2.

yhabteab · 2025-09-04T10:46:47Z

The previously used ASN1_TIME_adj OpenSSL function was for some reason that I don't understood and that I don't want to find out why - failing miserably on Windows 32. So, after searching for an equivalent function a bit online, I found X509_time_adj_ex that does similar things and obviously that also works on Windows 32, thus I've replaced all ASN1_TIME_adj usages with that function.

The certificate generated by `PkiUtility::NewCert()` is self-signed, and so the subsequent `PkiUtility::SignCsr()` call is required. However, `PkiUtility::SignCsr()` doesn't reuse existin cert, instead it'll generate a fresh one on its own. So, skip the first one entirely!

yhabteab · 2025-09-19T10:27:04Z

Just rebased to resolve the conflicts!

yhabteab · 2025-09-19T12:13:57Z

Fixed a small comparison failure on docker builds as it seems to take over a second to sign the certificate and load it from disk, thus fails the == comparison. The fix is to use the 0>= operator like in other places!

jschmidt-icinga

Just a few comments, mostly things clang-tidy suggested, one readability improvement. Otherwise this looks fine to me.

test/base-tlsutility.cpp

jschmidt-icinga

😆 assertIMMorDZSSC isn't much better than before, but it's not that important either.

Al2Klimov · 2025-09-23T15:29:25Z

lib/base/tlsutility.cpp

 	X509_set_version(cert, 2);
-	X509_gmtime_adj(X509_get_notBefore(cert), 0);
-	X509_gmtime_adj(X509_get_notAfter(cert), ca ? ROOT_VALID_FOR : LEAF_VALID_FOR);
+	auto notBefore = 0;


0 is an int.

Please stop splitting a single point across multiple comments. This makes it way harder to read, especially if individual conversations are marked as resolved and additionally, it clutters the diff view (see the screenshot below). If you want to refer to code in a different place, you can also just include it by referencing it like this:

icinga2/lib/base/tlsutility.cpp

Line 651 in 190dc86

notBefore = validFor - 365*24*60*60;

Al2Klimov · 2025-09-23T15:30:01Z

lib/base/tlsutility.cpp

+	if (validFor < 0) {
+		// Set the validity start date to one year past the validFor date,
+		// so that the generated cert doesn't expire before the validFor date.
+		notBefore = validFor - 365*24*60*60;


But validFor is a long.

Al2Klimov · 2025-09-23T15:30:33Z

lib/base/tlsutility.cpp

 	X509_set_version(cert, 2);
-	X509_gmtime_adj(X509_get_notBefore(cert), 0);
-	X509_gmtime_adj(X509_get_notAfter(cert), ca ? ROOT_VALID_FOR : LEAF_VALID_FOR);
+	auto notBefore = 0;


So:

Suggested change

auto notBefore = 0;

long notBefore = 0;

Al2Klimov · 2025-09-23T15:31:05Z

lib/base/tlsutility.cpp

+	if (validFor < 0) {
+		// Set the validity start date to one year past the validFor date,
+		// so that the generated cert doesn't expire before the validFor date.
+		notBefore = validFor - 365*24*60*60;


Why 1 year and not LEAF_VALID_FOR?

julianbrost · 2025-09-23T15:52:25Z

test/base-tlsutility.cpp

+	// Set the CA certificate to expire in 100 days, i.e. less than the LEAF_VALID_FOR threshold of 397 days.
+	auto now = time(nullptr);
+	BOOST_CHECK(X509_time_adj_ex(X509_get_notAfter(cacert.get()), 100, 0, &now));
+	BOOST_CHECK(!IsCaUptodate(cacert.get())); // Is CA outdated now?


This (and other places in the new tests) still use the pattern where you update some certificate parameters, but don't actually create a new signature. So after just doing that X509_time_adj_ex(), the X509 structure is in an inconsistent state where the parsed and changed information does not match the signature. IMHO this makes the whole test case questionable if you use inconsistent data structures. Thus, everywhere something needs to be changed in a certificate, a new certificate should be created.

I'm still using X509_time_adj_ex() here because I don't verify any cert signature here but want to only test whether the IsCaUptodate function behaves correctly or not and this has nothing to do with the actual cert signature. And for the other few cases that still use X509_time_adj_ex(), they don't use it to adjust the notAfter timestamp but the notBefore one, since it's not user configurable (and I don't want to add yet another parameter for it), so creating a new certificate would be pointless as it's going to have a notBefore ts of 0.

Your newly added comment highlights the problem, because of that inconsistency within the test data, it's possible the test doesn't even test what it's supposed to test:

icinga2/test/base-tlsutility.cpp

Lines 197 to 203 in c3ee33c

// Set the certificate validity start date to 2016, all certificates created before 2017 are considered outdated.

BOOST_CHECK(X509_time_adj_ex(X509_get_notBefore(cert.get()), 0, -(now-l_2016), &now));

BOOST_CHECK(!IsCertUptodate(cert));

// ... but verification should still work, as the certificate is still valid. Though, on AL2 the notBefore

// time set above will be reset to its original value, as VerifyCertificate transforms the crt first to string

// and back to X509, which causes OpenSSL to reset the notBefore time to its original value.

BOOST_CHECK(VerifyCertificate(cacert, cert, String()));

and I don't want to add yet another parameter for it

That would result in a better test case though. The actual use of these functions is to check a certificate that was either loaded from a file or sent by a peer, not a X509* where some values were adjusted retroactively. So using a regular certificate as an input in the test cases would be more realistic.

Plus, it would additionally remove the need for this special case in the implementation if the caller could just specify the whole validity period:

icinga2/lib/base/tlsutility.cpp

Lines 648 to 652 in c3ee33c

if (validFor < 0) {

// Set the validity start date to ~1 year past the validFor date,

// so that the generated cert doesn't expire before the validFor date.

notBefore = validFor - LEAF_VALID_FOR;

}

julianbrost · 2025-09-23T16:00:11Z

test/base-tlsutility.cpp

+	char errBuf[256]; // Buffer for OpenSSL error messages.
+	ERR_error_string_n(ERR_get_error(), errBuf, 256);


Would be nicer if it didn't repeat that magic constant:

Suggested change

char errBuf[256]; // Buffer for OpenSSL error messages.

ERR_error_string_n(ERR_get_error(), errBuf, 256);

char errBuf[256]; // Buffer for OpenSSL error messages.

ERR_error_string_n(ERR_get_error(), errBuf, sizeof(errBuf));

On that note (and because clang-tidy complained about it), make the char array a std::array and use .data() and .size() instead of decaying into a char * and sizeof(char[]).

…omparison

yhabteab · 2025-09-24T11:23:25Z

The notBefore cert ts is now configurable as well as requested in #10350 (comment).

cla-bot bot added the cla/signed label Feb 18, 2025

yhabteab force-pushed the unittest-certificate-verification branch 6 times, most recently from 79c98f8 to 0500a12 Compare February 24, 2025 15:55

yhabteab force-pushed the unittest-certificate-verification branch 4 times, most recently from 0f579b7 to a8ac9e4 Compare September 2, 2025 08:32

yhabteab marked this pull request as ready for review September 2, 2025 08:33

yhabteab force-pushed the unittest-certificate-verification branch 3 times, most recently from 9355e47 to 53b65c2 Compare September 2, 2025 11:46

yhabteab requested a review from jschmidt-icinga September 2, 2025 11:47

yhabteab added enhancement New feature or request area/tests Unit and environment tests labels Sep 2, 2025

yhabteab added this to the 2.16.0 milestone Sep 2, 2025

yhabteab force-pushed the unittest-certificate-verification branch 3 times, most recently from 6591932 to 9e2fe37 Compare September 2, 2025 13:05

yhabteab requested review from jschmidt-icinga and removed request for jschmidt-icinga September 2, 2025 13:12

jschmidt-icinga reviewed Sep 3, 2025

View reviewed changes

lib/base/tlsutility.cpp Outdated Show resolved Hide resolved

test/base-tlsutility.cpp Show resolved Hide resolved

test/base-tlsutility.cpp Outdated Show resolved Hide resolved

test/base-tlsutility.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the unittest-certificate-verification branch from 9e2fe37 to e1c8fba Compare September 3, 2025 14:35

yhabteab requested a review from jschmidt-icinga September 3, 2025 14:35

julianbrost reviewed Sep 3, 2025

View reviewed changes

lib/base/tlsutility.cpp Outdated Show resolved Hide resolved

lib/base/tlsutility.cpp Outdated Show resolved Hide resolved

lib/base/tlsutility.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the unittest-certificate-verification branch from e1c8fba to b15e692 Compare September 3, 2025 16:30

yhabteab force-pushed the unittest-certificate-verification branch 2 times, most recently from 9e8b7ec to 4c6b0b7 Compare September 4, 2025 10:22

yhabteab requested a review from julianbrost September 4, 2025 11:00

yhabteab force-pushed the unittest-certificate-verification branch from 4c6b0b7 to f7219f7 Compare September 19, 2025 10:23

yhabteab force-pushed the unittest-certificate-verification branch from f7219f7 to 0133fa1 Compare September 19, 2025 12:03

jschmidt-icinga reviewed Sep 23, 2025

View reviewed changes

test/base-tlsutility.cpp Outdated Show resolved Hide resolved

test/base-tlsutility.cpp Outdated Show resolved Hide resolved

test/base-tlsutility.cpp Outdated Show resolved Hide resolved

yhabteab force-pushed the unittest-certificate-verification branch from 0133fa1 to c48941f Compare September 23, 2025 08:31

yhabteab requested a review from jschmidt-icinga September 23, 2025 08:31

jschmidt-icinga previously approved these changes Sep 23, 2025

View reviewed changes

Al2Klimov reviewed Sep 23, 2025

View reviewed changes

julianbrost reviewed Sep 23, 2025

View reviewed changes

yhabteab dismissed jschmidt-icinga’s stale review via c3ee33c September 24, 2025 09:26

yhabteab force-pushed the unittest-certificate-verification branch from c48941f to c3ee33c Compare September 24, 2025 09:26

yhabteab requested review from jschmidt-icinga and julianbrost September 24, 2025 09:27

yhabteab added 2 commits September 24, 2025 13:21

tlsutility: make cert ts configurable & use ASN1_TIME_compare for c…

59a378f

…omparison

Test internal cert generation & verification process

3ee0336

yhabteab force-pushed the unittest-certificate-verification branch from c3ee33c to 3ee0336 Compare September 24, 2025 11:22

	// Set the certificate validity start date to 2016, all certificates created before 2017 are considered outdated.
	BOOST_CHECK(X509_time_adj_ex(X509_get_notBefore(cert.get()), 0, -(now-l_2016), &now));
	BOOST_CHECK(!IsCertUptodate(cert));
	// ... but verification should still work, as the certificate is still valid. Though, on AL2 the notBefore
	// time set above will be reset to its original value, as VerifyCertificate transforms the crt first to string
	// and back to X509, which causes OpenSSL to reset the notBefore time to its original value.
	BOOST_CHECK(VerifyCertificate(cacert, cert, String()));

	if (validFor < 0) {
	// Set the validity start date to ~1 year past the validFor date,
	// so that the generated cert doesn't expire before the validFor date.
	notBefore = validFor - LEAF_VALID_FOR;
	}

		char errBuf[256]; // Buffer for OpenSSL error messages.
		ERR_error_string_n(ERR_get_error(), errBuf, 256);

Test internal cert generation & verification process #10350

Are you sure you want to change the base?

Test internal cert generation & verification process #10350

Uh oh!

Conversation

yhabteab commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhabteab commented Sep 2, 2025

Uh oh!

jschmidt-icinga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yhabteab commented Sep 3, 2025

Uh oh!

yhabteab commented Sep 4, 2025

Uh oh!

yhabteab commented Sep 19, 2025

Uh oh!

yhabteab commented Sep 19, 2025

Uh oh!

jschmidt-icinga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jschmidt-icinga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhabteab commented Sep 24, 2025

Uh oh!

Uh oh!

yhabteab commented Feb 18, 2025 •

edited

Loading