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

Jump to bottom

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 •
edited

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

MarcelRaad
MarcelRaad reviewed May 7, 2021
View reviewed changes
lib/curl_setup.h Outdated
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@theawless theawless May 7, 2021 •
edited

Choose a reason for hiding this comment

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

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 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 05:44
@theawless theawless requested a review from MarcelRaad May 8, 2021 13:08
MarcelRaad
MarcelRaad approved these changes May 8, 2021
View reviewed changes
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.

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 bagder added build feature-window A merge of this requires an open feature window labels May 8, 2021
@bagder bagder removed the feature-window A merge of this requires an open feature window label Jun 2, 2021
@bagder bagder closed this in ee8c4f7 Jun 2, 2021
@bagder
Copy link
Member

bagder commented Jun 2, 2021

Thanks!

bagder pushed a commit that referenced this pull request Jun 2, 2021
@theawless @bagder
configure: add --disable-ntlm option
0e7638d 
Closes #7028
bagder pushed a commit that referenced this pull request Jun 2, 2021
@theawless @bagder
cmake: add CURL_DISABLE_NTLM option
4f209d8 
Closes #7028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarcelRaad MarcelRaad MarcelRaad approved these changes

Assignees
No one assigned
Labels
build
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@theawless @bagder @MarcelRaad