Conversation
pull the latest commits from the origin.
|
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
pabuhler
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| 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}") |
There was a problem hiding this comment.
This seems unfortunate. What module are we finding from within the source tree?
There was a problem hiding this comment.
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?
|
|
||
| if(ENABLE_MBEDTLS) | ||
| find_package(MbedTLS REQUIRED) | ||
| include_directories(${MBEDTLS_INCLUDE_DIRS}) |
There was a problem hiding this comment.
I believe that the target_link_libraries directive below will do this automatically.
There was a problem hiding this comment.
The same reason as the above stuff, FindMbedTLS.cmake.
config_in_cmake.h
Outdated
| #cmakedefine GCM 1 | ||
|
|
||
| /* Define this to use MBEDTLS. */ | ||
| #cmakedefine MBEDTLS 1 |
There was a problem hiding this comment.
I would reorder these to put MBEDTLS just after OPENSSL.
There was a problem hiding this comment.
Sure, I will change the order.
| return srtp_err_status_ok; | ||
| } | ||
|
|
||
| /* begin test case 0 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I have a local change that does this now, I will create a PR after this is merged.
crypto/include/cipher_types.h
Outdated
| #ifdef OPENSSL | ||
| extern srtp_debug_module_t srtp_mod_aes_gcm; | ||
| #endif | ||
| #ifdef MBEDTLS |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If you make this change then great other wise we can do it in a separate clean up PR.
There was a problem hiding this comment.
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)).
|
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? |
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. |
|
@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 |
There was a problem hiding this comment.
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
Mbedtls support
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.