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

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

Closed
wants to merge 2 commits into from

Conversation

mback2k
Copy link
Member

@mback2k mback2k commented Dec 3, 2020

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
Copy link
Member Author

mback2k commented Dec 3, 2020

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
Copy link
Member Author

mback2k commented Dec 4, 2020

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 added a commit to mback2k/curl that referenced this pull request Feb 27, 2021
Move the definition of USE_WIN32_CRYPTO from config-win32.h
to general curl_setup.h to make it work with configure builds.

Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO and USE_NTLM for SMB.

Remove USE_WIN32_CRYPTO from CMake as it is defined internally.

Align condition for NTLM features via CMake with configure builds.

Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Feb 28, 2021
Move the definition of USE_WIN32_CRYPTO from config-win32.h
to general curl_setup.h to make it work with configure builds.

Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO and USE_NTLM for SMB.

Remove USE_WIN32_CRYPTO from CMake as it is defined internally.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Feb 28, 2021
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

Simulate USE_WIN32_CRYPTO detection by checking for CryptCreateHash
in wincrypt.h which is not available in Windows App environment.

Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Feb 28, 2021
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

Simulate USE_WIN32_CRYPTO detection by checking for CryptCreateHash
in wincrypt.h which is not available in Windows App environment.

Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
@mback2k
Copy link
Member Author

mback2k commented Feb 28, 2021

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 added a commit to mback2k/curl that referenced this pull request Mar 1, 2021
Move the definition of USE_WIN32_CRYPTO from config-win32.h
to general curl_setup.h to make it work with configure builds.

Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO and USE_NTLM for SMB.

Remove USE_WIN32_CRYPTO from CMake as it is defined internally.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 1, 2021
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

Simulate USE_WIN32_CRYPTO detection by checking for CryptCreateHash
in wincrypt.h which is not available in Windows App environment.

Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 1, 2021
Move the definition of USE_WIN32_CRYPTO from config-win32.h
to general curl_setup.h to make it work with configure builds.

Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO and USE_NTLM for SMB.

Remove USE_WIN32_CRYPTO from CMake as it is defined internally.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 1, 2021
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

Simulate USE_WIN32_CRYPTO detection by checking for CryptCreateHash
in wincrypt.h which is not available in Windows App environment.

Consolidate additions of crypt32 to linked libraries.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 1, 2021
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

Simulate USE_WIN32_CRYPTO detection by checking for CryptCreateHash
in wincrypt.h which is not available in Windows App environment.

Consolidate additions of crypt32 to linked libraries.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 3, 2021
Move the definition of USE_WIN32_CRYPTO from config-win32.h
to general curl_setup.h to make it work with configure builds.

Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO and USE_NTLM for SMB.

Remove USE_WIN32_CRYPTO from CMake as it is defined internally.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 3, 2021
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

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

Closes curl#6277
@mback2k
Copy link
Member Author

mback2k commented Mar 3, 2021

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 added a commit to mback2k/curl that referenced this pull request Mar 4, 2021
Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO in config-win32.h.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 4, 2021
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.
Use SIZEOF_CURL_OFF_T instead of invalid CURL_SIZEOF_CURL_OFF_T.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
@mback2k
Copy link
Member Author

mback2k commented Mar 4, 2021

@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 added a commit to mback2k/curl that referenced this pull request Mar 4, 2021
Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO in the included
config-win32.h in case no build system is used.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 4, 2021
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.
Use SIZEOF_CURL_OFF_T instead of invalid CURL_SIZEOF_CURL_OFF_T.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Choose a reason for hiding this comment

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

👍

@MarcelRaad
Copy link
Member

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

@mback2k
Copy link
Member Author

mback2k commented Mar 5, 2021

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

mback2k added a commit to mback2k/curl that referenced this pull request Mar 6, 2021
Move the detection of the restricted Windows App environment
before the definition of USE_WIN32_CRYPTO in the included
config-win32.h in case no build system is used.

Part of curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 6, 2021
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.
Use SIZEOF_CURL_OFF_T instead of invalid CURL_SIZEOF_CURL_OFF_T.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 6, 2021
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.
Use SIZEOF_CURL_OFF_T instead of invalid CURL_SIZEOF_CURL_OFF_T.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
mback2k added a commit to mback2k/curl that referenced this pull request Mar 6, 2021
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.
Use SIZEOF_CURL_OFF_T instead of invalid CURL_SIZEOF_CURL_OFF_T.
Fix condition of Schannel SSL backend in CMake build.

Closes curl#6277
CMakeLists.txt Outdated Show resolved Hide resolved
@mback2k
Copy link
Member Author

mback2k commented Mar 8, 2021

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.

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

None yet

3 participants