-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Add Windows support for caching_sha2_password #5129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This requires OpenSSL to be linked to php*.dll. We also add an `alloca()` based fallback for MSVC (and potentially other compilers), which does not support variable length arrays.
LG from my side. |
LGTM |
Thanks for reviewing! I'm not absolutely sure regarding the dependency on OpenSSL (libcrypto.dll). |
@cmb69 perhaps we should look at a builtin ext/openssl, but I do wonder about the binary size increase if we are do that but it would make cases like these a lot easier (similar to how we did with ext/hash) |
Closing and re-opening to trigger CI. |
That would add a hard dependency on openssl, which is definitely not an option. This only worked for ext/hash because it has no dependencies. |
Ah, I see. |
So this PR is not an option as well. Building ext/mysqlnd statically is also likely not an option for BC reasons. So it seems for caching_sha2_password support on Windows, we have to use the CryptoAPI. I'll try to come up with a solution, but that is highly unlikely to make into PHP 7.4.3RC1. Sorry. |
My comment was referring to making PHP have a hard dep on openssl in general. I don't really see an issue with having it as a dependency on our standard Windows builds, as long as you can still disable it if you really want to. We just shouldn't have any hard dependencies in a "minimal" PHP build, beyond libc. |
To disable it, you would have to build PHP yourself, and AFAIK, almost nobody does that. And anybody using a standard Windows build, would suddenly (in a revision release) have a hard dependency on OpenSSL, even if they don't use it (nor mysqlnd). I don't think we should go this way. Run-time linking (dl() etc.) might be an option, but that is somewhat brittle per se. |
Was this ever fixed? I noticed PHP 7.4.3 is out, but this is something that wasn't mentioned in the changelog. (The fix for the non-Windows version wasn't mentioned in the changelog in any of the previous releases either, so it is hard to tell). |
No, not yet fixed for Windows. See https://bugs.php.net/bug.php?id=79275 for progress. |
@cmb69 Just to calibrate expectations, are there any plans to make this available on Windows in 7.4.x? |
@nikic, yes, at least if it's possible by using native Windows crypto APIs (which seems to be). |
I hope that is the case. I've been waiting for this since 7.2.8! :) |
See PR #5210. |
This requires OpenSSL to be linked to php*.dll. We also add an
alloca()
based fallback for MSVC (and potentially other compilers),which does not support variable length arrays.