Add client authentication with certificates #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I tested this on ESP32-C3 with debug output (we are not able to get debug output on Xtensa currently because there is a problem with variadic args) -0x4290 is likely |
|
I made some progress. First thing was increasing the heap ( https://github.com/esp-rs/esp-wifi/blob/cce6738220f4f12ab4db92f74295e762f5425e99/esp-wifi/src/lib.rs#L96 ) to 110k Then I was able to get through the handshake on ESP32-C3 but I wasn't able to receive data afterwards. Since there might be problems with the async IO I basically did the same things you did for async for the sync API. Now on ESP32-C3 I get this with a However, no luck so far with ESP32-S3 and ESP32. |
|
Seems there is really some mis-compilation / mis-optimization. Building mbedtls in debug mode made it kind of work on ESP32-S3 But the handshake takes forever to complete - also on ESP32 it still doesn't seem to work |
|
Some interesting observations - probably more as a note to self:
|
Merged
|
After rebasing this should work now |
|
Great! I'm gonna test it on my side and finish this PR. |
|
Oops didn't mean to do that. |
AnthonyGrondin
commented
Apr 18, 2023
- Add example for sync esp32s3 - Enable usage of encrypted key with password - Move certificates to a struct
|
Everything seems to work for now. I'm waiting for a first review before doing the other examples. Not sure if it's related to this PR, but it seems like the closing of connection isn't done properly. The given error is: |
bjoernQ
reviewed
Apr 19, 2023
bjoernQ
reviewed
Apr 19, 2023
bjoernQ
reviewed
Apr 19, 2023
bjoernQ
reviewed
Apr 19, 2023
|
I'm wondering if we should unify it under a single function to reduce duplication. Most of the body for |
Agreed - I think I wanted to do it like that in the beginning but when the problems with the pre-compiled binaries kicked in I just went that way The new examples seem to work fine for me on ESP32-S3 - the other examples need adjustments because of the changes to the constructor Great work! |
- Fix existing examples to use the new Certificates struct
|
Nice! Seems like the new examples for ESP32 and ESP32-C3 are missing an `use esp_mbedtls::Certificates;´ - I probably should setup CI in this repo @MabezDev works fine with our Rust 1.68 but with Rust 1.69 I see it gets stuck at the connection to the access point on ESP32-S3, again 😢 I tried tinkering with opt-level and lto etc. without success |
|
I added the imports that I missed. I've only tested on esp32s3, as it's the only device I have on hand. I think some optimizations could be made, by not allocating memory for certificates, if we don't use them. This would be especially useful when not using client certificates, but I'm not sure about the behavior of freeing memory that hasn't been allocated, when dropping the Session struct. |
|
I tested on ESP32 and ESP32-C3 - everything fine now. I'd say this is fine to get merged now. The suggested optimization totally makes sense - if some memory isn't allocated there would be a null-pointer which should get checked in Would be perfectly fine to do the optimization in a follow-up PR and we merge this - not sure what option you'd prefer. Just let me know and I'll approve and merge this |
|
I think we should merge this, then do the optimizations in another PR. I've implemented the functionnalities that I needed, and I would leave you with the optimization part. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable the ability to pass a client certificate for client authentication.
Testing:
cargo run --release --example async_client --features=asyncTesting the certs with curl:
curl https://certauth.cryptomix.com/json/ --key <PRIVATE_KEY>.pem --cert <Certificate>.pem -vCurrently, this returns an error,MBEDTLS_ERR_SSL_HANDSHAKE_FAILURE, and I don't know why it happens,with
MBEDTLS_SSL_VERIFY_OPTIONALthe error changes forMBEDTLS_ERR_SSL_BAD_CONFIG.FIXED
TODOs:
struct.This would make breaking changes less frequents in the future, and reduce the number of arguments
The functions about certificates could also be moved there to reduce complexity
We can allow the passing of a key, but that would increase the number of arguments even more,
hence the discussion of using a
structto handle certs.