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

Only try to call mbedtls_md4 if it's enabled #1004

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tkelman
Contributor

tkelman commented Sep 9, 2016

otherwise this is an undefined reference in the default build of mbedtls
https://github.com/ARMmbed/mbedtls/blob/mbedtls-2.3.0/include/mbedtls/config.h#L1901-L1911

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 9, 2016

Member

But doesn't the lack of this function then completely break NTLM? Ie shouldn't the lack of the function instead rather prevent USE_NTLM to be defined?

Member

bagder commented Sep 9, 2016

But doesn't the lack of this function then completely break NTLM? Ie shouldn't the lack of the function instead rather prevent USE_NTLM to be defined?

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 9, 2016

Contributor

I could see that being the case - would the test suite here flag that? Maybe in curl_setup.h we could do something like

/* Single point where USE_NTLM definition might be defined */
#if !defined(CURL_DISABLE_NTLM) && !defined(CURL_DISABLE_CRYPTO_AUTH)
#if defined(USE_OPENSSL) || defined(USE_WINDOWS_SSPI) || \
    defined(USE_GNUTLS) || defined(USE_NSS) || \
    defined(USE_DARWINSSL) || defined(USE_OS400CRYPTO) || \
    defined(USE_WIN32_CRYPTO)

#define USE_NTLM

#elif defined(USE_MBEDTLS)
#  include <mbedtls/md4.h>
#  if defined(MBEDTLS_MD4_C)
#define USE_NTLM
#  endif

#endif
#endif

or is that way too messy? Maybe add a configure probe specifically for this feature in mbedtls detection instead?

Contributor

tkelman commented Sep 9, 2016

I could see that being the case - would the test suite here flag that? Maybe in curl_setup.h we could do something like

/* Single point where USE_NTLM definition might be defined */
#if !defined(CURL_DISABLE_NTLM) && !defined(CURL_DISABLE_CRYPTO_AUTH)
#if defined(USE_OPENSSL) || defined(USE_WINDOWS_SSPI) || \
    defined(USE_GNUTLS) || defined(USE_NSS) || \
    defined(USE_DARWINSSL) || defined(USE_OS400CRYPTO) || \
    defined(USE_WIN32_CRYPTO)

#define USE_NTLM

#elif defined(USE_MBEDTLS)
#  include <mbedtls/md4.h>
#  if defined(MBEDTLS_MD4_C)
#define USE_NTLM
#  endif

#endif
#endif

or is that way too messy? Maybe add a configure probe specifically for this feature in mbedtls detection instead?

@tkelman tkelman referenced this pull request Sep 9, 2016

Merged

RFC: upgrade various deps #18376

tkelman added a commit to JuliaLang/julia that referenced this pull request Sep 10, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 11, 2016

Member

It is messy yes, but I'm not sure that adding it to configure will be less messy and doing that will then push the problem to get fixed in the windows and cmake builds independently.

For me, this is also a question if we should support this at all in the curl configure. There are a bazillion ways people can build bastardized versions of the libraries curl can use, and there's just no way we can make sure that curl works with all those crazy variations and combinations. Is this a common enough build setup that we should detect and work around it by default? Can you help us a bit by explaining to us why you build and use mbedTLS like this?

You can also work around this issue by defining CURL_DISABLE_NTLM in the curl build, right?

Member

bagder commented Sep 11, 2016

It is messy yes, but I'm not sure that adding it to configure will be less messy and doing that will then push the problem to get fixed in the windows and cmake builds independently.

For me, this is also a question if we should support this at all in the curl configure. There are a bazillion ways people can build bastardized versions of the libraries curl can use, and there's just no way we can make sure that curl works with all those crazy variations and combinations. Is this a common enough build setup that we should detect and work around it by default? Can you help us a bit by explaining to us why you build and use mbedTLS like this?

You can also work around this issue by defining CURL_DISABLE_NTLM in the curl build, right?

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 11, 2016

Contributor

I build and use mbedtls in a pretty standard configuration. This function is not built by default in mbedtls, you have to ask for it.

I'll try disabling ntlm, but that should probably be the default setting if this code is going to stay here when you use mbedtls. Should probably assume people are using something close to a default build of dependency libraries, at least with the default way of using them here.

Contributor

tkelman commented Sep 11, 2016

I build and use mbedtls in a pretty standard configuration. This function is not built by default in mbedtls, you have to ask for it.

I'll try disabling ntlm, but that should probably be the default setting if this code is going to stay here when you use mbedtls. Should probably assume people are using something close to a default build of dependency libraries, at least with the default way of using them here.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 11, 2016

Member

Ok, I hear you. That was not the case when I last tried a mbedtls build so something has changed since then. That is a bit annoying for curl users since it'll make it harder to build curl with NTLM support with mbedTLS. As this is the default build, I agree that we should deal with it properly so that it builds fine. Back to figuring out the least messy way...

Member

bagder commented Sep 11, 2016

Ok, I hear you. That was not the case when I last tried a mbedtls build so something has changed since then. That is a bit annoying for curl users since it'll make it harder to build curl with NTLM support with mbedTLS. As this is the default build, I agree that we should deal with it properly so that it builds fine. Back to figuring out the least messy way...

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 11, 2016

Contributor

must've been a while ago? ARMmbed/mbedtls@6506aff

A purely define based method might not be as robust as something in the build system that checks for function existence, since I believe you could enable md4 in mbedtls via a -D compiler flag instead of modifying the header.

Contributor

tkelman commented Sep 11, 2016

must've been a while ago? ARMmbed/mbedtls@6506aff

A purely define based method might not be as robust as something in the build system that checks for function existence, since I believe you could enable md4 in mbedtls via a -D compiler flag instead of modifying the header.

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 11, 2016

Contributor

oh but it would still be an undefined reference when used here if you didn't also specify the -D when building curl too. so maybe what I had above (the edited version) would work after all.

Contributor

tkelman commented Sep 11, 2016

oh but it would still be an undefined reference when used here if you didn't also specify the -D when building curl too. so maybe what I had above (the edited version) would work after all.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 11, 2016

Member

Now I remember, the curl build with mbedTLS never had NTLM at all until commit 497e7c9, which is fairly recent.

Member

bagder commented Sep 11, 2016

Now I remember, the curl build with mbedTLS never had NTLM at all until commit 497e7c9, which is fairly recent.

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 11, 2016

Contributor

right, I only encountered a problem when I tried to upgrade from 7.50.1 to 7.50.2. can we cc the author of that commit here to comment? I don't see a github id associated with him.

Contributor

tkelman commented Sep 11, 2016

right, I only encountered a problem when I tried to upgrade from 7.50.1 to 7.50.2. can we cc the author of that commit here to comment? I don't see a github id associated with him.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 12, 2016

Member

Yeah, let's do that. But with the check that avoids NTLM completely without the md4 function present, surely we can skip the first check?

Member

bagder commented Sep 12, 2016

Yeah, let's do that. But with the check that avoids NTLM completely without the md4 function present, surely we can skip the first check?

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 12, 2016

Contributor

If you get NTLM from something lower in the list like NSS, and also have mbedtls but without md4, couldn't that be a problem? or is that not possible?

Contributor

tkelman commented Sep 12, 2016

If you get NTLM from something lower in the list like NSS, and also have mbedtls but without md4, couldn't that be a problem? or is that not possible?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 12, 2016

Member

No that's not possible. You only ever have one of those crypto/TLS libraries.

Member

bagder commented Sep 12, 2016

No that's not possible. You only ever have one of those crypto/TLS libraries.

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 12, 2016

Contributor

ok, rebased out the first commit

Contributor

tkelman commented Sep 12, 2016

ok, rebased out the first commit

@bagder bagder closed this in 6656949 Sep 12, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Sep 12, 2016

Member

Excellent. Merged now. Thanks a lot for your help!

Member

bagder commented Sep 12, 2016

Excellent. Merged now. Thanks a lot for your help!

@tkelman tkelman deleted the tkelman:patch-1 branch Sep 12, 2016

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Sep 12, 2016

Contributor

Thank you for the prompt review and merge. Hope we can get some related PR's regarding mbedtls support reviewed and merged elsewhere.

Contributor

tkelman commented Sep 12, 2016

Thank you for the prompt review and merge. Hope we can get some related PR's regarding mbedtls support reviewed and merged elsewhere.

tkelman added a commit to JuliaLang/julia that referenced this pull request Sep 13, 2016

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