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

Initial mbedTLS 3.0.0 support #7428

Closed
wants to merge 1 commit into from
Closed

Initial mbedTLS 3.0.0 support #7428

wants to merge 1 commit into from

Conversation

Benau
Copy link
Contributor

@Benau Benau commented Jul 18, 2021

Tested in android and windows and works

Notice: I'm not sure if I've done the tls handling correctly as newest mbedtls 3 drops tls 1.1, 1.2 supports, comments welcome....

And I've not heard from mbedtls developer about prettier access to private member of some structures, so I have to use MBEDTLS_PRIVATE macros...

see #7376 (comment)

@Benau Benau marked this pull request as draft July 18, 2021 06:11
@Benau Benau marked this pull request as ready for review July 18, 2021 06:35
@bagder bagder added the TLS label Jul 18, 2021
@bagder
Copy link
Member

bagder commented Jul 18, 2021

It seems this breaks the build of the older mbedtls version?

@Benau
Copy link
Contributor Author

Benau commented Jul 18, 2021

ok sha256.c has an inconsistent HAS_RESULT_CODE_BASED_FUNCTIONS definition at the top in the past vs HAS_MBEDTLS_RESULT_CODE_BASED_FUNCTIONS (rest of the files), I assume you don't mind me fix that there....

Copy link
Member

@danielgustafsson danielgustafsson left a comment

Choose a reason for hiding this comment

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

I would not be comfortable in merging this unless we can an mbedTLS 3.0 job in the CI, are there stable packages around to make that happen?

lib/md4.c Outdated
#include <mbedtls/version.h>
#if MBEDTLS_VERSION_MAJOR < 3
Copy link
Member

Choose a reason for hiding this comment

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

Since the change that altered the header includes was introduced in 3, the check should be reversed and include the new file iff we are compiling against 3 and not the other way around. Nitpick to some extent, but it will improve readability.

Also, why doesn't this (and the remaining changes) use MBEDTLS_VERSION_NUMBER as the rest if the code does?

if(mbedtls_x509_crt_parse_der(p,
peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(p),
peercert->MBEDTLS_PRIVATE(raw).MBEDTLS_PRIVATE(len))) {
#else
Copy link
Member

Choose a reason for hiding this comment

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

This might be a dumb question, but I don't know mbedtls at all. Does this mean that the fields we need are now private but were public in the past, or have they always been private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

quote from mbedtls developer:
Fields in structures are now private by default. We're aware that we may have been overeager in some places, and we're likely to make a few fields public again in 3.x minor releases, or to add new accessor functions.
#7376 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

That sounds really scary, the fact that a member is private means that it can go away or radically change at any time. Relying on that not happening seems pretty terrible, but that's again not the fault of this patch.

#if MBEDTLS_VERSION_MAJOR >= 3
case CURL_SSLVERSION_TLSv1_0:
case CURL_SSLVERSION_TLSv1_1:
#else
Copy link
Member

Choose a reason for hiding this comment

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

This makes the code too hard to parse. Since the TLS support is so vastly different, I think we should provide two different implementations of mbedtls_version_from_curl entirely and do the version preprocessing around those rather than pick this one apart.

if(mbedtls_sha256_ret(input, inputlen, sha256sum, 0) != 0)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we now use the same function name in 3 that we used in 2.0.x which was deprecated in favour of _ret in 2.x needs a pretty big comment explaining how that mess is strung together.

Also, while not a fault of this PR, blimey thats ugly..

@bagder
Copy link
Member

bagder commented Jul 18, 2021

I would not be comfortable in merging this unless we can an mbedTLS 3.0 job in the CI, are there stable packages around to make that happen?

I presume we can always get it from git and build our own version. But yes, I too would like to have that in the CI when we merge v3 support.

@Benau
Copy link
Contributor Author

Benau commented Jul 19, 2021

I made the requested changes now by the way

except i'm not a good documentation writter for "a pretty big comment explaining how that mess is strung together"...

@@ -43,7 +43,7 @@
#include <mbedtls/version.h>

#if(MBEDTLS_VERSION_NUMBER >= 0x02070000)
#define HAS_RESULT_CODE_BASED_FUNCTIONS
#define HAS_MBEDTLS_RESULT_CODE_BASED_FUNCTIONS
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the code be easier if this code block instead was made like?

#if(MBEDTLS_VERSION_NUMBER >= 0x02070000) && \
   (MBEDTLS_VERSION_NUMBER < 0x03000000)
  #define HAS_MBEDTLS_RESULT_CODE_BASED_FUNCTIONS
#endif

@danielgustafsson
Copy link
Member

except i'm not a good documentation writter for "a pretty big comment explaining how that mess is strung together"...

That's fine, we're all in this together and can collaborate. I would recommend adding a TODO: comment which states that this would need to get done, so that we don't forget before it goes in.

@Benau
Copy link
Contributor Author

Benau commented Jul 22, 2021

done

Copy link
Contributor

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

Some drive-by-thoughts. Feel free to ignore.

I'd be happy to offer comments on the TODO text, but it doesn't make enough sense to me to comment at this time.

lib/md4.c Outdated

#if(MBEDTLS_VERSION_NUMBER >= 0x02070000)
#define HAS_MBEDTLS_RESULT_CODE_BASED_FUNCTIONS
#endif

Copy link
Contributor

Choose a reason for hiding this comment

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

This blank line seems odd... (compare lin 39/40 of lib/md5.c below)

(void)sha256len;
#if MBEDTLS_VERSION_NUMBER < 0x02070000
mbedtls_sha256(input, inputlen, sha256sum, 0);
#else
/* returns 0 on success, otherwise failure */
#if MBEDTLS_VERSION_NUMBER >= 0x03000000
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, wouldn't the preference be to put this before the MBEDTLS_VERSION_NUMBER < 0x02070000?

@@ -36,12 +36,17 @@
#endif /* USE_OPENSSL */

#ifdef USE_MBEDTLS
Copy link
Contributor

Choose a reason for hiding this comment

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

note to self: this is an #ifdef

@@ -105,8 +106,8 @@ typedef mbedtls_sha256_context SHA256_CTX;

static void SHA256_Init(SHA256_CTX *ctx)
{
#if !defined(HAS_RESULT_CODE_BASED_FUNCTIONS)
mbedtls_sha256_starts(ctx, 0);
#if !defined(HAS_MBEDTLS_RESULT_CODE_BASED_FUNCTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a style guide for #if !defined... vs #ifndef?

https://github.com/curl/curl/search?q=ifndef it looks like both flavors are used...

offhand, it looks like #if !defined... tends to be used with a boolean operator for when one is performing multiple checks concurrently.
https://github.com/curl/curl/search?p=2&q=%22%23if+%21defined%22

(Yes, I understand this is preexisting code.)

Copy link
Member

Choose a reason for hiding this comment

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

Is there a style guide for #if !defined... vs #ifndef?

We prefer #ifdef and #ifndef if you only check a single define. If you check more than one, then #if needs to be used.

@bagder
Copy link
Member

bagder commented Jul 30, 2021

Can you please squash the commits, rebase and force-push to make this easier to review? Then I figure we only need a CI job added for mbedTLS. I might take a stab at that.

@Benau
Copy link
Contributor Author

Benau commented Jul 30, 2021

Done

@wyattoday
Copy link
Contributor

Any chance of getting this merged soon? This fixes #7385, and looks about how we solved most of the problems before we ran out of time.

@bagder
Copy link
Member

bagder commented Aug 8, 2021

Once we have a CI build setup that verifies this build I'll be ready to merge.

@bagder
Copy link
Member

bagder commented Aug 9, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants