Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 29, 2020

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.

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.
cmb69 referenced this pull request Jan 29, 2020
With changes to (hopefully) correctly fall back if OpenSSL support
is missing. Furthermore the hard-coded dependency on ext/hash is
no longer an issue, as this extension is required in master.

This reverts commit 63072e9, reversing
changes made to 4cbabb6.
@nikic
Copy link
Member

nikic commented Jan 29, 2020

LG from my side.

@andreyhristov
Copy link

LGTM

@cmb69
Copy link
Member Author

cmb69 commented Jan 29, 2020

Thanks for reviewing! I'm not absolutely sure regarding the dependency on OpenSSL (libcrypto.dll).

@KalleZ
Copy link
Member

KalleZ commented Jan 29, 2020

@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)

@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

@KalleZ, that also has been suggested by @johannes (IIUC). However, that's certainly not an option for PHP 7.4, which this PR should have targeted. Retargeting now.

@cmb69 cmb69 changed the base branch from master to PHP-7.4 January 30, 2020 12:28
@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

Closing and re-opening to trigger CI.

@cmb69 cmb69 closed this Jan 30, 2020
@cmb69 cmb69 reopened this Jan 30, 2020
@nikic
Copy link
Member

nikic commented Jan 30, 2020

@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)

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.

@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

Ah, I see.

@cmb69
Copy link
Member Author

cmb69 commented Jan 31, 2020

That would add a hard dependency on openssl, which is definitely not an option.

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.

@cmb69 cmb69 closed this Jan 31, 2020
@nikic
Copy link
Member

nikic commented Jan 31, 2020

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.

@cmb69
Copy link
Member Author

cmb69 commented Feb 1, 2020

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.

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.

@SugarD-x
Copy link

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).

@cmb69
Copy link
Member Author

cmb69 commented Feb 24, 2020

No, not yet fixed for Windows. See https://bugs.php.net/bug.php?id=79275 for progress.

@nikic
Copy link
Member

nikic commented Feb 24, 2020

@cmb69 Just to calibrate expectations, are there any plans to make this available on Windows in 7.4.x?

@cmb69
Copy link
Member Author

cmb69 commented Feb 24, 2020

@nikic, yes, at least if it's possible by using native Windows crypto APIs (which seems to be).

@SugarD-x
Copy link

I hope that is the case. I've been waiting for this since 7.2.8! :)

@cmb69
Copy link
Member Author

cmb69 commented Feb 25, 2020

See PR #5210.

@cmb69 cmb69 deleted the cmb/caching_sha2_password branch February 26, 2020 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants