Skip to content

config: fix building SMB with configure using Win32 Crypto - #6277

Closed
mback2k wants to merge 2 commits into
curl:masterfrom
mback2k:fix-win32-configure-smb
Closed

config: fix building SMB with configure using Win32 Crypto#6277
mback2k wants to merge 2 commits into
curl:masterfrom
mback2k:fix-win32-configure-smb

Conversation

@mback2k

@mback2k mback2k commented Dec 3, 2020

Copy link
Copy Markdown
Member

Align conditions for NTLM features between CMake and configure
builds by differentiating between USE_NTLM and USE_CURL_NTLM_CORE,
just like curl_setup.h does internally to detect support of:

  • USE_NTLM: required for NTLM crypto authentication feature
  • USE_CURL_NTLM_CORE: required for SMB protocol

Implement USE_WIN32_CRYPTO detection by checking for Crypt functions
in wincrypt.h which are not available in the Windows App environment.

Link advapi32 and crypt32 for Crypto API and Schannel SSL backend.
Fix condition of Schannel SSL backend in CMake build.

Move the detection of the restricted Windows App environment
in curl_setup.h before the definition of USE_WIN32_CRYPTO
via included config-win32.h in case no build system is used.

@mback2k mback2k self-assigned this Dec 3, 2020
@mback2k mback2k added SMB Windows Windows-specific labels Dec 3, 2020
@mback2k

mback2k commented Dec 3, 2020

Copy link
Copy Markdown
Member Author

At the moment configure is not aware of USE_WIN32_CRYPTO at all, but CMake is and therefore the detection of SMB support is quite different:

curl/configure.ac

Lines 5030 to 5042 in 753a2c7

if test "x$CURL_DISABLE_CRYPTO_AUTH" != "x1"; then
if test "x$OPENSSL_ENABLED" = "x1" -o "x$USE_WINDOWS_SSPI" = "x1" \
-o "x$GNUTLS_ENABLED" = "x1" -o "x$MBEDTLS_ENABLED" = "x1" \
-o "x$NSS_ENABLED" = "x1" -o "x$SECURETRANSPORT_ENABLED" = "x1" \
-o "x$WOLFSSL_NTLM" = "x1"; then
SUPPORT_FEATURES="$SUPPORT_FEATURES NTLM"
if test "x$CURL_DISABLE_HTTP" != "x1" -a \
"x$NTLM_WB_ENABLED" = "x1"; then
SUPPORT_FEATURES="$SUPPORT_FEATURES NTLM_WB"
fi
fi
fi

curl/configure.ac

Lines 5130 to 5140 in 753a2c7

if test "x$CURL_DISABLE_SMB" != "x1" \
-a "x$CURL_DISABLE_CRYPTO_AUTH" != "x1" \
-a \( "x$OPENSSL_ENABLED" = "x1" \
-o "x$GNUTLS_ENABLED" = "x1" -o "x$MBEDTLS_ENABLED" = "x1" \
-o "x$NSS_ENABLED" = "x1" -o "x$SECURETRANSPORT_ENABLED" = "x1" \
-o "x$WOLFSSL_NTLM" = "x1" \); then
SUPPORT_PROTOCOLS="$SUPPORT_PROTOCOLS SMB"
if test "x$SSL_ENABLED" = "x1"; then
SUPPORT_PROTOCOLS="$SUPPORT_PROTOCOLS SMBS"
fi
fi

vs:

curl/CMakeLists.txt

Lines 507 to 509 in 753a2c7

if(WIN32)
set(USE_WIN32_CRYPTO ON)
endif()

curl/CMakeLists.txt

Lines 1333 to 1339 in 753a2c7

# NTLM support requires crypto function adaptions from various SSL libs
# TODO alternative SSL libs tests for SSP1, GNUTLS, NSS
if(NOT CURL_DISABLE_CRYPTO_AUTH AND (USE_OPENSSL OR USE_DARWINSSL OR USE_MBEDTLS OR USE_WIN32_CRYPTO))
set(use_ntlm ON)
else()
set(use_ntlm OFF)
endif()

I would like to suggest to either make USE_WIN32_CRYPTO something that is set by the configuration tooling (configure, CMake, etc.) or something that is set inside the curl source code automatically based on the other definitions, like this PR does. Therefore I would like to suggest removing it from the CMake files.

@mback2k

mback2k commented Dec 4, 2020

Copy link
Copy Markdown
Member Author

It seems like the SMB support detection and therefore also NTLM and for that Win32 Crypto detection will need to be moved into the configure scripts in order to make tests 1013 and 1014 work correctly. I will take a look at updating this PR accordingly.

@mback2k
mback2k marked this pull request as draft December 23, 2020 09:46
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 07ac5da to 1e8b97b Compare February 27, 2021 21:06
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 1e8b97b to a757a48 Compare February 28, 2021 20:43
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from a757a48 to 42680c2 Compare February 28, 2021 20:45
@mback2k

mback2k commented Feb 28, 2021

Copy link
Copy Markdown
Member Author

This whole comment is obsolete, see #6277 (comment):

Just pushed a new approach which aligns CMake and configure builds and leads to the following situation:

  • CMake and configure test for Crypt*-functions in wincrypt.h to detect USE_WIN32_CRYPTO and therefore consider SMB enabled via USE_CURL_NTLM_CORE (or if any other crypto backend is available).
  • libcurl actually does not rely on USE_WIN32_CRYPTO or USE_CURL_NTLM_CORE being set by the build system, instead it relies on detecting the Windows version and restricted Windows App environment in curl_setup.h (previously config-win32.h, see ntlm: fix condition for curl_ntlm_core usage #5771) to set USE_WIN32_CRYPTO and therefore USE_CURL_NTLM_CORE (unless crypto authentication is disabled).

Previously USE_WIN32_CRYPTO was set by the build system (just CMake) and config-win32.h, now it is just used as a helper variable in in the build systems to detect supported features, but actually only really setup and used internally inside libcurl.

@bagder is that okay, or is this kind of split-brain situation unwanted and the logic needs to be completely revamped?

@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 42680c2 to 83da2af Compare March 1, 2021 19:38
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 83da2af to 2194c11 Compare March 1, 2021 20:38
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 2194c11 to 4d056da Compare March 1, 2021 21:04
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 4d056da to 5f1a6b5 Compare March 3, 2021 20:27
@mback2k

mback2k commented Mar 3, 2021

Copy link
Copy Markdown
Member Author

TODO via separate PR once landed: update schannel.c to require USE_WIN32_CRYPTO just like USE_WINDOWS_SSPI.

@mback2k
mback2k requested a review from bagder March 3, 2021 20:52
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from c5d329f to deaaf99 Compare March 4, 2021 21:02
@mback2k

mback2k commented Mar 4, 2021

Copy link
Copy Markdown
Member Author

@MarcelRaad I just updated this PR and #6277 (comment) is now obsolete. Instead everything should be as one would expect, e.g. config-win32.h still defines USE_WIN32_CRYPTO based on CURL_WINDOWS_APP. If a build system like CMake or autotools is used, it is defined by that instead.

@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from deaaf99 to a6226fa Compare March 4, 2021 21:11

@MarcelRaad MarcelRaad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@MarcelRaad

Copy link
Copy Markdown
Member

Some CI tests are still failing, but in general this looks good to me.

@mback2k

mback2k commented Mar 5, 2021

Copy link
Copy Markdown
Member Author

Autotools based builds are fully functional now, but CMake still does not work reliable. Adding some debug commits...

@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from bc0761d to 8ff51d2 Compare March 6, 2021 14:52
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 8ff51d2 to dfa4a74 Compare March 6, 2021 20:39
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from dfa4a74 to 8356f7f Compare March 6, 2021 20:57
Comment thread CMakeLists.txt Outdated
@mback2k

mback2k commented Mar 8, 2021

Copy link
Copy Markdown
Member Author

Autotools based builds are fully functional now, but CMake still does not work reliable. Adding some debug commits...

CMake builds now also correctly handle crypt32, NTLM and SMB support, but the fix to Largefile detection does not work, see #6277 (comment). Thinking about doing a separate PR for that one then.

mback2k added 2 commits March 12, 2021 20:01
Move the detection of the restricted Windows App environment
in curl_setup.h before the definition of USE_WIN32_CRYPTO
via included config-win32.h in case no build system is used.

Reviewed-by: Marcel Raad

Part of curl#6277
Align conditions for NTLM features between CMake and configure
builds by differentiating between USE_NTLM and USE_CURL_NTLM_CORE,
just like curl_setup.h does internally to detect support of:

- USE_NTLM: required for NTLM crypto authentication feature
- USE_CURL_NTLM_CORE: required for SMB protocol

Implement USE_WIN32_CRYPTO detection by checking for Crypt functions
in wincrypt.h which are not available in the Windows App environment.

Link advapi32 and crypt32 for Crypto API and Schannel SSL backend.
Fix condition of Schannel SSL backend in CMake build accordingly.

Reviewed-by: Marcel Raad

Closes curl#6277
@mback2k
mback2k force-pushed the fix-win32-configure-smb branch from 8356f7f to fdeb031 Compare March 12, 2021 19:05
@mback2k
mback2k marked this pull request as ready for review March 12, 2021 20:50
mback2k added a commit to mback2k/curl that referenced this pull request Mar 15, 2021
Move the detection of the restricted Windows App environment
in curl_setup.h before the definition of USE_WIN32_CRYPTO
via included config-win32.h in case no build system is used.

Reviewed-by: Marcel Raad

Part of curl#6277
@mback2k mback2k closed this in cc615f4 Mar 15, 2021
mback2k added a commit to mback2k/curl that referenced this pull request Mar 28, 2021
Avoid enabling NTLM feature based upon Windows SSPI
being enabled in case that crypto auth is disabled.

Reported-by: Marcel Raad

Follow up to curl#6277
Fixes curl#6803
bagder pushed a commit that referenced this pull request Mar 29, 2021
Avoid enabling NTLM feature based upon Windows SSPI
being enabled in case that crypto auth is disabled.

Reported-by: Marcel Raad

Follow-up to #6277
Fixes #6803
Closes #6808
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SMB Windows Windows-specific

Development

Successfully merging this pull request may close these issues.

3 participants