Enable native SSL support in ext/phar

[petk]

SSL support in ext/phar is enabled either as native (using the system's OpenSSL and its Crypto library linked directly) or as a wrapper provided by ext/openssl.

Native OpenSSL support previously couldn't be enabled when building with shared openssl extension:

./configure --with-openssl=shared --enable-phar=shared

or:

./configure --with-openssl=shared --enable-phar

Some PHP packages build both of these extensions as shared and it makes sense to provide native OpenSSL support in ext/phar also when ext/openssl is build as shared.

Shared phar extension with native OpenSSL enabled now gets libcrypto linked directly:

ldd modules/phar.so
    linux-vdso.so.1
    libcrypto.so.3 => /lib/x86_64-linux-gnu/libcrypto.so.3
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6
    /lib64/ld-linux-x86-64.so.2

The new --with-phar-ssl Autotools configure option enables the SSL support in phar when building without openssl extension or in edge cases when building with phpize. Windows already includes similar option (--enable-phar-native-ssl):

./configure --with-phar --with-phar-ssl --without-openssl

Changed tests:

• ext/phar/tests/**/phar_setsignaturealgo2.phpt - needs ext/openssl enabled due to openssl_get_privatekey().
• ext/phar/tests/phar_setsignaturealgo.phpt - test for ext/phar with native OpenSSL support and ext/openssl disabled.

[cmb69]

Windows already includes similar option (--enable-phar-native-ssl):

Note that that implementation hasn't been updated for OpenSSL 1.1; it still looks for libeay32, in particular for libeay32st which is supposed to be a static build, but it seems to me that at least the PHP distribution of the Windows OpenSSL builds doesn't have that file for a long time. I've checked openssl-1.0.1c-vc11-x64.zip (from 2013), and it contains only the shared library (of course, users could get that library build from elsewhere). Anyhow, if we keep this configuration option on Windows, the code would need to be updated at the very least. See SETUP_OPENSSL() in confutils.js (although it seems to me that dropping support for OpenSSL 1.0 there has been missed in #13498).

[petk]

@cmb69 thanks for this info. Good to remove the libeay32, then yes. I'll see if I can squeeze this in somewhere until the feature freeze. I think it would be possible and not so much problematic to adjust it. I'll check what's up with this. I think this would be kind of good to add it in PHP-8.4 overall.

diff --git a/ext/phar/config.w32 b/ext/phar/config.w32
index 1643216516..24bf926831 100644
--- a/ext/phar/config.w32
+++ b/ext/phar/config.w32
@@ -13,16 +13,12 @@ if (PHP_PHAR != "no") {
                ADD_FLAG("CFLAGS_PHAR", "/D COMPILE_DL_PHAR ");
        }
        if (PHP_PHAR_NATIVE_SSL != "no") {
-               if (CHECK_LIB("libeay32st.lib", "phar")) {
-                       /* We don't really need GDI for this, but there's no
-                       way to avoid linking it in the static openssl build */
-                       ADD_FLAG("LIBS_PHAR", "libeay32st.lib gdi32.lib");
-                       if (PHP_DEBUG == "no") {
-                               /* Silence irrelevant-to-us warning in release builds */
-                               ADD_FLAG("LDFLAGS_PHAR", "/IGNORE:4089 ");
-                       }
+
+               var ret = SETUP_OPENSSL("phar", PHP_PHAR);
+
+               if (ret >= 2) {
+                       MESSAGE("Native OpenSSL support in Phar enabled");
                        AC_DEFINE('PHAR_HAVE_OPENSSL', 1);
-                       STDOUT.WriteLine('        Native OpenSSL support in Phar enabled');
                } else {
                        WARNING('Could not enable native OpenSSL support in Phar');
                }

[cmb69]

@petk, I think your patch should work (I haven't tested it, though), but that would not statically link OpenSSL, at least not regarding the old naming convention. Not sure if that is relevant nowadays; that code is unmodified since c6aa379.

[petk]

Merging this soon then, after rechecking everything. I think this will work ok.

[cmb69]

Thank you! This looks good so far, but I'm not sure about the Windows CI test failures; I'm going to retrigger the CI workflow.

[petk]

Thank you! This looks good so far, but I'm not sure about the Windows CI test failures; I'm going to retrigger the CI workflow.

I'm double checking this also on Windows soon. Yes, this Windows change worried me a bit also because probably it was never used as such yet. On *nix systems Phar native SSL support was already used a lot so it works ok.

[petk]

Yes, this doesn't seem to work ok on Windows as I thought it would work on *nix builds:

configure --enable-phar=shared --enable-phar-ssl=shared

The php_phar.dll can't be loaded. I'll see what to do with this. I'm leaning towards dropping the Windows change here.

[cmb69]

Yes, this doesn't seem to work ok on Windows as I thought it would work on *nix builds:

I just did

buildconf && configure --enable-phar=shared --enable-phar-native-ssl --with-php-build=..\deps-master && nmake
nmake /nologo run ARGS=-m | grep Phar

output:

Phar

Note that nmake run sets up the proper PATH so the required OpenSSL DLL (libcrypto-3-x64.dll) can be found. You can use deplister.exe (contained in any PHP binary download; or build yourself using nmake snap) to check the DLL requirements of any executable. In this case deplister php_phar.dll may show that libcrypto-3-x64.dll cannot be found (a PATH issue).

Previously I tested with a static ext/phar build, and that worked as supposed (the new test would be run, while without --enable-phar-native-ssl it would be skipped).

Now building with --enable-phar-native-ssl=shared, although I do not understand what that's supposed to mean. You can have a php_phar.dll (--enable-phar=shared) with or without OpenSSL support built in.

php-src/ext/phar/config.w32

Line 12 in f5ae5ac

if (PHP_PHAR_SHARED || (PHP_PHAR_NATIVE_SSL_SHARED && PHP_SNAPSHOT_BUILD == "no")) {

does not make sense to me. I think that can be simplified to if (PHP_PHAR_SHARED) { without loosing any functionality/flexibility.

[cmb69]

I think that can be simplified to if (PHP_PHAR_SHARED) { without loosing any functionality/flexibility.

Nope, might be better to leave that alone.

There is an issue with snapshot builds (which we are using to build the official Windows binaries), since ext/phar is built statically. As the PR is now, the core would have a dependency on OpenSLL, what we do not want.

Maybe the following is sufficient:

Suggested change

	if (PHP_PHAR_NATIVE_SSL != "no") {
	if (PHP_PHAR_NATIVE_SSL != "no" && PHP_SNAPSHOT_BUILD == "no") {

This likely also solves the test failures. However, this may still cause issues, if ext/phar is built statically with native SSL support, and somebody loads php_openssl.dll (maybe built via phpize). And that might be a problem on non Windows systems as well.

[petk]

I see. Yes, the OpenSSL crypto library wasn't found. Yes, that nmake run part is what I was missing. Ok, that's good news then (at least that it's loading extensions and all).

Yes, this --enable-phar-native-ssl option was meant to be also in a way that it enables the phar extension itself if --enable-phar wasn't added. But that might be a bit confusing.

if ext/phar is built statically with native SSL support, and somebody loads php_openssl.dll (maybe built via phpize). And that might be a problem on non Windows systems as well.

Hm, this does sound scary. On *nix builds it's working fine according to some quick tests I've done. But it can be totally possible that I've missed something. However up to PHP-8.3 that might already be an issue with other similar extensions (ext/ftp, ext/mysqlnd, and ext/imap which could do the same: they being static and having OpenSSL library linked in PHP, then openssl being shared and loaded dynamically with extension INI directive.

I'm adjusting these Windows options in the next commits...

[cmb69]

Since I've recently worked on a new configure option for Windows (--enable-debug-assertions) which must not be enabled for Windows snapshot builds, I was reminded of the snapshot exclusion list:

php-src/win32/build/confutils.js

Lines 456 to 460 in a092bcb

	var snapshot_build_exclusions = new Array(
	'debug', 'lzf-better-compression', 'php-build', 'snapshot-template', 'zts',
	'ipv6', 'fd-setsize', 'pgi', 'pgo', 'all-shared', 'config-profile', 'sanitizer',
	'phpdbg-debug'
	);

It might be better to add 'native-ssl' there, and remove the PHP_SNAPSHOT_BUILD checks from phar/config.w32. Apparently, that is the only extension specifc config.w32 which even uses that variable.

Generally, it would be good to have this change checked by a code owner, but since there is none for ext/phar, maybe @Girgias or @nielsdos who both recently worked on the extension will have a look. Or maybe @bukka since he's the code owner of ext/openssl.

[bukka]

So I had a look to the phar openssl usage in util.c. It's quite outdated but it should still work for now. It might not be ideal as we want to move away from default provider for ZTS (see #14734 ) so it would be better to always prefer ext openssl implementation in the future. We should however provide a public API rather than call PHP functions which is not really nice. We have got an API already for encryption and some other parts so we could add something for signing.

I don't exactly understand the description though. Do you mean that if both are shared, then only native support cannot be enabled but it can still use the OpenSSL extension one? Or it cannot use singing at all? If former, I'm not exactly sure what the problem is? As I mentioned it's better to always prefer openssl ext implementation probably even now.

[petk]

The crucial info here is that ext/openssl is "better" than some duplicate linking of OpenSSL into the extension and using the OpenSSL directly. This info I was missing :D If only the ext/ftp and ext/mysqlnd would also work like this...

It makes also usage much much easier to work with and understand. Then this native SSL in phar is on the list to be removed? I don't see any reason to have such functionality actually.

I don't exactly understand the description though. Do you mean that if both are shared, then only native support cannot be enabled but it can still use the OpenSSL extension one?

When both are shared everything works ok. If ext/openssl is enabled, SSL in phar is enabled. If it is disabled, phar works without SSL.

[cmb69]

If ext/openssl is enabled, SSL in phar is enabled. If it is disabled, phar works without SSL.

So for proper signing (the md5/sha hashes are almost useless), ext/openssl need to be enabled. If that is not a problem, no need for --enable-native-ssl then.

[cmb69]

We should however provide a public API rather than call PHP functions which is not really nice. We have got an API already for encryption and some other parts so we could add something for signing.

Indeed. That's terrible as it is.

[petk]

Closing in favor of the removal #15574
Ideally it should go to PHP-8.4 but as the change somehow is being done late, it can also go to the next version then.