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

Ability to disable specific auths (Basic, Bearer, Digest, etc.) and non-standard like AWS. #11490

Closed
wants to merge 13 commits into from

Conversation

wyattoday
Copy link
Contributor

@wyattoday wyattoday commented Jul 20, 2023

In the spirit of tiny-curl deprecated, optional, and non-standard code should be able to be compiled-out.

AWS-SIG4 is an organization-specific (namely, Amazon) chunk of code that is not applicable outside of communicating with Amazon servers. It is not standardized. Personally, I think this should be disabled by default in libCurl (and maybe even curl-cli too), but a good place to start is even adding the ability to disable it.

Digest Auth is actually a standard (unlike AWS-SIG4) but has long-since become deprecated (both by standards organizations and by end-user corporations that had previously used Digest).

Lastly, Bearer, Basic, Negotiate, and Kerberos auth types are also configurably disabled. These are all safe authentication methods, but not every user of curl / libcurl will need them. Hence, reduce compiled binary size (and executed run-time checks) when they're not needed.

This PR continues that by adding 6 configure options / defines:

--disable-basic-auth
--disable-bearer-auth
--disable-digest-auth
--disable-kerberos-auth
--disable-negotiate-auth
--disable-aws
CURL_DISABLE_BASIC_AUTH
CURL_DISABLE_BEARER_AUTH
CURL_DISABLE_DIGEST_AUTH
CURL_DISABLE_KERBEROS_AUTH
CURL_DISABLE_NEGOTIATE_AUTH
CURL_DISABLE_AWS

This also removes the general CURL_DISABLE_CRYPTO_AUTH (as suggested below by @bagder ) since the specific auth-types can be disabled one-by-one.

Personally, I think both Digest & AWS should be disabled by default. But this is a good start.

We already deploy and use versions of libcurl with these auths (and AWS) disabled (and have for a few years now).

@dfandrich
Copy link
Contributor

dfandrich commented Jul 20, 2023 via email

@wyattoday
Copy link
Contributor Author

--disable-crypto-auth is already there to disable them all, Basic notwithstanding.

Yeah, Basic isn't in there. And there's the case where you need NTLM but --disable-crypto-auth removes that ability.

Plus this removes AWS and give the ability for fine-grained disabling of individual auths that aren't needed.

configure.ac Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Jul 23, 2023

--disable-basic-auth
--disable-bearer-auth
--disable-digest-auth
--disable-aws

Shouldn't this then also include ntlm and negotiate to the mix so that they all are independently controlled the same way?

Explicitly mark which is default on @vszakats request.
@wyattoday
Copy link
Contributor Author

wyattoday commented Jul 26, 2023

--disable-basic-auth
--disable-bearer-auth
--disable-digest-auth
--disable-aws

Shouldn't this then also include ntlm and negotiate to the mix so that they all are independently controlled the same way?

CURL_DISABLE_NTLM already exists, but you're right, I've added CURL_DISABLE_NEGOTIATE_AUTH.

@MarcelRaad
Copy link
Member

MarcelRaad commented Jul 27, 2023

Would it make sense to change CURL_DISABLE_CRYPTO_AUTH to only set these other defines and remove all checks for CURL_DISABLE_CRYPTO_AUTH?

Also, CMake is missing and the spellcheck job is failing. Apart from that, this PR looks good to me!

configure.ac Outdated Show resolved Hide resolved
docs/CURL-DISABLE.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the tests label Sep 5, 2023
@wyattoday wyattoday changed the title Ability to disable auths (Basic, Bearer, and Digest) and non-standard like AWS. Ability to disable specific auths (Basic, Bearer, Digest, etc.) and non-standard like AWS. Sep 6, 2023
@wyattoday
Copy link
Contributor Author

@MarcelRaad

Also, CMake is missing and the spellcheck job is failing. Apart from that, this PR looks good to me!

Fixed the spelling and added the CMake changes.

@bagder

I'm all done making the requested changes. All the tests are passing (except for some failures due to the brittleness of the tests, not the code being tested).

Should be good for a final review and/or merge.

Let me know if there's anything else you want me to address.

@bagder bagder closed this in e92edfb Sep 7, 2023
@bagder
Copy link
Member

bagder commented Sep 7, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants