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

HTTP/SASL/SOCKS: Implement CURLOPT_SAFE_AUTH #8295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

monnerat
Copy link
Contributor

@monnerat monnerat commented Jan 19, 2022

These options disable clear text password authentication mechanisms over
a non-encrypted connection. In particular:

  • HTTP Basic authentication is disabled
  • PLAIN and LOGIN mechanisms are disabled for all SASL protocols
  • IMAP LOGIN is disabled
  • POP3 USER/PASS authentication is disabled
  • FTP authentication is disabled unless the user name is "anonymous"
  • LDAP simple bind is disabled if not anonymous.
  • SOCKS5 proxy basic authentication is disabled.

TLS-encrypted connections (either implicit or explicit) are not affected
as they wont transport clear text passwords.

These options are triggered by the new curl tool options --safe-auth and
--proxy-safe-auth respectively.

New tests 730-746 check these options in various contexts.

@bagder
Copy link
Member

bagder commented Jan 20, 2022

Test 452 needs to be disabled for rustls for now:

12:24:46.379717 == Info: rustls_connect: unimplemented

(there's a list of disabled tests in tests/data/DISABLED already, just add 452 to that)

@monnerat
Copy link
Contributor Author

Thanks for the hint.
I was scratching my head about this failing test that is very similar to test400 and I did not see the latter was already disabled.

.SH DESCRIPTION
Pass a long as parameter, 0 or 1 where 1 is for enable and 0 for disable.
When set, This option rejects authentication mechanisms that would
transfer clear passwords on a non-encrypted connection.
Copy link
Member

Choose a reason for hiding this comment

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

It feels reasonable to expect a user to mostly want to set this for both host+proxy at the same time, or not at all. Therefore I think it could make sense to be able to set both using the same libcurl option using a bitmask instead of adding two separate options.

Copy link
Member

Choose a reason for hiding this comment

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

What happens if this option is set true and CURLOPT_HTTPAUTH sets Basic? It would probably help if this spells that out.

Copy link
Contributor Author

@monnerat monnerat Jan 29, 2022

Choose a reason for hiding this comment

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

Thanks for the review.

What happens if this option is set true and CURLOPT_HTTPAUTH sets Basic?

Sure, these are contradictory options.
In case it occurs, the CURLOPT_SAFE_AUTH takes the precedence. This is also true for SASL mechanisms. I'll add a doc paragraph for that.

Copy link
Contributor Author

@monnerat monnerat Jan 29, 2022

Choose a reason for hiding this comment

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

... set both using the same libcurl option using a bitmask instead of adding two separate options.

I did it that way because we don't have a curl_easy_getopt() (BTW: possible future feature?). But I'll change it if you prefer.
What about flag names? I suggest CURLSAFE_AUTH and CURLSAFE_PROXYAUTH.

lib/http.c Outdated
#define SAFE_AUTH_MECHS (CURLAUTH_NEGOTIATE | CURLAUTH_BEARER | \
CURLAUTH_DIGEST | CURLAUTH_NTLM | \
CURLAUTH_NTLM_WB | CURLAUTH_AWS_SIGV4)

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be the same bitmask as CURLAUTH_ANYSAFE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I missed it. Will change.

.SH DEFAULT
0, disabled
.SH PROTOCOLS
FTP, HTTP, IMAP, LDAP, POP3 and SMTP
Copy link
Member

Choose a reason for hiding this comment

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

Hm don't SCP and SFTP then basically always have this option implied? I don't think they can send any auth in clear text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ssh protocol always login on a secure channel. See https://www.ssh.com/academy/ssh/protocol#how-does-the-ssh-protocol-work.
Thus there's no need to handle these options there.

@monnerat
Copy link
Contributor Author

Ping ?

@monnerat
Copy link
Contributor Author

monnerat commented Jun 2, 2022

@bagder: I was not aware you were reluctant about this PR: your review did not comment in this sense.
This feature has already be mentioned quite a few times as needed both on github and the mailing list in the past years, although not listed in TODO.
It can be emulated for HTTP-based protocols using CURLAUTH_ANYSAFE but is completely missing for SASL protocols.
SASL mechanisms are distinct from the HTTP ones and are not public symbols. The only way to select/limit them is via the login option string.

Alternatives to this PR are:

  1. A new option for SASL only, either behaving as the current CURLOPT_SAFE_AUTH or as a mechanism mask setting option (this requires making the flag bits public). In the latter case, how to combine the mask with the login option string ?
  2. Unify the mechanism flag bits between HTTP and SASL: HTTP auth options would then be available to SASL (same combination remark as above).
  3. Leave as it is now, regretting this decision each time someone moan this feature would be a plus !

Additional question: without this PR, how to block clear text non-SASL logins (FTP, IMAP, POP3, LDAP, SOCKS5) ?

I'm open to develop an alternative solution if I have some hints in which direction to go, in order not to get a needs-votes label on it and have a quick commit.
Else I vote for the current one (if the author can vote!)

@monnerat monnerat force-pushed the safeauth branch 2 times, most recently from 0b062f0 to 0ae77ec Compare November 20, 2023 11:32
@monnerat monnerat force-pushed the safeauth branch 3 times, most recently from bcd6b82 to 6219fb5 Compare December 20, 2023 11:18
@monnerat monnerat force-pushed the safeauth branch 4 times, most recently from b8b5fe6 to f42f8b3 Compare January 24, 2024 08:19
@monnerat monnerat force-pushed the safeauth branch 2 times, most recently from ed291f7 to 43761f3 Compare January 30, 2024 14:04
@monnerat monnerat force-pushed the safeauth branch 2 times, most recently from 78382be to f3d46fb Compare March 1, 2024 12:01
@monnerat monnerat force-pushed the safeauth branch 2 times, most recently from db03ad8 to 1118c7e Compare March 25, 2024 13:25
@monnerat monnerat force-pushed the safeauth branch 2 times, most recently from 9de2f7e to 13a9acd Compare April 17, 2024 13:55
This option disables clear text password authentication mechanisms over
a non-encrypted connection. In particular:
- HTTP Basic authentication is disabled
- PLAIN and LOGIN mechanisms are disabled for all SASL protocols
- IMAP LOGIN is disabled
- POP3 USER/PASS authentication is disabled
- FTP authentication is disabled unless the user name is "anonymous"
- LDAP simple bind is disabled if not anonymous.
- SOCKS5 proxy basic authentication is disabled.

TLS-encrypted connections (either implicit or explicit) are not affected
as they won't transport clear text passwords.

This option is triggered by the new curl tool options --safe-auth and
--proxy-safe-auth.

New tests 750-766 check this option in various contexts.
@bagder
Copy link
Member

bagder commented Jun 15, 2024

  1. When performed over an authenticated channel, why consider them "unsafe"?
  2. When --safe-auth is used, should it then not rather return an error if an unsafe auth method (over an unauthenticated protocol) is attempted?

@monnerat
Copy link
Contributor Author

When performed over an authenticated channel, why consider them "unsafe"?

What are you calling an *authenticated channel"?
The connection we are dealing with is obviously not yet authenticated.
An encrypted (wether authenticated or not) proxy carrier connection may be considered as safe up to the proxy itself, but the final clear connection between the proxy and the server may be sniffed.

When --safe-auth is used, should it then not rather return an error if an unsafe auth method (over an unauthenticated protocol) is attempted?

This is what will finally happen if no safe auth method can be selected.

@bagder
Copy link
Member

bagder commented Jun 17, 2024

An authenticated channel would be using TLS, SSH or QUIC (without --insecure).

Your test 750 does a Basic auth for http://. It seems to strip off the auth, not fail the operation. I don't think a user would like that. I think it should rather fail as early as possible because it cannot perform the operation "safely".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authentication CI Continuous Integration cmdline tool libcurl API needs-votes Pull-request in need of thumbs-ups to make progress tests
Development

Successfully merging this pull request may close these issues.

None yet

5 participants