| Message ID | 20230110170257.113527-1-frank@lichtenheld.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] check_engine_keys: make pass with OpenSSL 3 | expand |
Hi, On Tue, Jan 10, 2023 at 06:02:57PM +0100, Frank Lichtenheld wrote: > @@ -27,7 +27,7 @@ ${top_builddir}/src/openvpn/openvpn --cd ${top_srcdir}/sample --config sample-co > # first off check we died because of a key mismatch. If this doesn't > # pass, suspect openssl of returning different messages and update the > # test accordingly > -loggrep '(X509_check_private_key:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } > +loggrep '(x509 certificate routines:(X509_check_private_key)?:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } This change does not convince me - assuming normal regex here, the original grep would have found any lines with X509_check_private_key:key values mismatch in them, while the new one would find X509_check_private_key::key values mismatch X509_check_private_key:X509_check_private_key:key values mismatch but not "the old one with just one :" Should this be (x509 certificate routines:(X509_check_private_key:)?key va... with the second ":" part of the (...:)? zero-or-once match? (... as a side note, I hate this test... it's easily the test with the most commits to make it behave across platforms and SSL library versions) gert
On Wed, Jan 11, 2023 at 08:39:51AM +0100, Gert Doering wrote: > Hi, > > On Tue, Jan 10, 2023 at 06:02:57PM +0100, Frank Lichtenheld wrote: > > @@ -27,7 +27,7 @@ ${top_builddir}/src/openvpn/openvpn --cd ${top_srcdir}/sample --config sample-co > > # first off check we died because of a key mismatch. If this doesn't > > # pass, suspect openssl of returning different messages and update the > > # test accordingly > > -loggrep '(X509_check_private_key:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } > > +loggrep '(x509 certificate routines:(X509_check_private_key)?:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } > > This change does not convince me - assuming normal regex here, the > original grep would have found any lines with > > X509_check_private_key:key values mismatch > > in them, while the new one would find > > X509_check_private_key::key values mismatch > X509_check_private_key:X509_check_private_key:key values mismatch > > but not "the old one with just one :" > > Should this be > > (x509 certificate routines:(X509_check_private_key:)?key va... > > with the second ":" part of the (...:)? zero-or-once match? So, OpenSSL 3 prints: x509 certificate routines::key values mismatch yes, two colons! OpenSSL 1.1 prints: x509 certificate routines:X509_check_private_key:key values mismatch My patch matches for both. But you are right that I made it slightly stricter, since it previously we only matched on X509_check_private_key:key values mismatch for OpenSSL 1.1. If you would prefer that we could go for (x509 certificate routines:|X509_check_private_key):key values mismatch But I like my version better. If that passes buildbot, I think it should be "good enough". > (... as a side note, I hate this test... it's easily the test with the > most commits to make it behave across platforms and SSL library versions) yeah, definitely. Regards,
Acked-by: Gert Doering <gert@greenie.muc.de> Looking more closely I can see that I misread the regex, and it's all fine indeed. Passing the test on my test candidates (ossl 1.1.x, ossl 3.0.x but no engine support) and also on the GHA actions with both ossl versions. Your patch has been applied to the master branch. commit af25448ee19da5d225a6a1f30f26dc3949ed8921 (master) commit d09db604dac1e8140c0e4cad73a8b3d2e21cbebc (release/2.6) Author: Frank Lichtenheld Date: Tue Jan 10 18:02:57 2023 +0100 check_engine_keys: make pass with OpenSSL 3 Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20230110170257.113527-1-frank@lichtenheld.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25949.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/.gitignore b/.gitignore index 7335154f..813413fe 100644 --- a/.gitignore +++ b/.gitignore @@ -68,6 +68,10 @@ tests/t_client-*-20??????-??????/ t_client.rc t_client_ips.rc tests/unit_tests/**/*_testdriver +tests/unit_tests/engine-key/client.key +tests/unit_tests/engine-key/log.txt +tests/unit_tests/engine-key/openssl.cnf +tests/unit_tests/engine-key/passwd src/openvpn/openvpn include/openvpn-plugin.h diff --git a/tests/unit_tests/engine-key/check_engine_keys.sh b/tests/unit_tests/engine-key/check_engine_keys.sh index 7e9a0e80..12dd2301 100755 --- a/tests/unit_tests/engine-key/check_engine_keys.sh +++ b/tests/unit_tests/engine-key/check_engine_keys.sh @@ -27,7 +27,7 @@ ${top_builddir}/src/openvpn/openvpn --cd ${top_srcdir}/sample --config sample-co # first off check we died because of a key mismatch. If this doesn't # pass, suspect openssl of returning different messages and update the # test accordingly -loggrep '(X509_check_private_key:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } +loggrep '(x509 certificate routines:(X509_check_private_key)?:key values mismatch|func\(128\):reason\(116\))' log.txt || { echo "Key mismatch not detected"; exit 1; } # now look for the engine prints (these are under our control) loggrep 'ENGINE: engine_init called' || { echo "Engine initialization not detected"; exit 1; }
Not enabled by default with OpenSSL 3, so we don't see this in our builds. While here add missing entries to .gitignore (which is what made me look at engine-key test in the first place). Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com> --- .gitignore | 4 ++++ tests/unit_tests/engine-key/check_engine_keys.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)