-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Security manager take 2 #286
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for your work on this! I made a first pass through it, and noted the following:
-
I'm wondering if it's more appropriate if security_manager is part of the connection_manager, rather than adding methods on connection_manager to convert between index and handle. It's a bad to hand out indexes to anything other than Connection instances, especially if it's used across await points, because then you may use an index to a connection that has been drop'ed, and even if it's safe now (I haven't really traced the code paths for that), it might lead to accidental bugs later.
-
From what I understand, the security manager packets work on top of l2cap, and could use the tx packet pool (https://github.com/embassy-rs/trouble/blob/main/host/src/host.rs#L70). Would it make sense to use that instead rather than having it's own?
-
For the examples you've added, are they tested and verified to work on real hardware?
Other than the above, I think it pretty much aligns nicely with the existing api and architecture.
Thanks for reviewing,
|
Would it be possible for the host to take an arbitrary |
It probably would be possible, I did not find any good solution tough. When I started out I tried to sprinkle |
I personally think it's fine to not use the CryptoRng traits, it's has an unfortunate effect of creeping into the entire code base and making it a lot harder to make modifications. I learned this the hard way in lora-rs, and my preferred tradeoff from that experience would be to maybe use the CryptoRng trait when creating the stack only, as a way to produce a good seed for the 'internal' rng. Another point is that the hardware rngs are not always that fast, at least the nRF rng is pretty slow (if i remember correctly). And finally, if I understand correctly, it's only used in the pairing process, so it's not something on the critical path once pairing is done? So at least in the first pass, I'm fine with not using the rng traits. |
Providing a CryptoRng to generate a seed is a good idea. I will try that out. I tried to check if ChaCha12 is good enough as per the Bluetooth standard (Vol 2, Part H, Chapter 2). It needs to pass a test suite, and it seems like ChaCha12 passes the test suite dieharder. Might need some more investigation tough. |
Yes, I agree that adding generic spam to everything is not worth it. It would only be useful if the generic "blast radius" could be contained to the security manager or related functions. Using CryptoRng to generate a seed is a good idea. |
a2679a3
to
a0f0d9d
Compare
Notes from using a CryptoRng to seed ChaCha12. Serial-HCI, tests, ...I provided nRF-SDCthe nRF-SDC takes the random generator, which means I couldn't put it into the Maybe there is a way to initiate in another order to be able to use the nRF random generator. ESP32I created a Raspberry Pi Pico WNot very familiar with the RPI-pico's. I found a ring oscillator random bit generator, which doesn't seem to have cryptographic properties. I seeded a ChaCha12 from |
a0f0d9d
to
12ae4d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is starting to look really good. I have one wish though, and that is the API for passing the rng seed. Rather than passing it to new(), I think it would be nicer expose a set_random_seed() or 'enable_rng()' on the Stack
type (and propagate to BleHost internally), similar to the set_random_address(). This way it doesn't get in the way if you don't use the security manager.
If it's hard to do since it happens after creating the security manager, we should consider a StackBuilder or something that has that method.
This is how the random seed was configured earlier, I believe it will work. One nit is that the user might forget to add a seed and end up with a not so secure random generator, but this can be improved upon at a later date. I will try to implement the change. I was able to wire up a Raspberry Pi W, the I also want to look into how to implement the pairing timeout, currently investigating if |
What I'd like to see, as a developer, is a build warning if I'm building without proper seeds. Like In practise, it could be done like this:
An alternate way could be having different APIs based on the features. Some library had this, but I personally find it weird. It's confusing for documentation and what not. I'd recommend always sticking to one API per released version. Just my 2c. |
bff1355
to
eee3170
Compare
Use ChaCha12 in security manager. Tested on ESP32-C6, nrf52833 and Serial HCI.
5cd98e1
to
bf8888d
Compare
Security manager
Work in progress Bluetooth LE security manager for trouble-host.
Builds on the works of @HaoboGu and https://github.com/bjoernQ/bleps.
Starting this draft PR for working out integration into TrouBLE. Following details some of the current design choices
Current Implementation
Bluetooth LE Security Connection (LESC) with Just Works.
Inner cryptographic random generator
Chacha12 is used to provide a cryptographic random generator in the security manager. Seeded from the user.
HCI Event and Command handling
HCI event and command handling is done through a
embassy_sync
Channel
.Known limitations
Pairing Methods
Only LESC with Just Works/Numerical comparison is implemented. No support for BLE Legacy, LESC keypass entry or LESC out-of-band.
No privacy or signing support
Support for identity resolving keys (IRK) and Connection Signature Resolving Key (CSRK) has not been implemented.
Testing done
Tested
ble_bas_peripheral_sec
against nRF Connect on Android and iPad.Tested
ble_bas_central_sec
againstble_bas_peripheral_sec
.Following hardware was used,