esp-rs / esp-mbedtls

mbedtls for ESP32 bare-metal
Apache License 2.0
17 stars 7 forks source link

Ensure that certificates are null terminated #6

Closed AnthonyGrondin closed 1 year ago

AnthonyGrondin commented 1 year ago

This is a quality of life PR.

Mbedtls requires that certificates and passwords passed to it are null terminated \0, like C strings.

This new helper checks if the certificates are null-terminated, and if not, it adds the nullish terminator.

This allow to include certificates using include_str!() without having to append \0 ourselves, since the library takes care of it.

AnthonyGrondin commented 1 year ago

~Currently having an Error when using a private key that isn't null terminated. Trying to figure that one out.~

https://github.com/esp-rs/esp-mbedtls/blob/623f5eb07a54e69d28c94472562f1e44ff810e28/esp-mbedtls/src/compat.rs#L190-L199

This returns a pointer to a buffer that gets dropped at the end of the function. I would need to refactor to do something like StrBuf.

Would it be simpler to just change StrBuf to take a generic, so we can dynamically size it?

EDIT: With the current implementation, where Certificates {} only take &str, it doesn't allow us to use certificates in DER format. I think we should do something like: https://github.com/esp-rs/esp-idf-svc/blob/master/src/tls.rs, and handle it there.

bjoernQ commented 1 year ago

For certs at least maybe taking a byte slice instead of &str is a good thing (plus checking the null termination at least in debug mode)

Adding the \0 ourselves would be very convenient but we need to copy all the data to RAM then.

For passwords we can do the same or do the null-termination for convenience (since they are not that huge) - we just should make sure that we wipe out the copied bytes of the password after usage

AnthonyGrondin commented 1 year ago

Since it would be easier to take a reference to a slice, and let the user manage how they store their certificates, I've moved over most of the code from https://github.com/esp-rs/esp-idf-svc/blob/master/src/tls.rs, but we return an error if the pem doesn't end with a null byte, instead of panicking.

I've tested with both der and pem, and it seems to work. ~I'm waiting for review before updating the other examples.~

EDIT: I've updated the examples

AnthonyGrondin commented 1 year ago

I think we could merge this, unless there are other changes needed.

bjoernQ commented 1 year ago

Thanks for the reminder - I somehow forgot about this