Skip to content
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

lib/config-win32.h: let SMB/SMBS be enabled with OpenSSL/NSS #1943

Closed
wants to merge 1 commit into from
Closed

lib/config-win32.h: let SMB/SMBS be enabled with OpenSSL/NSS #1943

wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 4, 2017

The source code is now prepared to handle the case when both
Win32 Crypto and OpenSSL/NSS crypto backends are enabled
at the same time, making it possible to enable USE_WIN32_CRYPTO
whenever the targeted Windows version supports it. Since this
matches the minimum Windows version supported by curl
(Windows 2000), enable it unconditionally for the Win32 platform.

This in turn enables SMB (and SMBS) protocol support whenever
Win32 Crypto is available, regardless of what other crypto backends
are enabled.

Ref: #1840 (comment)

@@ -709,7 +709,7 @@ Vista
#endif

/* Define to use the Windows crypto library. */
#if !defined(USE_OPENSSL) && !defined(USE_NSS)
#if defined(USE_SCHANNEL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have changed it to use win32 crypto only if schannel but is that really what you meant? Aren't crypto and schannel two separate things, and now that there are changes in ntlm to handle win32 crypto properly can't we just enable USE_WIN32_CRYPTO all the time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going along @dscho's suggestion here, but, yes, as far as I can see the code, this macro is protecting crypto-only and unless I'm missing something NTLM-crypto-only functions (2-4 of them) throughout all lib sources. If this is correct, it could be enabled based on _WIN32.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're in config-win32.h, _WIN32 should always be defined, meaning that USE_WIN32_CRYPTO could just be enabled unconditionally in this header.

Copy link
Member Author

@vszakats vszakats Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE_WIN32_CRYPTO requires: wincrypt.h, CryptAcquireContext(), CryptCreateHash(), CryptHashData(), CryptGetHashParam(), CryptDestroyHash(), CryptReleaseContext(), CryptImportKey(), CryptImportKey(), CryptDestroyKey().

As per MSDN, these are available starting Windows XP/2003, so the condition required is _WIN32_WINNT >= 0x0501.

Copy link
Member

@MarcelRaad MarcelRaad Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vszakats

As per MSDN, these are available starting Windows XP/2003

That's only because mentions of old unsupported OSs are regularly removed from the MSDN documentation. https://msdn.microsoft.com/en-us/library/ms867086.aspx says it's at least available since Windows NT 4 and Windows 95.

Copy link
Member

@jay jay Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI Windows 2000 has all those functions xported in advapi32. Whether they're actually declared or not for Windows 2k I don't know (yes in original mingw), but we've never had an os version check on that symbol before and people do build for Win2k at least according to the survey. I think if you remove the guard it will be fine. If someone is building for Windows 98 (how?) we can wait for feedback if it breaks for them.

edit: run it through CI just to be sure

Copy link
Member Author

@vszakats vszakats Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll commit that and wait for a green flag of course. MSDN tends to be unreliable for minimum versions, exactly like @MarcelRaad says.

The source code is now prepared to handle the case when both
Win32 Crypto and OpenSSL/NSS crypto backends are enabled
at the same time, making it now possible to enable `USE_WIN32_CRYPTO`
whenever the targeted Windows version supports it. Since this
matches the minimum Windows version supported by curl
(Windows 2000), enable it unconditionally for the Win32 platform.

This in turn enables SMB (and SMBS) protocol support whenever
Win32 Crypto is available, regardless of what other crypto backends
are enabled.

Ref: #1840 (comment)
@vszakats vszakats changed the title lib/config-win32.h: let win32 crypto be enabled with openssl/nss lib/config-win32.h: let smb/smbs be enabled with openssl/nss Oct 6, 2017
@vszakats vszakats changed the title lib/config-win32.h: let smb/smbs be enabled with openssl/nss lib/config-win32.h: let SMB/SMBS be enabled with OpenSSL/NSS Oct 6, 2017
@vszakats vszakats closed this in 24bba40 Oct 6, 2017
@vszakats vszakats deleted the smbauto branch October 15, 2017 23:14
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
@bagder bagder added the SMB label May 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants