Skip to content

Add Windows support for caching_sha2_password - #5129

Closed
cmb69 wants to merge 2 commits into
php:PHP-7.4from
cmb69:cmb/caching_sha2_password
Closed

Add Windows support for caching_sha2_password#5129
cmb69 wants to merge 2 commits into
php:PHP-7.4from
cmb69:cmb/caching_sha2_password

Conversation

@cmb69

Copy link
Copy Markdown
Member

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.
Comment threadext/mysqlnd/mysqlnd_auth.c Outdated
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
Copy Markdown
Member

LG from my side.

@andreyhristov

Copy link
Copy Markdown

LGTM

@cmb69

Copy link
Copy Markdown
MemberAuthor

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

@KalleZ

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

@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.4January 30, 2020 12:28
@cmb69

Copy link
Copy Markdown
MemberAuthor

Closing and re-opening to trigger CI.

@cmb69cmb69 closed this Jan 30, 2020
@cmb69cmb69 reopened this Jan 30, 2020
@nikic

Copy link
Copy Markdown
Member

@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
Copy Markdown
MemberAuthor

Ah, I see.

@cmb69

Copy link
Copy Markdown
MemberAuthor

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.

@cmb69cmb69 closed this Jan 31, 2020
@nikic

Copy link
Copy Markdown
Member

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

cmb69 commented Feb 1, 2020

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown

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
Copy Markdown
MemberAuthor

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

@nikic

Copy link
Copy Markdown
Member

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

@cmb69

Copy link
Copy Markdown
MemberAuthor

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

@SugarD-x

Copy link
Copy Markdown

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

@cmb69

Copy link
Copy Markdown
MemberAuthor

See PR #5210.

@cmb69
cmb69 deleted the cmb/caching_sha2_password branch February 26, 2020 15:08
Sign up for freeto 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

@cmb69@nikic@andreyhristov@KalleZ@SugarD-x