zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.66k stars 6.52k forks source link

Bluetooth: Support for multiple local identity addresses #7473

Closed carlescufi closed 6 years ago

carlescufi commented 6 years ago

In order to support the HID usecase, the BLE Host needs to be able to switch its identity address on the fly.

The parts involved in order to make this possible are:

carlescufi commented 6 years ago

@pizi-nordic and @jhn-nordic: Please detail the full requirements here so we can take a look CC @oyvindronningstad

jhedberg commented 6 years ago

Assuming that we go with the approach of supporting multiple identity addresses, I'd propose extending the settings storage in the following forward-compatible way:

Currently the identity address is stored in the key bt/id and the local IRK in bt/irk. I.e. one bt_addr_le_t in the first one and one u8_t irk[16] in the second. What I'd do is to convert these to arrays, i.e. first one would be array of bt_addr_le_t and the second array of u8_t irk[16]. The index to the arrays would be the "identity identifier", which is something we'd also use in the code to refer to a specific identity. So identity 0 would be what we support right now.

The next step is to extend the bonding keys and CCCD storage. Currently these are stored in bt/keys/<remote addr> and bt/ccc/<remote addr>. I'd consider keeping these as the locations for identity 0. For other identities I'd just append the identity identifier, so e.g. bonding keys for identity 1 would be stored in bt/keys1/<remote addr>. In the code itself I'd use u8_t for the identifier, so there'd be a limit of max 256 identities, is that ok? This u8_t identifier would be present in struct bt_dev (e.g. to identify which identity we're advertising with), struct bt_conn (to identify which identity the connection was created with) as well as struct bt_keys. The existing bt_dev.id_addr and bt_dev.irk variables would become arrays, whose size would be determined by a new CONFIG_BT_IDENTITY_MAX Kconfig variable.

For the public API I'd propose something like:

int bt_identity_create(bt_addr_le_t *addr);

int bt_identity_get(u8_t id, bt_addr_le_t *addr);

These would fill in the bt_addr_le with the identity address, and return the identifier (index) of the identity (or a negative error number on error). The recently added bt_set_id_addr() could remain, and would keep operating on identity 0.

For removing an identity there could be something like:

int bt_identity_remove(u8_t id);

And, in the longer run, if we want to have something like was proposed by @holtmann on IRC, there could be a:

int bt_identity_set_privacy(u8_t id, bool enable);

To be able to select a specific identity to advertise with, I'd extend our struct bt_le_adv_opt with a u8_t identity member. This should keep any old code doing the "right thing" since any member not explicitly initialized defaults to 0 (well, depends on how the stack variable is declared and initialized, but it'd at least work the way our helper macros do it and how our existing sample code does it).

Thoughts?

jhedberg commented 6 years ago

One thing I'm wondering, I didn't find it useful to expose the actual IRK value publicly, and perhaps we don't even need/want to expose the address itself either? I.e. all the app knows is that there's a certain set of identities or that it created a new one with a given id.

carlescufi commented 6 years ago

Thanks @jhedberg for proposing this API. I think it makes sense. In general I think it is a very good proposal. Here are some comments:

  1. We still need to support setting the id address manually for each identity. This is not only because we already have an API, but also because hard-coded addresses in production are very common. In fact longer term we might want to think about the app pre-provisioning IRK and LTK as well, since as mentioned it is common to do this in "bundle" products. Exposing the IRK to the app is probably not required, but allowing it to set it is, later down the line.

  2. The way you suggest to store bonds is logical to me. For pre-provisioning we would need an additional bt_identity_set_local_keys(id, peer) or similar. In any case I don't see anything in the way that you are going to store the local addresses, IRKs or anything else that would be an issue here.

  3. The selection of identity needs to apply to scanning and connection creation down the line as well, so we will need to think about it.

carlescufi commented 6 years ago

@jhedberg why not:

bt_identity_create(bt_addr_le_t **addr, u8_t **irk);

If the addr/irk params are NULL, then the stack populates them. Otherwise the stack takes the values coming from the app. EDIT: This still doesn't solve the public address one completely, since that comes from the controller itself. Maybe we need to separate the address type from the address pointer in this call.

oyvindronningstad commented 6 years ago

About the index:

jhn-nordic commented 6 years ago

Here are a description of the real life use cases that are the reason behind the need for an approach with multiple identities

Use case A:

kb_3hosts

Use case A1:

I have a BT enabled keyboard that supports only one bond at the time. I am now connected and bonded to Host A and want to pair with Host B. I press the pairing button on the keyboard. This operation deletes the bond the keyboard have to Host A (in the keyboard), and the keyboard starts advertising for a new bond. If we do not change our identity Host A will connect to keyboard immediately if within range and we cannot do a succesfull pairing with Host B.

Use case A2:

I have a BT enabled keyboard that can support up to i.e 3 bonds. The keyboard is only connected to one Host at the time, and it has 3 buttons that selects which host it should connect to. When searching for new bonds we will run into the same problems as in Usecase A1 If we do not operate with multiple identities

Use case A3:

I have a BT enabled keyboard that can support up to i.e 3 bonds The keyboard is CAN BE connected to all 3 hosts simultaneously, and it has 3 buttons or a host side SW that selects which host that shall receieve the HID inputs. When searching for new bonds we will run into the same problems as in Usecase A1 and A2 If we do not operate with multiple identities. However, in this use case we must also handle having simultaneous connections using different identites.

Use case B:

These are basically variants of the A3 use case with similar needs for multiple identies and connections.

remote_2hosts

Use case B1:

I have a BT enabled remote control that supports only one bond at the time. I am now connected and bonded to Host D and want to pair with Host E. I press the pairing button on the remote to start the pairing procedure towards Host E. However I still want the remote control to be fully operation towards Host D until the moment where the remote control is fully configured and bonded to Host E. Then HID input is routed to Host E and Host D is disconnected.

Use case B2:

I have a BT enabled multi remote control that supports multiple bonds The remote control is CAN BE connected to multiple hosts simultaneously (i.e. TV, set-top box, Hi-Fi, media centre), Different buttons can send HID events to one or multiple hosts. I.e a global "mute"-button could send the mute HID command to several of the hosts.

carlescufi commented 6 years ago

since the user might have stored the index for its own purposes.

Not sure I follow. The user is the one deleting the identity, so it should know not to use that index anymore unless it creates a new one.

oyvindronningstad commented 6 years ago

since the user might have stored the index for its own purposes.

Not sure I follow. The user is the one deleting the identity, so it should know not to use that index anymore unless it creates a new one.

In practice, this is cumbersome because one part of the app doesn't know every other part. Being told that an identity is deprecated would make some things much easier.

carlescufi commented 6 years ago

@jhn-nordic: Thanks for the use cases.

carlescufi commented 6 years ago

I would like to cover the following possibilities here in this issue:

a) Single identity, multiple IRKs, per-peer b) Multiple identities, multiple IRKs, per-identity

In each of the above the public address should be an identity, only if it exists.

Additionally I would like to cover pre-provisioning of bonding information. This can be done in 2 ways:

1) Add APIs for it 2) Add a sample that pre-populates the settings, from the application code, to add the required addresses, IRKs and LTKs.

I think I'd rather concentrate on 2) to avoid polluting the APIs.

pizi-nordic commented 6 years ago

Hi @jhedberg, @carlescufi

Here you have some of my thoughts:

Vudentz commented 6 years ago

@pizi-nordic bt_gatt_attr already takes the bt_conn as parameter with that the application should be able to tell what identity it is using. Also just to be clear here, when it comes to GATT database the stack only manages the attributes of the services it has registered, the only exception being CCCD when registered with BT_GATT_CCC and Id like to keep it like that, if you want any service to have advanced control using the identity addresses I guess we would need it to be part of the stack with kconfig entry to enable it, though it may be tricky with service that do require specialized application since that probably means profile specific public API on top of GATT.

pizi-nordic commented 6 years ago

Thanks @Vudentz. I just wanted to be sure that this feature will be easy to use from application standpoint.

jhedberg commented 6 years ago

Question: we have the public int bt_unpair(const bt_addr_le_t *addr); API. With the introduction of multiple identities, should this remain as-is, i.e. unpair addr for any identity it's found under. Or, should we break the API by extending it with an id parameter?

Another question: how important is it to extend this support for central role APIs? We have things like the following which may be affected:

struct bt_conn *bt_conn_create_le(const bt_addr_le_t *peer,
                                  const struct bt_le_conn_param *param);
int bt_le_set_auto_conn(bt_addr_le_t *addr,
                        const struct bt_le_conn_param *param);

Should we break those APIs as well? Another less intrusive alternative would be to have something like bt_le_central_set_id() to be called before either of the above two calls.

jhn-nordic commented 6 years ago

I think we need to know which identity we unpair from. Adding id as a parameter would work. As an example if you have a BLE keyboard that supports 3 bonds that you can select with 3 buttons you can do 3 pairings with the same host using 3 different identities (if you want). If you then call bt_unpair() with only the host address as a reference you would then unpair all the 3 bonds you have.

the bt_unpair() in the API is quite new and I guess the pain of changing the API now will be much less painful than doing it on later stage.

Regarding central role API I do not see a use for multiple IDs at the moment from my perspective (HID). Any comments @pizi-nordic @Qbicz ?

Qbicz commented 6 years ago

I agree with @jhn-nordic. Regarding central role - it is probably not suffering from the limitation described in the HID use case, as usually the central initiates the connection. Moreover if central connects to one peripheral it does not prevent it from scanning and connecting to another peripheral. So for central having multiple identities is not that important.

pizi-nordic commented 6 years ago

At the moment I do not see any use case for multiple identities for central role.