Skip to content

Implement pairing with btstack. #14291

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felixdoerre
Copy link
Contributor

@felixdoerre felixdoerre commented Apr 12, 2024

Btstack offers two abstraction layers for secret storage, one
called "device db" and another called "tlv". Pairing information
is stored in the "device db", additional secrets (like own keys)
are stored directly in the tlv. Luckily there is a "device db"
implementation using tlv, so we only need to provide one interface.

Additionally, I've removed some btstack files from compilation that
were not referenced.

This PR builds upon #10838 where, from what I can tell, only the signed-off-header is missing. If that's a problem I could re-implement those changes.

With these changes I've tested bonding (with mp/btstack as peripheral), with random, public and RPA address.

Adds nimble equivalent behavior to btstack BLE implementations
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Btstack offers two abstraction layers for secret storage, one
called "device db" and another called "tlv". Pairing information
is stored in the "device db", additional secrets (like own keys)
are stored directly in the tlv. Luckily there is a "device db"
implementation using tlv, so we only need to provide one interface.

Additionally, I've removed some btstack files from compilation that
were not referenced.
@andrewleech
Copy link
Contributor

@brianreinhold you might like to compare notes here!

@brianreinhold
Copy link

brianreinhold commented Apr 13, 2024

@felixdoerre We have been trying to get pairing to work on the PICO W which uses the btstack. We have a working implementation (except for interactive passkeys) but it is sharply geared toward our needs. We do not have python expertise and are new to Micropython and the btstack (I have done a lot with other BTLE C stacks designed for specific platforms such as Nordic and Renesas).

Do you have an 'official' version supported through aioble?

We found that some of the trickiest parts were devices that sent security requests to invoke the client to begin either pairing or encryption or both and sometimes as a replacement to an insufficient authentication error.

The difficulty was that the btstack handles the security request and aioble and the app know nothing about it BUT the app (via aioble) has to handle the insufficient encryption/authentication errors. To solve this problem I had to add a method to aioble that would register for an encrytion changed event but do nothing else. In the modbluetooth_btstack.c file I had to add more events to those that would be fed to aioble's encryption changed event. I would run this listener as a task in the background after connection. In this manner I could inform aioble and my application of a pairing or re-encryption event due to security request. It works but it is not nice. Ideally a security request event should be passed up to aioble and the application so the application could invoke pairing/encryption as it does with an insufficient authentication/encryption error.

I suppose I should add to this discussion on pairing the problem of bonding. Many devices (at least health devices that we are interested in) assume that on a bonded reconnect that they can send their data as soon as encryption is established and disconnect (saves battery). aioble does not save service discovery results so these devices often disconnect before aioble completes re-doing service discovery. aioble even invokes descriptor discovery when one tries to 'subscribe' which is slow exasperating the problem. I tried to see if it was possible to save the results of service discovery but it is not as they are objects and not just data and on the next connection (or after a power cycle) this information is no longer valid and one is forced to redo these transactions.

@felixdoerre
Copy link
Contributor Author

Do you have an 'official' version supported through aioble?

I aim that this will be the 'official' version, but that depends on maintainers picking it up. I have not tested it with aioble, but in the scenario that I tested this with, I don't see anything that would prevent it from working with aioble.

We found that some of the trickiest parts were devices that sent security requests to invoke the client to begin either pairing or encryption or both and sometimes as a replacement to an insufficient authentication error.

I did have an issue, where btstack would not answer a pairing request (bluekitchen/btstack#584), but that was a mistake in btstack and it got resolved. The scenario that I have working is micropython/btstack as a ble peripheral (ble hid keyboard) and an android device as ble central. The android device initiates pairing, and micropython/btstack correctly responds and stores bonding information. Reconnection across micropython restarts work flawlessly. This works now both with public and with rpa addresses.

The difficulty was that the btstack handles the security request and aioble and the app know nothing about it BUT the app (via aioble) has to handle the insufficient encryption/authentication errors

So you have micropython acting as ble central/gatt client? I have not tested that.

In the modbluetooth_btstack.c file I had to add more events to those that would be fed to aioble's encryption changed event.

I don't get which problem you are trying to avoid here. I have not tested btstack as ble central, so I am not sure how pairing in response to a insufficient authentication/encryption error would work, but for BLE peripheral/gatt server, I just flag the attribute as authentication/encryption required and btstack handles the corresponding responses.

aioble even invokes descriptor discovery when one tries to 'subscribe' which is slow exasperating the problem

I thought that you only need to "subscribe" once per device/bond and that this information is supposed to be saved persistently.

@brianreinhold
Copy link

@felixdoerre our application is a BTLE central running on the PICO W. The central/client is responsible for initiating pairing (first time) encryption (reconnects). An insufficient authentication/encryption happens when the peripheral has secured an action on a characteristic/descriptor and the client tries to perform that action pre-pairing or pre-encryption. Peripherals do not have to deal with that error. A client is supposed to either pair or encrypt and then redo the action that caused the error.

The btstack does not automatically handle this error (the Android client does) so my application has to catch the GattError exception raised by aioble and use aioble to initiate pairing (same method works for encryption since btstack knows if it is paired).

However, a peripheral can also invoke a security request. Many health devices do this. This tells the client to get off its butt and initiate pairing/encryption. The btstack handles this automatically (as does the Android client) and aioble (and my application) has no idea that it ever happened.

Ideally the btstack would handle BOTH insufficient authentication/encryption errors and the security requests automatically (as on Android), or pass both situations up to the application and let the application do it. I would prefer the latter as ongoing pairing the application may not know about could lead to timeouts on operations the application is trying to do. This is a big pain in Android as I have to check every write and read operation to see if pairing is ongoing and wait until it is finished.

As far as bonding is concerned you are correct - in theory you should only need to subscribe once and if you are bonded that should not have to be repeated. We have not tested that by itself, however, service discovery results are not persisted and that is part of bonding. We need to read the Current Time Service and other characteristics like battery level every connection which requires that we redo service discovery. Also, many BTLE devices require that one re-subscribe event though they say they support bonding. That is just a bad implementation by the peripheral but such devices are on the market and we need to work with them. Resubscribing is fast if the results of service discovery are persisted so it has not been a problem (on Android).

IN any case, that is where we are at. We have it working including some passkey support and unpairing/bonding support at least for a PICO W client. We have no need at this time for peripheral support.

@felixdoerre
Copy link
Contributor Author

However, a peripheral can also invoke a security request. Many health devices do this. This tells the client to get off its butt and initiate pairing/encryption. The btstack handles this automatically (as does the Android client) and aioble (and my application) has no idea that it ever happened.

I would assume that this is what the _IRQ_ENCRYPTION_UPDATE-event is for. I am not sure if this PR already ensures it is triggered correctly, but I imagine this event should be triggered, when pairing and/or encryption was established. aioble does seem to update the DeviceConnection-object (https://github.com/micropython/micropython-lib/blob/master/micropython/bluetooth/aioble/aioble/security.py#L82), but as far as I can see, you cannot subscribe to this event, if this is was you need.

We need to read the Current Time Service and other characteristics like battery level every connection which requires that we redo service discovery.

To me, this feels like a limitation of aioble and not of a limitation of the low-level bluetooth API. And from what I see, there is nothing stopping you from storing the integer-handles from the individual characteristics, including CCCD, and re-creating the objects yourself. However the subscribe method, as implemented in aioble, always does one descriptor lookup, so you would need to provide your own.

@brianreinhold
Copy link

I fully agree that the btstack has no limitations. Once can do all Bluetooth transactions in that library.

The problem with the _IRQ_ENCRYPTION_UPDATE event is that you have to invoke some method that anticipates it, for example, the pairing method in aioble's security module. In order to detect the pairing caused by a security request I added a method to aioble which did nothing but register for that _IRQ_ENCRYPTION_UPDATE event. I invoke a background task on connection which calls that aioble method and when a security request happens, that task gets notified (and I also notify the DeviceConnection object of aioble). It's not the most elegant approach but it does work.

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Apr 19, 2024
@dpgeorge
Copy link
Member

Thanks @felixdoerre for this BTstack pairing implementation, it's definitely something we want and need to support.

From what I can tell the code here looks correct. But when I run the existing BLE multitests for pairing/bonding, they do not work. Looking at tests/multi_bluetooth/ble_gap_pair.py, when I run it on a Pico W with this PR (and a PYBD-SF6 on the other side running NimBLE), it fails to emit the _IRQ_ENCRYPTION_UPDATE event.

Investigating this, I see that BTstack is emitting the SM_EVENT_PAIRING_STARTED event after gap_pair() is called, but then nothing else happens, neither side of the connection sees the pairing complete. In particular I would expect to see the SM_EVENT_PAIRING_COMPLETE event.

In your testing do you ever see the _IRQ_ENCRYPTION_UPDATE on the Python side, or the SM_EVENT_PAIRING_COMPLETE on the C side (by adding printf's in btstack_packet_handler_generic to print the events received)?

@@ -16,45 +16,45 @@ target_include_directories(micropy_extmod_btstack INTERFACE

target_sources(micropy_extmod_btstack INTERFACE
${BTSTACK_LIB_DIR}/platform/embedded/hci_dump_embedded_stdout.c
${BTSTACK_LIB_DIR}/src/ad_parser.c
#${BTSTACK_LIB_DIR}/src/ad_parser.c
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to just remove these unused lines, rather than comment them out.

#if MICROPY_PY_BLUETOOTH_ENABLE_PAIRING_BONDING

static int btstack_tlv_mp_get_tag(void *context, uint32_t tag, uint8_t *buffer, uint32_t buffer_size) {
UNUSED(context);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please just use (void)context; to match the rest of the code

}

static void btstack_tlv_mp_delete_tag(void *context, uint32_t tag) {
mp_bluetooth_gap_on_set_secret(0, (uint8_t *)&tag, sizeof(tag), NULL, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could do with a (void)context; here

@@ -35,7 +35,7 @@
#define MAX_NR_LE_DEVICE_DB_ENTRIES 4

// Link Key DB and LE Device DB using TLV on top of Flash Sector interface
// #define NVM_NUM_DEVICE_DB_ENTRIES 16
#define NVM_NUM_DEVICE_DB_ENTRIES 16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this here, the event _IRQ_GET_SECRET is called 16 times, with the string <n>DBT for n in 0..15 inclusive. I'm not sure exactly what this is needed for, do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the le_device_db implementation of btstack: https://github.com/bluekitchen/btstack/blob/2b49e57bd1fae85ac32ac1f41cdb7c794de335f6/src/ble/le_device_db_tlv.c#L208. Basically it implements the device db as a (fixed) list of device, and this code scans through all slots to find the least recently used empty slot.

@felixdoerre
Copy link
Contributor Author

In your testing do you ever see the _IRQ_ENCRYPTION_UPDATE on the Python side, or the SM_EVENT_PAIRING_COMPLETE on the C side (by adding printf's in btstack_packet_handler_generic to print the events received)?

I only ever tested pairing as initiated by the non-btstack device and in this case I see _IRQ_ENCRYPTION_UPDATE:

1=> 64;42be8ceee95c
31=>DISPLAY: 447648
30=>None;(0, b'\x13CTB')
28=>(64, 1, 1, 1, 16)
30=>b'....';(0, b'\x0fDTB')
28=>(64, 1, 1, 1, 16)
4=>(64, 3)
27=>(64, 6, 0, 500, 0)
27=>(64, 36, 0, 500, 0)

I've just tested it with btstack as pairing initiator and it seems to have worked as well:

ble.gap_connect(1, binascii.unhexlify("...."))
>>> 7=>(65, 1, <memoryview>)
>>> ble.gap_pair(65)
>>> 65;2;0
ble.gap_passkey(65,2,306789)
>>> 28=>(65, 1, 1, 0, 16)
30=>b'...';(0, b'\x0fDTB')
30=>b'...';(0, b'\x0fDTB')
28=>(65, 1, 1, 1, 16)

So this triggers an ENCRYPTION_UPDATE event as well.

I'm testing with ble.config(io=0,addr_mode=2,le_secure=False,mitm=True,bond=True), and ble.config(io=4,addr_mode=2,le_secure=False,mitm=True,bond=True)

I'll look closer at tests/multi_bluetooth/ble_gap_pair.py (between two pico Ws) and see why it fails.

@dpgeorge
Copy link
Member

I only ever tested pairing as initiated by the non-btstack device

The multitests can be run either way around. So if you have a non-Pico-W that has BLE (eg an esp32 device) it's possible to test both cases.

As for the configuration:

  • tests/multi_bluetooth/ble_gap_pair.py tests with ble.config(mitm=True, le_secure=True, bond=False)
  • tests/multi_bluetooth/ble_gap_pair_band.py tests with ble.config(mitm=True, le_secure=True, bond=True)

@brianreinhold
Copy link

@dpgeorge I had to add extra code in the modbluetooth_btstack.c file in order to get the _IRQ_ENCRYPTION_UPDATE on the python side due to peripherals sending security requests. When a peripheral sends a security request, the btstack client side responds by invoking pairing (if a first time connect) and invoking encryption (on a bonded reconnect). I had to map the completion of those processes to the _IRQ_ENCRYPTION_UPDATE. I don't recall the details as I did it many months ago.

As I stated, it works, but I don't like the asymmetric behavior (the app handles the insufficient authentication error but the stack handles security requests).

@dpgeorge
Copy link
Member

As I stated, it works, but I don't like the asymmetric behavior (the app handles the insufficient authentication error but the stack handles security requests).

The first step is to get BTstack on par with the NimBLE implementation, and that means getting the above-mentioned multtests to pass on BTstack. The semantics of _IRQ_ENCRYPTION_UPDATE should match how/when the multitests expect this event to be raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants