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

Define USE_WIN32_CRYPTO when using CMake on Windows #4717

Closed

Conversation

@marc-groundctl
Copy link
Contributor

@marc-groundctl marc-groundctl commented Dec 13, 2019

This is necessary for SMB support when using SSPI (which is on when using WinSSL). It is unconditionally defined in config-win32.h, so this just adds it to the CMake build as well.

@jay
Copy link
Member

@jay jay commented Dec 14, 2019

It looks like this is causing a bunch of test failures in some builds according to appveyor. It appears NTLM auth strings have changed and/or there's missing smb/smbs protocol in curl-config.

@bagder
Copy link
Member

@bagder bagder commented Dec 15, 2019

The protocols SMB and SMBS don't get set properly in curl-config when cmake is used, which has been reported before but never fixed, making this patch incomplete. See for example this comment

@marc-groundctl
Copy link
Contributor Author

@marc-groundctl marc-groundctl commented Dec 16, 2019

The issue with NTLM is that previous non-SSL builds would not enable NTLM and so the NTLM tests would be skipped. Defining USE_WIN32_CRYPTO causes NTLM to be enabled, but since there is no SSL engine, USE_NTLM2SESSION is not defined (curl_ntlm_core.h:54) and so NTLM2FLAG is set to 0 (ntlm.c:411), but the tests expect it to be set to NTLMFLAG_NEGOTIATE_NTLM2_KEY. I don't know how to change the tests to support not indicating NTLM2 (or even if they should be changed).

@jay
Copy link
Member

@jay jay commented Dec 16, 2019

The issue with NTLM is that previous non-SSL builds would not enable NTLM and so the NTLM tests would be skipped.

Ok. How long has it been like that? Will anyone notice if we make SSL required for NTLM during build? If that can't or shouldn't be changed then I'm tempted to at least make it required during tests. I don't know of a way we can support NTLM testing the non-SSL builds without separate tests since the auth strings are different.

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented Dec 17, 2019

Will anyone notice if we make SSL required for NTLM during build?

Probably, yes. Until some years ago, I used to build libcurl without SSL support but with NTLM support (for proxy authentication) through SSPI and relied on that.

make it required during tests

+1

@marc-groundctl
Copy link
Contributor Author

@marc-groundctl marc-groundctl commented Dec 17, 2019

How long has it been like that?

Always. Before this patch, CMake didn't support NTLM without SSL.

jay added a commit to jay/curl that referenced this pull request Dec 29, 2019
Prior to this change tests that required NTLM feature did not require
SSL feature.

There are pending changes to cmake builds that will allow enabling NTLM
in non-SSL builds in Windows. In that case the NTLM auth strings created
are different from what is expected by the NTLM tests and they fail:

"The issue with NTLM is that previous non-SSL builds would not enable
NTLM and so the NTLM tests would be skipped."

Assisted-by: marc-groundctl@users.noreply.github.com

Ref: curl#4717 (comment)

Closes #xxxx
@jay
Copy link
Member

@jay jay commented Dec 29, 2019

Please rebase off of #4768

@marc-groundctl marc-groundctl force-pushed the GroundControl-Solutions:enable-smb branch from 5d7dc08 to de331f3 Dec 30, 2019
jay added a commit that referenced this pull request Dec 31, 2019
Prior to this change tests that required NTLM feature did not require
SSL feature.

There are pending changes to cmake builds that will allow enabling NTLM
in non-SSL builds in Windows. In that case the NTLM auth strings created
are different from what is expected by the NTLM tests and they fail:

"The issue with NTLM is that previous non-SSL builds would not enable
NTLM and so the NTLM tests would be skipped."

Assisted-by: marc-groundctl@users.noreply.github.com

Ref: #4717 (comment)

Closes #4768
@jay
Copy link
Member

@jay jay commented Dec 31, 2019

Test 168 is failing in two builds but I can't figure out why. Coincidence?

You can rebase on master now, I landed #4768 earlier this evening.

@marc-groundctl
Copy link
Contributor Author

@marc-groundctl marc-groundctl commented Jan 2, 2020

Test 168 is failing in two builds but I can't figure out why. Coincidence?

The underlying reason that tests 168 and 335 fail sometimes is that they send extra data that curl is supposed to ignore, but curl ignores this data by closing the connection without reading the data, and so if that happens before the test sws server can finish writing it then send_doc will return early (line 1276), which will cause prevtestno and prevpartno to not be set, which will cause the swsbounce check in service_connection (line 1933) to fail, which will cause the wrong response data to be sent. You can reliably cause this to happen by modifying sws.c to write 1 byte at a time and wait 100 milliseconds after each write.

@marc-groundctl marc-groundctl force-pushed the GroundControl-Solutions:enable-smb branch from ec5142e to d0b8742 Jan 2, 2020
jay added a commit that referenced this pull request Jan 3, 2020
Prior to this change the swsbounce check in service_connection could
fail because prevtestno and prevpartno were not set, which would cause
the wrong response data to be sent to some tests and cause them to fail.

Ref: #4717 (comment)
@jay
Copy link
Member

@jay jay commented Jan 3, 2020

Thanks, I've landed the test fix.

@@ -1199,6 +1204,14 @@ if(BUILD_TESTING)
add_subdirectory(tests)
endif()

# 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_WINDOWS_SSPI OR USE_DARWINSSL OR USE_MBEDTLS OR USE_WIN32_CRYPTO))

This comment has been minimized.

@jay

jay Jan 3, 2020
Member

This is missing NSS and SECTRANSP (what I'm pretty sure replaces DARWINSSL). Is there a reason for that?

This comment has been minimized.

@marc-groundctl

marc-groundctl Jan 3, 2020
Author Contributor

No, that's a mistake.

@marc-groundctl
Copy link
Contributor Author

@marc-groundctl marc-groundctl commented Jan 3, 2020

The tests for this new commit are failing because the SMTP server is going down. When test 900 first connects, the sockfilt server attempts to write to stdout, but that fails with ERROR_NO_DATA (The pipe is being closed) and exits. Then every subsequent SMTP test fails because there's no SMTP server.

I see no reason for the pipe to have closed, I can't reproduce this locally, and I can't see any way the most recent commit could have caused this, so I'm inclined to chalk this up to Windows or perl doing something weird and this being a spurious failure. Certainly the later test failures are spurious, since they're all caused by the SMTP server not being up.

@jay jay closed this in ea6d620 Jan 12, 2020
@jay
Copy link
Member

@jay jay commented Jan 12, 2020

Thanks. I'm not sure what's up with the SMTP server going down. I will open up a separate issue for that.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.