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

Re-add CURL_DISABLE_NTLM #7028

Closed
wants to merge 3 commits into from
Closed

Re-add CURL_DISABLE_NTLM #7028

wants to merge 3 commits into from

Conversation

@theawless
Copy link
Contributor

@theawless theawless commented May 7, 2021

This pull request is meant to re-add CURL_DISABLE_NTLM flag and corresponding ifdefs.
It more or less reverts the pull request: #6809.

Conversation on IRC:

[12:06:52] <theawless> Hi! I want to enable CRYPTO_AUTH but disable NTLM completely while building curl from source.
[12:06:52] <theawless> In old versions of curl this was possible using the flag CURL_DISABLE_NTLM, but I see that recently it's been removed: https://github.com/curl/curl/pull/6809.
[12:06:52] <theawless> Is there any other way I can do this? I couldn't find any build flags either...
[12:07:40] <bagder> no, it wasn't done properly so we (I) removed it
[12:07:52] <bagder> you could bring it back
[12:08:05] <bagder> done correctly I wouldn't mind
[12:10:00] (curl) bagder pushed 5c53bd9 to master: ngtcp2: fix the cb_acked_stream_data_offset proto - https://git.io/J3Dkr
[12:14:23] <theawless> so a configure option like  `--disable-ntlm` would be good? it will set the flag CURL_DISABLE_NTLM as true and then we can bring back the if defs.
[12:14:50] <bagder> yes, and also add it to docs/CURL-DISABLE.md

The flag was originally removed because it was not getting set anywhere.
As per the conversation on IRC, I have tried to add build options exposing the flag in both CMake and autotools build.
These changes will allow the users to build curl from source with CRYPTO_AUTH support but without NTLM support.
I have performed basic sanity testing but do let me know if anything more is required.

Output from make checksrc

curl % make checksrc
(cd lib && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
  RUN      checksrc
(cd src && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
  RUN      checksrc
(cd tests && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
cd libtest && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc
  RUN      checksrc
cd unit && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc
  RUN      checksrc
cd server && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc
  RUN      checksrc
(cd include/curl && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
  RUN      checksrc
(cd docs/examples && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
  RUN      checksrc
(cd packages && /Applications/Xcode.app/Contents/Developer/usr/bin/make checksrc)
  RUN      checksrc

Output from ./configure --help

...TRUNCATED...
  --enable-crypto-auth    Enable cryptographic authentication
  --disable-crypto-auth   Disable cryptographic authentication
  --enable-ntlm           Enable NTLM support
  --disable-ntlm          Disable NTLM support
  --enable-ntlm-wb[=FILE] Enable NTLM delegation to winbind's ntlm_auth
                          helper, where FILE is ntlm_auth's absolute filename
                          (default: /usr/bin/ntlm_auth)
  --disable-ntlm-wb       Disable NTLM delegation to winbind's ntlm_auth
                          helper
...TRUNCATED...

Output from ./configure --with-secure-transport

...TRUNCATED...
checking whether to enable cryptographic authentication methods... yes
checking whether to support NTLM... yes
checking whether to enable NTLM delegation to winbind's helper... yes
...TRUNCATED...
  Protocols:        DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP LDAPS MQTT POP3 POP3S RTMP RTSP SMB SMBS SMTP SMTPS TELNET TFTP
  Features:         AsynchDNS HSTS IDN IPv6 Largefile NTLM NTLM_WB SSL UnixSockets alt-svc brotli libz zstd

Output from ./configure --with-secure-transport --disable-ntlm

...TRUNCATED...
checking whether to enable cryptographic authentication methods... yes
checking whether to support NTLM... no
checking whether to enable NTLM delegation to winbind's helper... yes
...TRUNCATED...
  Protocols:        DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP LDAPS MQTT POP3 POP3S RTMP RTSP SMTP SMTPS TELNET TFTP
  Features:         AsynchDNS HSTS IDN IPv6 Largefile SSL UnixSockets alt-svc brotli libz zstd

Output from cmake -DCURL_DISABLE_NTLM=ON .

-- curl version=[7.77.0-DEV]
-- Enabled features: SSL IPv6 unixsockets libz AsynchDNS Largefile alt-svc HSTS HTTPS-proxy
-- Enabled protocols: DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP MQTT POP3 POP3S RTSP SCP SFTP SMTP SMTPS TELNET TFTP
-- Enabled SSL backends: OpenSSL
-- Configuring done
-- Generating done

Output from cmake -DCURL_DISABLE_NTLM=OFF .

-- curl version=[7.77.0-DEV]
-- Enabled features: SSL IPv6 unixsockets libz AsynchDNS Largefile alt-svc HSTS NTLM HTTPS-proxy
-- Enabled protocols: DICT FILE FTP FTPS GOPHER GOPHERS HTTP HTTPS IMAP IMAPS LDAP MQTT POP3 POP3S RTSP SCP SFTP SMB SMBS SMTP SMTPS TELNET TFTP
-- Enabled SSL backends: OpenSSL
-- Configuring done
-- Generating done
@@ -641,7 +641,7 @@ int netware_init(void);
#endif

/* Single point where USE_NTLM definition might be defined */
#ifndef CURL_DISABLE_CRYPTO_AUTH
#if !defined(CURL_DISABLE_NTLM) && !defined(CURL_DISABLE_CRYPTO_AUTH)

This comment has been minimized.

@MarcelRaad

MarcelRaad May 7, 2021
Member

This should probably also apply for the USE_NTLM below. So for SSPI, we wouldn't want NTLM either with CURL_DISABLE_NTLM.

This comment has been minimized.

@theawless

theawless May 7, 2021
Author Contributor

The scope of this if ends at line 662 so USE_NTLM will be disabled even when USE_WINDOWS_SSPI is enabled.
I can try to improve the readability here by adding new lines here and there.

Thanks, your comment led me to the bug in CMake build in this patch which has the exact same issue.

@theawless theawless changed the title Re add CURL_DISABLE_NTLM Re-add CURL_DISABLE_NTLM May 7, 2021
@theawless theawless changed the title Re-add CURL_DISABLE_NTLM [WIP] Re-add CURL_DISABLE_NTLM May 7, 2021
@theawless theawless force-pushed the theawless:master branch from e1d824e to 5481822 May 7, 2021
@theawless theawless changed the title [WIP] Re-add CURL_DISABLE_NTLM Re-add CURL_DISABLE_NTLM May 7, 2021
@theawless theawless marked this pull request as ready for review May 8, 2021
@theawless theawless requested a review from MarcelRaad May 8, 2021
Copy link
Member

@MarcelRaad MarcelRaad left a comment

Thanks, looks good to me! We have to wait until the feature window opens again though as we're in the bugfix-only period.

@bagder
Copy link
Member

@bagder bagder commented Jun 2, 2021

Thanks!

bagder added a commit that referenced this pull request Jun 2, 2021
bagder added a commit that referenced this pull request Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants