Skip to content
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

CSIS: Invalid usage of bt_conn_auth_cb callbacks #42928

Closed
MariuszSkamra opened this issue Feb 17, 2022 · 8 comments · Fixed by #43540
Closed

CSIS: Invalid usage of bt_conn_auth_cb callbacks #42928

MariuszSkamra opened this issue Feb 17, 2022 · 8 comments · Fixed by #43540
Assignees
Labels
area: Bluetooth Audio area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@MariuszSkamra
Copy link
Collaborator

The current CSIS implementation registers authentication callbacks. Those callbacks are used to handle authentication pairing requests. There can be only one instance of registered callbacks, thus the one shall be registered by application that handles pairing and csis.c is not the one.

Once the CSIS registers the callbacks, the callbacks cannot be further registered in any other place, as -ALREADY error is returned.
The only callbacks handled in csis.c are pairing_complete and bond_deleted which disables support for authenticated pairing in the system.

@MariuszSkamra MariuszSkamra added bug The issue is a bug, or the PR is fixing a bug area: Bluetooth Audio labels Feb 17, 2022
@MariuszSkamra
Copy link
Collaborator Author

@Thalley please assign the "Bluetooth LE Audio" project, seems I do not have sufficient permissions to assign project

@Thalley
Copy link
Collaborator

Thalley commented Feb 17, 2022

With the current implementation of bt_conn_auth_cb_register, the only work around is then to require the application to inform CSIS of a bond, which is something I'd like to avoid.

I wonder if it would be possible to allow multiple registrations of bt_conn_auth_cb instead, as we have for many other callbacks.

@MariuszSkamra
Copy link
Collaborator Author

If the bond is removed, the ACL link is disconnected. So the only need is to check whether bt_addr_le_is_bonded on bt_conn_cb->connected callback and register for bt_conn_cb->security_changed callback

@Thalley
Copy link
Collaborator

Thalley commented Feb 17, 2022

If the bond is removed, the ACL link is disconnected. So the only need is to check whether bt_addr_le_is_bonded on bt_conn_cb->connected callback and register for bt_conn_cb->security_changed callback

CSIS also needs to know when security with bonding is finished, as there is a requirement to send notifications when that happens. For this purpose, CSIS also needs to know when a bond has been removed (which it may be after disconnection).

@carlescufi
Copy link
Member

Please add "Bluetooth" as a label aside from "Bluetooth Audio"

@Thalley Thalley self-assigned this Feb 22, 2022
@carlescufi carlescufi added the priority: low Low impact/importance bug label Feb 22, 2022
@Thalley
Copy link
Collaborator

Thalley commented Mar 8, 2022

@jhedberg @joerchan Do you know/remember why only a single instance of struct bt_conn_auth_cb can be registered?

It does have some sense for the majority of the callbacks where the host asks for acceptance from the register, but for the informational callbacks (pairing_complete, pairing_failed and bond_deleted) it should be possible to register that multiple times.

We can split up the callback structure between functional and informational callbacks, or allow multiple registers. The security failed/complete is all that necessary as you can get the same information from bt_conn_cb.security_changed, but bonding information can only current be retrieved from the bt_conn_auth_cb, and doesn't really fit into the bt_conn_cb struct.

I do have an alternative to using the bonded_deleted that I'd like to try out before we change anything.

@joerchan
Copy link
Contributor

joerchan commented Mar 8, 2022

@Thalley I don't remember the reasoning, but probably @Vudentz does.

@jhedberg
Copy link
Member

jhedberg commented Mar 8, 2022

@Thalley yes, the original reasoning is that it needs to be clear who answers to the callbacks that solicit a response. I'm fine with splitting this up so that informational callbacks are separate (and can be registered multiple times)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Audio area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants