search

Synss / python-mbedtls

Cryptographic library with an mbed TLS back end
MIT License
79 stars 28 forks source link

Add support for EC J-PAKE key exchange feature #81

Closed dfrancis closed 1 year ago

dfrancis commented 1 year ago

The PR fulfills these requirements

More details in CONTRIBUTING.

I am submitting a …

Description

This change adds an ecjpake module to perform EC J-PAKE key exchange. Note that the mbed TLS library does not enable the ECJPAKE feature API by default.

Other information

I believe I've added proper runtime checks so python-mbedtls can still be used either with or without the ECJPAKE feature enabled. The tests are skipped if the ECJPAKE feature is unavailable in the mbedtlscrypto shared library.

To ensure the ecjpake module tests pass will of course require a version of mbed TLS to be built with the proper feature configuration. I have not attempted to update build Action scripts to do this (yet). I expect this additional built/test step will need to be enabled before this pull require can be approved. As a step towards that please see my next comment below.

The changes required in the mbed TLS backend to enable the ECJPAKE feature are as follows:

diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h
index 61db79362..cc9fe73ba 100644
--- a/include/mbedtls/config.h
+++ b/include/mbedtls/config.h
@@ -1183,7 +1183,7 @@
  * enabled as well):
  *      MBEDTLS_TLS_ECJPAKE_WITH_AES_128_CCM_8
  */
-//#define MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED
+#define MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED

 /**
  * \def MBEDTLS_PK_PARSE_EC_EXTENDED
@@ -2816,7 +2816,7 @@
  *
  * Requires: MBEDTLS_ECP_C, MBEDTLS_MD_C
  */
-//#define MBEDTLS_ECJPAKE_C
+#define MBEDTLS_ECJPAKE_C

 /**
  * \def MBEDTLS_ECP_C
coveralls commented 1 year ago

Coverage Status

Coverage: 88.999% (-0.02%) from 89.022% when pulling 03e9780f3227ed712d5094f7233784ef275b1870 on dfrancis:dfrancis/ecjpake into 658a9c16e0b2218efa768cb401fc662f08ab423e on Synss:master.

Synss commented 1 year ago

Thank you for your contribution. I'm a bit short of time these days but I'll have a look in your patches and red builds.

You can change the options in config.h without modifying the file in the same way ARIA gets added to the wheels: https://github.com/Synss/python-mbedtls/blob/master/scripts/install-mbedtls.sh#L26 so in your case, probably (untested), with an extra -DMBEDTLS_ECJPAKE_C=ON -D MBEDTLS_KEY_EXCHANGE_ECJPAKE_ENABLED=ON in the make lib call.

At some point, I guess the tests should run twice: once without any options added to the library and one with ARIA and EC-J-PAKE. Currently, I think it's OK if you add the option to install-mbedtls.sh and always let the tests run with EC-J-PAKE.

A last point, I'm trying to move to mbedtls-3 as the backend. I haven't checked the status of EC-J-PAKE upstream. If ever it was dropped, I won't be able to accept the patch because I would need to drop it whenever I'm ready for mbedtls-3.

Synss commented 1 year ago

Do you know where the build errors come from? It looks like missing requirements but I don't see these errors in other runs. Also, in the present case, I'd prefer if you'd just squash all the commits and force pushed.

I double checked and EC-J-PAKE is still there in mbedtls 3 so there is not problem here.

I'll review the code when the tests are green unless you need help somewhere.

dfrancis commented 1 year ago

Thank you for the feedback and direction. I squashed commits and force-pushed. I'll repeat that after making further changes to keep things as a single commit. As for the build failures, I'll try to investigate but I might need help. I see this failure: https://github.com/Synss/python-mbedtls/actions/runs/4852172732/jobs/8646831666 I'm curious to know if this failure still occurs. I opened a second PR to help determine if the same failure occurs only minimal code changes. If the problem is due to pre-commit/action then perhaps we could try its replacement? https://github.com/pre-commit/action ?