This is obviously a large increase in code size (for the builds that can afford it). But I don't see any other option. Without elliptic curve cryptography, MicroPython devices are partially cut off from the internet.

For users that need to reduce code size they'll need to build custom firmware with a custom mbedtls config.

This is obviously a large increase in code size (for the builds that can afford it). But I don't see any other option. Without elliptic curve cryptography, MicroPython devices are partially cut off from the internet.

Definitely, for reference Server_Side_TLS recommendations from Mozilla.
(from python ssl docs)

For users that need to reduce code size they'll need to build custom firmware with a custom mbedtls config.

In case anyone needs this: Reducing Mbed TLS memory and storage footprint

Thanks @Carglglz , both of those links are helpful!

The ssl_data.py multitest now fails, due to this PR. It's because the server wants to use elliptic curves but the supplied key/cert in ssl_data.py is not "good enough" anymore.

So this PR is on hold until we resolve that issue.

The ssl_data.py multitest now fails, due to this PR. It's because the server wants to use elliptic curves but the supplied key/cert in ssl_data.py is not "good enough" anymore.

So this PR is on hold until we resolve that issue.

I can reproduce this, #define MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED option is what makes it fail. Not sure why, but ssl_cert_rsa.py test passed, maybe is time to deprecatessl_data.py and add ssl_cert_ec.py? (I do have this test already and it passed too)

maybe is time to deprecatessl_data.py and add ssl_cert_ec.py?

But deprecating it will be a breaking change for users who have built a web server using older encryption standards. Would be best to not break ssl_data.py.

The ssl_data.py multitest now fails, due to this PR. It's because the server wants to use elliptic curves but the supplied key/cert in ssl_data.py is not "good enough" anymore.
So this PR is on hold until we resolve that issue.

I can reproduce this, #define MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED option is what makes it fail. Not sure why, but ssl_cert_rsa.py test passed, maybe is time to deprecatessl_data.py and add ssl_cert_ec.py? (I do have this test already and it passed too)

The problem here is that it's negotiating the highest supported cipher by both ends (now ECDHE), but then the server is presenting an RSA cert.

I think what we need to do is support the ciphers kwarg to wrap_socket (this is also in CPython, as well as on SSLContext.set_ciphers), which will take a comma separated list of mbedtls cipher names to turn into a list of integers to update o->conf with after https://github.com/micropython/micropython/blob/master/extmod/modussl_mbedtls.c#L188

Could use mbedtls_ssl_ciphersuite_from_string etc...

We'll have to make the default something that maintains backwards compatibility (as Damien just commented above).

maybe is time to deprecatessl_data.py and add ssl_cert_ec.py?

But deprecating it will be a breaking change for users who have built a web server using older encryption standards. Would be best to not break ssl_data.py.

I agree with not making breaking changes, I meant to change key/cert in ssl_data.py to use EC key/cert.
Right now ssl_data.py key is RSA 512 bits, which is definitely not a real use case (at least it needs 2048 bits as per recommendations above).

DBG:../../lib/mbedtls/library/ssl_srv.c:0816: cert. version     : 1
...
DBG:../../lib/mbedtls/library/ssl_srv.c:0754: signed using      : RSA with SHA1

DBG:../../lib/mbedtls/library/ssl_srv.c:0754: RSA key size      : 512 bits

The problem here is that it's negotiating the highest supported cipher by both ends (now ECDHE), but then the server is presenting an RSA cert.

I did enabled #define MBEDTLS_DEBUG_C to see what's really going on and looks like cipher suite is

DBG:../../lib/mbedtls/library/ssl_srv.c:2024: selected ciphersuite: TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384

And failure happens here

+DBG:../../lib/mbedtls/library/ssl_srv.c:3274: mbedtls_pk_sign() returned -16512 (-0x4080)
+
+DBG:../../lib/mbedtls/library/ssl_tls.c:8216: <= handshake
+
+DBG:../../lib/mbedtls/library/ssl_tls.c:9060: => free
+
+DBG:../../lib/mbedtls/library/ssl_tls.c:9125: <= free
+
+Traceback (most recent call last):
+  File "<stdin>", line 121, in <module>
+  File "<stdin>", line 51, in instance0
+  File "ssl.py", line 1, in wrap_socket
+OSError: (-16512, 'MBEDTLS_ERR_RSA_BAD_INPUT_DATA')

Running ssl_cert_rsa.py it uses same cipher suite

DBG:../../lib/mbedtls/library/ssl_srv.c:2024: selected ciphersuite: TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384

The only difference is in key/cert

DBG:../../lib/mbedtls/library/ssl_tls.c:5489: cert. version     : 3
...
DBG:../../lib/mbedtls/library/ssl_tls.c:5489: signed using      : RSA with SHA-256

DBG:../../lib/mbedtls/library/ssl_tls.c:5489: RSA key size      : 4096 bits

And it does pass the test. So it looks to me like this cipher suite TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA384 does not allow RSA 512 bits key size.

If MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED is disabled, the cipher suite selected

+DBG:../../lib/mbedtls/library/ssl_srv.c:2024: selected ciphersuite: TLS-RSA-WITH-AES-256-CBC-SHA256

allows this key size and the test runs without failures.

[EDIT]
I traced the error to

mbedtls/library/pk.c
387:int mbedtls_pk_sign_restartable( mbedtls_pk_context *ctx,
441:    return( mbedtls_pk_sign_restartable( ctx, md_alg, hash, hash_len,

int mbedtls_pk_sign_restartable( mbedtls_pk_context *ctx,
 388   │              mbedtls_md_type_t md_alg,
 389   │              const unsigned char *hash, size_t hash_len,
 390   │              unsigned char *sig, size_t *sig_len,
 391   │              int (*f_rng)(void *, unsigned char *, size_t), void *p_rng,
 392   │              mbedtls_pk_restart_ctx *rs_ctx )
 393   │ {
 394   │     PK_VALIDATE_RET( ctx != NULL );
 395   │     PK_VALIDATE_RET( ( md_alg == MBEDTLS_MD_NONE && hash_len == 0 ) ||
 396   │                      hash != NULL );
 397   │     PK_VALIDATE_RET( sig != NULL );
 398   │
 399   │     if( ctx->pk_info == NULL ||
 400   │         pk_hashlen_helper( md_alg, &hash_len ) != 0 )
 401   │         return( MBEDTLS_ERR_PK_BAD_INPUT_DATA );  <--

So it may be that this ciphersuite doesn't allow hashing algo SHA1...

 * Helper for mbedtls_pk_sign and mbedtls_pk_verify
 223   │  */
 224   │ static inline int pk_hashlen_helper( mbedtls_md_type_t md_alg, size_t *hash_len )
 225   │ {
 226   │     const mbedtls_md_info_t *md_info;
 227   │
 228   │     if( *hash_len != 0 )
 229   │         return( 0 );
 230   │
 231   │     if( ( md_info = mbedtls_md_info_from_type( md_alg ) ) == NULL )
 232   │         return( -1 );
 233   │
 234   │     *hash_len = mbedtls_md_get_size( md_info );
 235   │     return( 0 );
 236   │ }

Hard to tell tbh 🤷🏼‍♂️ which one it is (key or md algo) , also from mbedtls/include/mbedtls/rsa.h

990   │ int mbedtls_rsa_rsassa_pkcs1_v15_sign( mbedtls_rsa_context *ctx,
 991   │                                int (*f_rng)(void *, unsigned char *, size_t),
 992   │                                void *p_rng,
 993   │                                int mode,
 994   │                                mbedtls_md_type_t md_alg,
 995   │                                unsigned int hashlen,
 996   │                                const unsigned char *hash,
 997   │                                unsigned char *sig );

...

* \note           This function always uses the maximum possible salt size,
 *                 up to the length of the payload hash. This choice of salt
 *                 size complies with FIPS 186-4 §5.5 (e) and RFC 8017 (PKCS#1
 *                 v2.2) §9.1.1 step 3. Furthermore this function enforces a
 *                 minimum salt size which is the hash size minus 2 bytes. If
 *                 this minimum size is too large given the key size (the salt
 *                 size, plus the hash size, plus 2 bytes must be no more than
 *                 the key size in bytes), this function returns
 *                 #MBEDTLS_ERR_RSA_BAD_INPUT_DATA.

@jimmo

I think what we need to do is support the ciphers kwarg to wrap_socket (this is also in CPython, as well as on SSLContext.set_ciphers),

I've already done this (kind of) in #8968 (Only one cipher suite at the moment, but I could change it to make it accept a list)
However due to different naming conventions between MbedTLS (IANA) and OpenSSL it won't be 100% compatible with CPython
see openssl-iana mapping.

I've added ciphers kwarg to wrap_socket and set to TLS-RSA-WITH-AES-256-CBC-SHA256 cipher suite. The test passed as expected, however I still would update the RSA key to at least 2048 bits and add a ssl_cert_ec.py test...

@dpgeorge
If you don't want to modify ssl_data.py by any means disabling #define MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED for unix port is the only option I see right now. It won't be a breaking change since there would be plenty of cipher suites (RSA and ECDSA based) that server and client can agree while doing the handshake...

Also to avoid confusion#define MBEDTLS_ECP_DP_CURVE25519_ENABLED can be disabled since it is not used for TLS right now (It's not implemented in MbedTLS, there are some PRs and it is supposed to be on the roadmap but no ETA as of now...)

If you don't want to modify ssl_data.py by any means

I would like to keep this test as-is. It provides a good baseline test with a simple (albeit old/weak) key/cert pair.

disabling #define MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED for unix port is the only option I see right now.

Yes, that could be a solution to get this PR merged. The rp2 port has that enabled already, and it can just stay enabled in that port only, no other ports will enable it (yet).

so to avoid confusion#define MBEDTLS_ECP_DP_CURVE25519_ENABLED can be disabled

OK.

Also to avoid confusion#define MBEDTLS_ECP_DP_CURVE25519_ENABLED can be disabled since it is not used for TLS right now (It's not implemented in MbedTLS, there are some PRs and it is supposed to be on the roadmap but no ETA as of now...

@Carglglz can you provide more info about this? It looks like there's code in mbedtls to implement this curve, and disabling it reduces code size on the unix port by around 4.5k. So it must do something...?

@dpgeorge

@Carglglz can you provide more info about this? It looks like there's code in mbedtls to implement this curve, and disabling it reduces code size on the unix port by around 4.5k. So it must do something...?

Yes, full info here curve25519 support

The key part:

Unfortunately, they use slightly different data structures and representations than the other curves, so they haven’t been ported yet to TLS and PKIX in Mbed TLS. We do support basic Curve25519 arithmetic though.

So it must do something...?

Yes, there is a sample ecdh_curve25519.c program in mbedtls/programs/pkey where it performs a ECDHE key exchange, but that's it, unfortunately no TLS implementation as of now (see EdDSA software implementation project of MbedTLS github)

Also based on performance

# NIST 
ECDSA-secp521r1          :    1093 sign/s
ECDSA-secp384r1          :    1556 sign/s
ECDSA-secp256r1          :    2121 sign/s
ECDSA-secp224r1          :    3103 sign/s
ECDSA-secp192r1          :    4107 sign/s
ECDSA-secp521r1          :     299 verify/s
ECDSA-secp384r1          :     431 verify/s
ECDSA-secp256r1          :     612 verify/s
ECDSA-secp224r1          :     935 verify/s
ECDSA-secp192r1          :    1316 verify/s

# BRAINPOOL
ECDSA-brainpoolP512r1    :     163 sign/s
ECDSA-brainpoolP384r1    :     361 sign/s
ECDSA-brainpoolP256r1    :     603 sign/s
ECDSA-brainpoolP512r1    :      37 verify/s
ECDSA-brainpoolP384r1    :      81 verify/s
ECDSA-brainpoolP256r1    :     156 verify/s

I would disable Brainpool curves, and even maybe a subset of NIST could be good enough e.g. only these three.

#define MBEDTLS_ECP_DP_SECP256R1_ENABLED
#define MBEDTLS_ECP_DP_SECP384R1_ENABLED
#define MBEDTLS_ECP_DP_SECP521R1_ENABLED

I'm pretty sure these ones are widely supported.

Thanks @Carglglz for the info. Based on that I have disabled curve 25519 and the brainpool curves. Hopefully that doesn't break any existing use cases!

I think this PR is now ready to merge.