search

espressif / esp-mqtt

ESP32 mqtt component
Apache License 2.0
603 stars 255 forks source link

add explicit cert-/key-length parameters to esp_mqtt_client_config_t (IDFGH-1366) #119

Closed huelsenfruchtzwerg closed 5 years ago

huelsenfruchtzwerg commented 5 years ago

this is to allow using other formats besides PEM for certificates and keys, like DER the underlying library (mbedtls) supports this, but since DER may contain 0-bytes, it's necessary to explicity set the length instead of relying on strlen.

huelsenfruchtzwerg commented 5 years ago

No comments? Admittedly it's not the prettiest patch ('cause of the - 1 workaround). The proper way would be to patch esp_transport_ssl as well. If there's interest I'd be willing to go do that as well, as I'd really like the ability to use der certificates for the smaller flash footprint.

david-cermak commented 5 years ago

Hi @huelsenfruchtzwerg

Sorry, have missed the first notification. very useful to enable also DER certificates. Thanks!

As said, this -1 fix is not 100% clean (calling transport_ssl API which sets PEM cert data.) Please feel free to create a corresponding PR in IDF with related modification of transport_ssl (and link it with this one) Also note, that we shall keep the backward compatibility for the current esp_transport_ssl_set_cert_data with current arguments (size without terminating '\0') and perpaps create a new API to accept both PEM and DER certificates (in case of PEM include additional zero termination to be in line with mbedtls)

huelsenfruchtzwerg commented 5 years ago

I made an esp-idf pull-request to clarify some comments and add _der variants of the set_cert_data (etc.) functions.

huelsenfruchtzwerg commented 5 years ago

I changed the commit to use the new _der variants in esp-tls. Unfortunately (and unsurprisingly, I suppose), now the legacy build fails. I'm a little uncertain on how to proceed. #ifndef the feature away?

david-cermak commented 5 years ago

@huelsenfruchtzwerg Yes, please. Use the "feature" way and make it available for IDF >= 4.1.0. Thanks!

huelsenfruchtzwerg commented 5 years ago

@M1cha: You could still set the length explicitly in esp_mqtt_client_config_t and it would work... that is if mbedtls would support non-terminated PEM, which it unfortunately doesn't. Look at it this way: If the length parameter is provided, it assumes you've passed it the correct length, and if it isn't, it assumes the cert/key parameter is 0-terminated pem, because that's the only circumstance it could determine the length itself (and the current behaviour).

huelsenfruchtzwerg commented 5 years ago

@david-cermak: I added the feature-check

huelsenfruchtzwerg commented 5 years ago

I'm not sure about adding the _len parameters unconditionally. What would we do with them on esp-idf < 4.1? The only two options I could think of are:

  1. silently ignoring it
  2. or using the old functions and applying the "-1 hack" to the length.

Something else I just realized: The comments now claim to support DER-format unconditionally as well, which is not true, (at least in case of option 1).

david-cermak commented 5 years ago

@huelsenfruchtzwerg Good point, we can Ignore it, but not silently. If the feature is not available in actual IDF but used, we could emit an error, such as here: https://github.com/espressif/esp-mqtt/blob/master/mqtt_client.c#L1369-L1372

PS: That's how it typically works for other features as well. Public header provides all the latest functionalities, but when linking with older framework some of them may not be available an appropriate error message is thrown.

huelsenfruchtzwerg commented 5 years ago

@david-cermak: Alright, I added error checks when _len is used.