Add support for libargon2

[mpociot]

This PR is my attempt to add libargon2 support.
I have only tested this on macOS so far and there are two issues where I could use some help:

1. The "compile check" fails, because password-argon2 support is not an extension. That's why php --ri password-argon2 fails.
2. When building PHP with the combination of "sodium" and "password-argon-2" (sodium,password-argon-2) the linker throws the following errors:

duplicate symbol '_validate_inputs' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
duplicate symbol '_fill_memory_blocks' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
duplicate symbol '_fill_first_blocks' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
duplicate symbol '_initial_hash' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
duplicate symbol '_finalize' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
duplicate symbol '_initialize' in:
    /[redacted]/buildroot/lib/libsodium.a(libsodium_la-argon2-core.o)
    /[redacted]/buildroot/lib/libargon2.a(core.o)
ld: 6 duplicate symbols for architecture arm64

When building PHP only with password-argon-2 this works fine.

Tasks after merging

• Docs Add password-argon2 support static-php/static-php-cli-docs#6 (temporarily disable compatibility with libsodium for linux)
• Integration test
• Common extensions test
• Bulk extensions test
• New issue (modified libargon2 still conflicts with libsodium on Linux)

[crazywhalecc]

Question 1.

Extension check names can be overwritten, but only the name can be changed. For example, mbregex extension is a function for mbstring, create a function named getDistName: https://github.com/crazywhalecc/static-php-cli/blob/refactor/src/SPC/builder/extension/mbregex.php#L15

If there is more than one extension like this, I think we can add a method to override the check command.

[crazywhalecc]

Question 2.

The libsodium library seems to contain some argon2 code, causing conflicts, but directly using the password-argon2 extension with the libsodium library may also have some linking issues. Perhaps the patch code is still needed in the end.

[crazywhalecc]

Unfortunately, the PHP official website has stated that the argon library of libsodium cannot be used. php.net.

It is caused by php itself (at least it looks like this for now).

Overall, there are two solutions available.

1. Support the password-argon2 algorithm as a special extension, while it is mutually exclusive with the sodium extension.
2. fork the code of libargon2, change the conflicting symbol names, and modify the PHP source code when compiling static php. (libargon2 has not been updated for a long time, so changing it has less impact than changing libsodium)

[mpociot]

Thank you for the in-depth analysis of this.

I'm not really sure how common the combination of sodium and argon2 password hashing is, but I stumbled upon this as I was trying to replicate the "default" PHP setup when installing PHP via brew. This does include sodium and argon2.

I agree - patching libargon2 seems to be the "easier" option in this case.
Would you prefer to fork libargon2 and rename the symbols in the fork, or do you think this could just be done on-the-fly in the build process itself?

[crazywhalecc]

Well, I don't know much about the architecture of the libargon2 project, so it may take some time to read the source code to avoid potential bugs caused by simply changing the name. Some symbols may not be changed directly.

PHP installed via homebrew is a dynamically linked binary, and the code of the two libraries is not included in the PHP binary file, they can dynamically call the linked library as needed. static-php-cli just adds all of required library archives.

For now I have tried this PR on linux, same issue, and even more symbol conflicts. This should require a fork project to achieve, because this piece of patch code is theoretically not part of static-php-cli.

And in view of the fact that this part may need to modify a lot of code, this PR can be used as solution 1 to temporarily solve the problem.

[mpociot]

I have forked libargon2 and patched the source files so that there are no duplicate symbols.
It was not necessary to patch anything in the PHP sourcecode itself.

I don't really know what to use as the extension check name though, as password-argon2 does not get registered as a custom extension. It also does not show up in php -i besides being mentioned in the configure command.

Maybe it would be good to be able to skip this for extensions - we could still add a sanity check to ensure successful compilation.

[crazywhalecc]

I have two ways:

1. change Extension::getDistName(), let it be able to return null value. Null value will not run --ri $name check.

$name = $ext->getDistName();
if ($name !== null) {
    [$ret] = shell()->execWithResult(BUILD_ROOT_PATH . '/bin/php --ri "' . $name . '"', false);
    if ($ret !== 0) {
        throw new RuntimeException('extension ' . $ext->getName() . ' failed compile check');
    }
}

2. Add a method, example: Extension::check(string $binary): bool { return shell()->execWithResult("{$binary} --ri \"{$this->name}\""); } and overwrite in password-argon2 extension class.

// class Extension { }
/**
 * Sanity check for extension
 * Basically check if "--ri" can be used to get extension info
 * You can override this method to do more checks
 *
 * @param string $binary Compiled binary path
 * @return bool
 */
public function check(string $binary): bool
{
    [$ret] = shell()->execWithResult("{$binary} --ri \"{$this->getDistName()}\"", false);
    return $ret === 0;
}

// class xxx extends Extension
public function check(string $binary): bool
{
    // We can choose whether to check `--ri`
    if (!parent::check($binary)) {
        return false;
    }
    // TODO: Do more check with independent test file, but how?
    return true;
}

1 method getDistName() is only used for checking --ri. If null checking is added, it will make this place a little strange. You know it's a checking process and most extensions follow this check logic.

2 method (I temporarily call it check()), if we choose this to do check, sanity check scripts will become inappropriate. Is it appropriate to put all the checks and special compilation options for a specific extension into one class?

Which one do you think is better? Or a more reasonable solution?

[crazywhalecc]

I introduced the Extension::runCliCheck() method in #253 , which overrides the check extension's script.

The issue has not been formally addressed for a long time. But now everything is working fine, and if there's nothing else to add, it's time to merge. 🎉

[mpociot]

This looks good to me now, I'm already using this for Laravel Herd since its launch 👍

[robdekort]

As a fresh Herd user, thanks for all your work on this @mpociot and @crazywhalecc.