Mbedtls support #512

ycyang1229 · 2020-11-17T14:08:00Z

Recently focusing on the implementation of webrtc on rtos-based embedded devices, so adding the support of mbedtls. Hope this can help someone using this lib with mbedtls. thanks.

pull the latest commits from the origin.

fluffy · 2020-12-27T16:00:29Z

I'd like to add the MBed support but I want to make sure we get a few people to review this carefully.

ycyang1229 · 2020-12-29T09:00:06Z

I'd like to add the MBed support but I want to make sure we get a few people to review this carefully.

Thanks a lot. It is nice to have someone to review this work.

minimal build to ensure some thing works

crypto/hash/hmac_mbedtls.c

pabuhler

As far as I can tell this is ok. I have done testing and it works on Ubuntu for me. The majority of the change is the same as the openssl/nss integration.
As a follow up note we should look at converting more tests to the cmake build, supporting vagrind in travis for cmake and finally move the test vectors in to a common file that can be shared between back end implementations.

ycyang1229 · 2021-01-05T02:24:14Z

As far as I can tell this is ok. I have done testing and it works on Ubuntu for me. The majority of the change is the same as the openssl/nss integration.
As a follow up note we should look at converting more tests to the cmake build, supporting vagrind in travis for cmake and finally move the test vectors in to a common file that can be shared between back end implementations.

Cool, if anything I can help, please let me know. big thanks.

bifurcation

Here are a few comments from me. I'm not an mBedTLS expert, though, so I have asked @hannestschofenig to review. He tells me he'll take a look tomorrow.

bifurcation · 2021-01-05T18:47:18Z

CMakeLists.txt

@@ -4,7 +4,7 @@ project(libsrtp2 LANGUAGES C)

 set(PACKAGE_VERSION 2.4.0)
 set(PACKAGE_STRING "${CMAKE_PROJECT_NAME} ${PACKAGE_VERSION}")
-
+set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR};${CMAKE_MODULE_PATH}")


This seems unfortunate. What module are we finding from within the source tree?

I added this for looking for FindMbedTLS.cmake, and it will be used for searching mbedtls related header files and libs. As I remember, I need to use such a cmake files to search mbedtls related stuff. Did I mistake it?

bifurcation · 2021-01-05T18:48:14Z

CMakeLists.txt

+
+if(ENABLE_MBEDTLS)
+  find_package(MbedTLS REQUIRED)
+  include_directories(${MBEDTLS_INCLUDE_DIRS})


I believe that the target_link_libraries directive below will do this automatically.

The same reason as the above stuff, FindMbedTLS.cmake.

bifurcation · 2021-01-05T22:20:50Z

config_in_cmake.h

@@ -21,6 +21,9 @@
 /* Define this to use AES-GCM. */
 #cmakedefine GCM 1

+/* Define this to use MBEDTLS. */
+#cmakedefine MBEDTLS 1


I would reorder these to put MBEDTLS just after OPENSSL.

Sure, I will change the order.

bifurcation · 2021-01-05T22:24:10Z

crypto/hash/hmac_mbedtls.c

+    return srtp_err_status_ok;
+}
+
+/* begin test case 0 */


It seems like the test cases should be common across the various implementations. The test data should hash to the same thing regardless of whether you're using OpenSSL, mBedTLS, etc. Suggest factoring the tests out to a different file. I haven't looked at the encryption code, but I suspect the same comments apply. Perhaps we could just make a crypto_test_cases.c file that contains all of them. Then the individual methods can just point to the one that makes sense.

This idea came up when I was coding. But I saw the code inside openssl, I left it as the same as the part of openssl. Do you want me add this part, or cut another pr?

I have a local change that does this now, I will create a PR after this is merged.

bifurcation · 2021-01-05T22:41:49Z

crypto/include/cipher_types.h

@@ -74,6 +74,9 @@ extern srtp_debug_module_t srtp_mod_aes_icm;
 #ifdef OPENSSL
 extern srtp_debug_module_t srtp_mod_aes_gcm;
 #endif
+#ifdef MBEDTLS


This is getting wordy (as opposed to just doing #if defined(OPENSSL) || defined(MBEDTLS) || defined(NSS)). And don't we have a GCM variable anyway? Seems like we should use #ifdef GCM, or if that doesn't work, the #if defined || defined || defined as above.

If you make this change then great other wise we can do it in a separate clean up PR.

I did not go through the nss part in detail, and see the nss in the cmakefiles either. So I am not sure it includes the gcm or not. Went through the nss part quickly, and it should have the gcm part. I changed it as the second choice #if (defined(OPENSSL) || defined(MBEDTLS) || defined(NSS)).

hannestschofenig · 2021-01-06T09:10:33Z

Thanks for adding Mbed TLS support to libsrtp!

I took a look at the code, as requested by Richard. The Mbed TLS related code looks good to me. I cannot comment on the requirements for tests, documentation and coding style.

(Note that we have been working on a new API, called the PSA Crypto API, which allows hardware crypto to be used conveniently. I understand that you are using our current API instead but in the future there may be benefit to also take a look at the new API. You can find the details of the PSA Crypto API at https://armmbed.github.io/mbed-crypto/PSA_Cryptography_API_Specification.pdf and I have placed examples at Mbed-TLS/mbedtls#3833)

Just for my understanding: Are you expecting this code to be used in combination with the Mbed TLS code (with MBEDTLS_SSL_DTLS_SRTP enabled) so that the key exchange is done based on the code in https://github.com/ARMmbed/mbedtls/ and then for SRTP you are using the code from this library?

ycyang1229 · 2021-01-08T10:50:21Z

Thanks for adding Mbed TLS support to libsrtp!

I took a look at the code, as requested by Richard. The Mbed TLS related code looks good to me. I cannot comment on the requirements for tests, documentation and coding style.

(Note that we have been working on a new API, called the PSA Crypto API, which allows hardware crypto to be used conveniently. I understand that you are using our current API instead but in the future there may be benefit to also take a look at the new API. You can find the details of the PSA Crypto API at https://armmbed.github.io/mbed-crypto/PSA_Cryptography_API_Specification.pdf and I have placed examples at ARMmbed/mbedtls#3833)

Just for my understanding: Are you expecting this code to be used in combination with the Mbed TLS code (with MBEDTLS_SSL_DTLS_SRTP enabled) so that the key exchange is done based on the code in https://github.com/ARMmbed/mbedtls/ and then for SRTP you are using the code from this library?

Sure, I would love to take a look, and it is my plesaure. Yes, I took this pr(https://github.com/ARMmbed/mbedtls/pull/3235/files) in the beginning. Big thanks.

pabuhler · 2021-01-08T11:36:04Z

@ycyang1229 the failed build is related to some meson issues in travis that started yesterday, so I think you just need to ignore that for now.

ycyang1229 · 2021-01-08T11:44:05Z

@ycyang1229 the failed build is related to some meson issues in travis that started yesterday, so I think you just need to ignore that for now.

Thanks for immediate response. I just tried to see why. :D

bifurcation

Thanks for the review, @hannestschofenig. I filed #517 to track the PSA upgrade.

Given the responses on my other comments, this looks good to merge once CI passes.

merging from master first to get ci fix

ycyang1229 added 3 commits November 17, 2020 21:05

Merge pull request #1 from cisco/master

27a2b0d

pull the latest commits from the origin.

add the mbedtls support.

268eb50

disable mbedtls.

1e8aaa7

fluffy requested review from pabuhler and bifurcation and removed request for pabuhler December 27, 2020 16:00

add a mbedtls build to travis

53e0746

minimal build to ensure some thing works

pabuhler reviewed Jan 3, 2021

View reviewed changes

crypto/hash/hmac_mbedtls.c Outdated Show resolved Hide resolved

pabuhler approved these changes Jan 3, 2021

View reviewed changes

use angle brackets for consistency.

5db3eed

bifurcation reviewed Jan 5, 2021

View reviewed changes

paulej approved these changes Jan 5, 2021

View reviewed changes

ycyang1229 added 2 commits January 8, 2021 18:32

change the order of mbedtls.

1c9b6bc

remove the wordy define.

c134959

remove unnecessary parentheses.

ad783d3

bifurcation mentioned this pull request Jan 8, 2021

Upgrade to Mbed TLS PSA API #517

Open

bifurcation approved these changes Jan 8, 2021

View reviewed changes

Merge branch 'master' into mbedtls-support

d7243eb

merging from master first to get ci fix

pabuhler merged commit dd89597 into cisco:master Jan 8, 2021

ycyang1229 mentioned this pull request Jun 2, 2021

[FEATURE] the mbedtls support for srtp awslabs/amazon-kinesis-video-streams-webrtc-sdk-c#1159

Closed

iameli mentioned this pull request Dec 5, 2023

meson.build: implement mbedtls support #660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mbedtls support #512

Mbedtls support #512

ycyang1229 commented Nov 17, 2020

fluffy commented Dec 27, 2020

ycyang1229 commented Dec 29, 2020

pabuhler left a comment

ycyang1229 commented Jan 5, 2021

bifurcation left a comment

bifurcation Jan 5, 2021

ycyang1229 Jan 6, 2021 •

edited

Loading

bifurcation Jan 5, 2021

ycyang1229 Jan 6, 2021

bifurcation Jan 5, 2021

ycyang1229 Jan 6, 2021

bifurcation Jan 5, 2021

ycyang1229 Jan 6, 2021 •

edited

Loading

pabuhler Jan 6, 2021

bifurcation Jan 5, 2021

pabuhler Jan 6, 2021

ycyang1229 Jan 8, 2021

hannestschofenig commented Jan 6, 2021

ycyang1229 commented Jan 8, 2021

pabuhler commented Jan 8, 2021

ycyang1229 commented Jan 8, 2021

bifurcation left a comment •

edited

Loading

Mbedtls support #512

Mbedtls support #512

Conversation

ycyang1229 commented Nov 17, 2020

fluffy commented Dec 27, 2020

ycyang1229 commented Dec 29, 2020

pabuhler left a comment

Choose a reason for hiding this comment

ycyang1229 commented Jan 5, 2021

bifurcation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycyang1229 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycyang1229 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannestschofenig commented Jan 6, 2021

ycyang1229 commented Jan 8, 2021

pabuhler commented Jan 8, 2021

ycyang1229 commented Jan 8, 2021

bifurcation left a comment • edited Loading

Choose a reason for hiding this comment

ycyang1229 Jan 6, 2021 •

edited

Loading

ycyang1229 Jan 6, 2021 •

edited

Loading

bifurcation left a comment •

edited

Loading