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.

Sign up for GitHub

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

mbedtls: Update mbedTLS 2.16.2 #2

Merged
merged 2 commits into from
Aug 28, 2019
Merged

mbedtls: Update mbedTLS 2.16.2 #2

merged 2 commits into from
Aug 28, 2019

Conversation

ioannisg
Copy link
Member

Bump mbedTLS version to 2.16.2.

Origin: ARMmbed/mbedTLS
License: Apache-2.0
URL: https://github.com/ARMmbed/mbedtls/releases/tag/mbedtls-2.16.2
commit: d81c11b8ab61fd5b2da8133aa73c5fe33a0633eb

Signed-off-by: Ioannis Glaropoulos Ioannis.Glaropoulos@nordicsemi.no

@ioannisg ioannisg marked this pull request as ready for review August 10, 2019 18:09
@ioannisg ioannisg requested review from galak and MaureenHelm August 10, 2019 18:09
d3zd3z
d3zd3z approved these changes Aug 10, 2019
View reviewed changes
Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Assuming that this does match the upstream code, this looks good. We probably want to make sure Zephyr runs through CI with this version to catch any unintended breakage.

@ioannisg
mbedtls: Update mbedTLS 2.16.2
c7ce45f 
Bump mbedTLS version to 2.16.2.

Origin: ARMmbed/mbedTLS
License: Apache-2.0
URL: https://github.com/ARMmbed/mbedtls/releases/tag/mbedtls-2.16.2
commit: d81c11b8ab61fd5b2da8133aa73c5fe33a0633eb

Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>
@ioannisg
Copy link
Member Author

Assuming that this does match the upstream code, this looks good. We probably want to make sure Zephyr runs through CI with this version to catch any unintended breakage.

@d3zd3z yes, this matches the upstream code. I did the following

  • copy pasted Changelog
  • updated README to match the new release version
  • copy pasted include/mbedtls and /library folders

One thing I had to do, also, was to maintain the modification we describe in README:

One change was applied in the original code. In mbedTLS the file
net_sockets.c was defining _POSIX_C_SOURCE and as Zephyr build all files
together this was raising and warning. In order to fix this problem one
define guard was added, as showed bellow:

#if defined(_POSIX_C_SOURCE)
#undef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 200112L
#endif

@ioannisg
Copy link
Member Author

@d3zd3z you can see the related Zephyr PR, with the updated mbedtls here: zephyrproject-rtos/zephyr#18172

ioannisg
ioannisg commented Aug 10, 2019
View reviewed changes
library/x509_crt.c Outdated Show resolved Hide resolved
@ioannisg ioannisg mentioned this pull request Aug 14, 2019
Merged
aescolar
aescolar reviewed Aug 14, 2019
View reviewed changes
Copy link
Member

@aescolar aescolar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See Mbed-TLS/mbedtls#2392
And 3437a1b
The way you are fixing the uninitialized variable warning was not accepted, but a different proposal was chosen, which was then applied to Zephyr's mbedtls ahead of it being merged in the real mbedtls.
But for some reason that PR did not get enough attention to get merged upstream.

In this merge you seem to be reverting that local fix.

aescolar
aescolar previously requested changes Aug 14, 2019
View reviewed changes
library/x509_crt.c Outdated Show resolved Hide resolved
@pfalcon
Copy link
Collaborator

pfalcon commented Aug 15, 2019

See Mbed-TLS/mbedtls#2392

I'm going to try and pick that up.

@aescolar
Copy link
Member

@pfalcon Thanks. Are you able to push to Andy's branch to take the PR from where he left it?

@pfalcon
Copy link
Collaborator

pfalcon commented Aug 15, 2019

Are you able to push to Andy's branch to take the PR from where he left it?

I doubt that. If you have specific instructions how to achieve that, feel free to share. (It always amazed me that github allows in some cases to do that. That has questionable implications from security to copyright/impersonation.)

@ioannisg
ext: crypto: mbedtls: Fix uninitialized variable
044b383 
This patch fixes an issue with an uninitialized variable in the x509
mbedtls feature.  I sent a related patch to the mbedtls project so
that this can be fixed in the future.

Mbed-TLS/mbedtls#2392

Signed-off-by: Andy Gross <andy.gross@linaro.org>
@ioannisg ioannisg requested a review from aescolar August 28, 2019 09:10
@ioannisg
Copy link
Member Author

@pfalcon @aescolar brought back the patch by @agross-oss (i.e. what was added on top of the 2.16.0 bump), instead of my fix. I'll run, now, a CI on zephyr using the current PR tip.

@aescolar aescolar dismissed their stale review August 28, 2019 10:22

addressed

pfalcon
pfalcon reviewed Aug 28, 2019
View reviewed changes
include/mbedtls/ssl.h
* \note With TLS, this currently only affects ApplicationData (sent
* with \c mbedtls_ssl_read()), not handshake messages.
* With DTLS, this affects both ApplicationData and handshake.
* \note On the client side, the maximum fragment length extension
Copy link
Collaborator

Choose a reason for hiding this comment

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

@rlubos: Worth reading this change. I'm not sure if we use max fragment length ext anywhere, but if we do, looks like we'll need to call extra code now.

pfalcon
pfalcon approved these changes Aug 28, 2019
View reviewed changes
Copy link
Collaborator

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

Ok, eyeballed the changes. Let me approve now, but I would hope to actually build and test it later today.

@nashif nashif merged commit 4bba3b8 into zephyrproject-rtos:master Aug 28, 2019
@pfalcon
Copy link
Collaborator

pfalcon commented Aug 28, 2019

Ok, I tested new version using sockets/big_http_download sample with overlay-tls.conf (downloading https://www.7-zip.org:443/a/7z1805.exe). Everything's ok, downloaded 24MB in total (few repetitions).

@pfalcon
Copy link
Collaborator

pfalcon commented Sep 6, 2019

See Mbed-TLS/mbedtls#2392

I'm going to try and pick that up.

Ok, so revamped version of that patch was merged to both 2.16 and 2.18 mbedTLS branches: Mbed-TLS/mbedtls#2795, Mbed-TLS/mbedtls#2813, so we'll pick it up on next update.

SebastianBoe pushed a commit to SebastianBoe/mbedtls that referenced this pull request Dec 22, 2021
@minosgalanakis @davidhorstmann-arm
readme: Addressed review comments zephyrproject-rtos#2
0f2a46c 
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Co-authored-by: davidhorstmann-arm <70948878+davidhorstmann-arm@users.noreply.github.com>
SebastianBoe pushed a commit to SebastianBoe/mbedtls that referenced this pull request Dec 22, 2021
@minosgalanakis
changelog: Addressed review comments zephyrproject-rtos#2
3a77949 
Signed-off-by: Minos Galanakis <minos.galanakis@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3zd3z d3zd3z d3zd3z approved these changes

@nashif nashif nashif approved these changes

@pfalcon pfalcon pfalcon approved these changes

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@aescolar aescolar Awaiting requested review from aescolar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ioannisg @pfalcon @aescolar @d3zd3z @nashif