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

Pairing agent #1100

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft

Pairing agent #1100

wants to merge 3 commits into from

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented Oct 31, 2022

This is a work-in-progress attempt at implementing to implement programmatic pairing on Linux and Windows (alternative to the stalled #640).

I'm fairly happy with the Linux/BlueZ implementation at this point. But the Windows implementation is quite flaky need work.

Fixes #827

Issues to be resolved before merging:

  • Clarify new exception types. Currently there are two new exceptions to indicate that pairing failed based on error returned by BlueZ. It was expected that these should correspond to the org.bluez.Error.Canceled and org.bluez.Error.Rejected responses sent by the agent. However, these don't seem to get passed through the Bluetooth stack and both come out the other end as a generic org.bluez.Error.AuthenticationFailed error. org.bluez.Error.AuthenticationCanceled seems to only happen when the device disconnects before pairing is complete.
  • Additional pairing methods need to be implemented in BlueZ backed. Currently, only "RequestPasskey" is implemented, which corresponds to BaseBleakAgentCallbacks.request_pin().
  • Figure out how to properly do cancellation on Windows. It seems very easy to get the Bluetooth stack on Windows in a bad state if you end the Python process without allowing the pairing to complete. I had to reboot my computer many times while working on this to restore the Windows Bluetooth stack to a good state.
  • Figure out how to get the pair_async() method to return when accept() is not called. The pair method seems to block forever unless the accept() method of the DevicePairingRequestedEventArgs is called. But the user might not want to accept the pairing, so how do we handle this case? Also, if a pin is requested and you reply with a non-numeric string, the pair_async() method never returns instead of the expected invalid pin error.
  • Settle on pin type, int vs str. BlueZ uses UInt32 for the pin while Windows uses a string. It is currently implemented here using str. But (as noted in the previous item) Windows seems to have problems with non-numeric pins, so it might be better to use int.
  • Figure out how to do cross-platform async reading from stdin for the example. I found a method that works nicely on *nix, but it doesn't work on Windows. Everything I have tried on Windows so far causes a crash or locks up stdin.

@dlech dlech mentioned this pull request Oct 31, 2022
@dlech dlech marked this pull request as draft November 8, 2022 21:01
@bojanpotocnik
Copy link
Contributor

bojanpotocnik commented Nov 15, 2022

Using the pairing_agent.py example provided in this PR

    # REVISIT: implement passing capability if needed
    # "DisplayOnly", "DisplayYesNo", "KeyboardOnly", "NoInputNoOutput", "KeyboardDisplay"
    capability = ""
    print("pairing...")
    callbacks = AgentCallbacks()
    async with BleakClient(device) as client:
        try:
            await client.pair(callbacks)
            print("success")
        except BleakPairingCancelledError:
            print("paring was canceled")
        except BleakPairingFailedError:
            print("pairing failed (bad pin?)")

connection to my test device fails with (error from Silabs BLE stack):

Bonding failed (reason=0x1205 (Pairing not supported))

When the original code is modified, so that the pairing agent has proper capabilities

    # REVISIT: implement passing capability if needed
    # "DisplayOnly", "DisplayYesNo", "KeyboardOnly", "NoInputNoOutput", "KeyboardDisplay"
    capability = "KeyboardDisplay"

and the agent is created before connection

    print("pairing...")
    callbacks = AgentCallbacks()
    client = BleakClient(device)
    # Bus needs to be connected for RegisterAgent in bluez_agent() to succeed
    await client._backend._bus.connect()
    async with bluez_agent(bus=client._backend._bus, callbacks=callbacks):
        async with client:
            try:
                await client.pair()  # Do not pass callbacks to prevent registering another agent
                print("success")
            except BleakPairingCancelledError:
                print("paring was canceled")
            except BleakPairingFailedError:
                print("pairing failed (bad pin?)")

the example works.

I took this concept and changed API a bit - my proposed solution is here pairing-agent...bojanpotocnik:bleak:pairing

@dlech
Copy link
Collaborator Author

dlech commented Nov 16, 2022

Thanks for trying this out.

When the original code is modified, so that the pairing agent has proper capabilities

    # REVISIT: implement passing capability if needed
    # "DisplayOnly", "DisplayYesNo", "KeyboardOnly", "NoInputNoOutput", "KeyboardDisplay"
    capability = "KeyboardDisplay"

BlueZ is supposed to use "KeyboardDisplay" as a fallback when given an empty string 1. Which BlueZ version are you using? Can you share Wireshark logs of the different behavior between "" and "KeyboardDisplay"?

and the agent is created before connection

Hmm... if this is needed for this device, then the Windows implementation doesn't seem likely to be able to work with this device. Can you Wireshark logs showing the difference between registering the agent before connecting vs. registering the agent after connecting?

@bojanpotocnik
Copy link
Contributor

bojanpotocnik commented Nov 17, 2022

You're correct about IO capabilities - the behaviour in both cases is the same (that is, the pairing works with my fork even with capability = "").

I've attached bleak_pr1100_wireshark_captures.zip containing original.pcapng and bojan_fork.pcapng, captured using nRF Sniffer for Bluetooth LE.

Interestingly, in both cases Master (my PC) sends LL_FEATURE_REQ with the same feature set:

Feature Set: 0x00000000000059ff
    .... ...1 = LE Encryption: True
    .... ..1. = Connection Parameters Request Procedure: True
    .... .1.. = Extended Reject Indication: True
    .... 1... = Slave Initiated Features Exchange: True
    ...1 .... = LE Ping: True
    ..1. .... = LE Data Packet Length Extension: True
    .1.. .... = LL Privacy: True
    1... .... = Extended Scanner Filter Policies: True
    .... ...1 = LE 2M PHY: True
    .... ..0. = Stable Modulation Index - Transmitter: False
    .... .0.. = Stable Modulation Index - Receiver: False
    .... 1... = LE Coded PHY: True
    ...1 .... = LE Extended Advertising: True
    ..0. .... = LE Periodic Advertising: False
    .1.. .... = Channel Selection Algorithm #2: True
    0... .... = LE Power Class 1: False
    .... ...0 = Minimum Number of Used Channels Procedure: False
    0000 000. = Reserved: 0
    Reserved: 0000000000

Shortly after, Slave requests pairing with

Bluetooth Security Manager Protocol
    Opcode: Security Request (0x0b)
    AuthReq: 0x0d, Secure Connection Flag, MITM Flag, Bonding Flags: Bonding
        000. .... = Reserved: 0x0
        ...0 .... = Keypress Flag: False
        .... 1... = Secure Connection Flag: True
        .... .1.. = MITM Flag: True
        .... ..01 = Bonding Flags: Bonding (0x1)

to which Master responds with

Bluetooth Security Manager Protocol
    Opcode: Pairing Failed (0x05)
    Reason: Pairing Not Supported (0x05)

in original.pcapng, or

Bluetooth Security Manager Protocol
    Opcode: Pairing Request (0x01)
    IO Capability: Keyboard, Display (0x04)
    OOB Data Flags: OOB Auth. Data Not Present (0x00)
    AuthReq: 0x0d, Secure Connection Flag, MITM Flag, Bonding Flags: Bonding
        000. .... = Reserved: 0x0
        ...0 .... = Keypress Flag: False
        .... 1... = Secure Connection Flag: True
        .... .1.. = MITM Flag: True
        .... ..01 = Bonding Flags: Bonding (0x1)
    Max Encryption Key Size: 16
    Initiator Key Distribution: 0x0d, Link Key, Signature Key (CSRK), Encryption Key (LTK)
    Responder Key Distribution: 0x0f, Link Key, Signature Key (CSRK), Id Key (IRK), Encryption Key (LTK)

in bojan_fork.pcapng.

Looks like BlueZ thinks bonding is not supported unless I register my own agent. Note that I'm testing this on Lenovo Thinkpad using Ubuntu 20.04, BlueZ 5.62 with system pairing agent present (I would understand such behaviour on a headless RPi).

@dlech
Copy link
Collaborator Author

dlech commented Nov 17, 2022

Thanks for the analysis. I had not considered the case where the remote device initiates pairing.

Given that Windows only support custom pairing when the central initiates the pairing, it would be nice if we could only support that case to keep things consistent across platforms. Would it work to do something like this?

client = BleakClient(device, ...)
await client.pair(...)
try:
     ...
finally:
    await client.disconnect()

There are probably some modifications needed to get the services but in BlueZ pairing will result in connecting the device. I'll have to look to see how it would work on Windows.

@bojanpotocnik
Copy link
Contributor

bojanpotocnik commented Nov 18, 2022

I had not considered the case where the remote device initiates pairing.

Technically, pairing is still initiated by the central device (Pairing Request), the peripheral just requests the higher security level (the thing which triggers pairing in in Core Bluetooth).

The only difference here is that usually, peripheral devices allow service discovery without pairing, but send Security Request when central tries to read the value of certain protected characteristic. We usually know that such device will require pairing to read the value, so we initiate pairing before reading the value (after service discovery), so the Security Request is never received.
My device requires higher security level for whole GATT table, meaning it requires pairing even for service discovery. As this is usually done automatically, initial service discovery will fail and peripheral will send Security Request, expecting central to initiate pairing and try again.

I am not sure if I understand what is different in solution proposed in your last comment. However, I just tested with

    print("pairing...")
    callbacks = AgentCallbacks()
    async with BleakClient(device, pairing_callbacks=callbacks) as client:
        await asyncio.sleep(10)

and

class AgentCallbacks(BaseBleakAgentCallbacks):
    async def request_pin(self, device: BLEDevice) -> str:

was still invoked, so effectively you're correct about "the remote device initiates pairing".

I understand your argument about keeping things consistent across platforms, however I am fond of the idea of supporting wider range of devices, if it only cost changing API in a way that pairing callbacks are provided to the BleakClient constructor instead of pair() method and registering the pairing agent earlier. The behaviour for all existing devices shall stay the same - or am I missing something? 🤔
I will try to get a chance to test what happens with my device on Windows in the following days.

@dlech
Copy link
Collaborator Author

dlech commented Nov 18, 2022

My device requires higher security level for whole GATT table, meaning it requires pairing even for service discovery.

This seems to go against the requirement of the Bluetooth specification. For example, in the Core spec v5.3, Part G, Section 3.1, it states:

A service declaration is an Attribute with the Attribute Type set to the UUID for «Primary Service» or «Secondary Service». ... The Attribute Permissions shall be read-only and shall not require authentication or authorization.

There is similar language for other attributes related to service discovery.

however I am fond of the idea of supporting wider range of devices, if it only cost changing API

While I generally agree with this sentiment, I'm not fond of trying to support devices that don't follow the specifications.

The behaviour for all existing devices shall stay the same - or am I missing something?

In the case of BlueZ, it means that the agent is registered as long as the device is connected instead of just during the pairing operation. It also complicates managing the lifetime of the agent, such as ensuring that the agent is unregistered when the device is disconnected. These sorts of issues are a common source of bugs and cost quite a bit of my time, hence my pushing back against the proposed API changes.

Since this only works with the BlueZ backend and is only needed for devices like this that don't follow the specifications, I would suggest using your original workaround using the current proposed API:

    callbacks = AgentCallbacks()
    client = BleakClient(device)
    await client._backend_bus.connect()
    async with bluez_agent(client._backend._bus, callbacks), client:
        ...

This adds a close() method to the BleakClient class and backends. This
is used to explicitly close an resource, like the D-Bus socket in the
BlueZ backend.

In the BlueZ backend, the D-Bus socket is no longer closed on
disconnect. This is needed to avoid problems with D-Bus methods failing
with EOFError() or hanging forever because the socket was disconnect.
This adds a callbacks arg to programmatically responding to pairing
requests instead of allowing the OS to prompt the user.

This just adds the parameter but does not provide an implementation yet.
@bojanpotocnik
Copy link
Contributor

bojanpotocnik commented Nov 21, 2022

I double checked the peripheral device and done a bit more testing.

There are 2 separate things this peripheral device is using to ensure sufficient security level, both of them are per BT spec.

You are correct that Attribute Permissions do not require authentication or authorization, but reading the value does - this peripheral device has Authenticated + Bonded + Encrypted selected for all enabled entries in table shows under Set up Attribute Permissions in GATT. So although it is indeed impossible to protect reading attribute access properties and permission levels, everything else can be protected (and most central devices try to automatically read GATT characteristic descriptors).

Additionally, there is another feature, described in Core Spec 5.2 | Vol 3, Part H, 2.4.6 Slave Security Request:

The slave device may request security by transmitting a Security Request command to the master. When a master device receives a Security Request command it may encrypt the link, initiate the pairing procedure, or reject the request.
...
If pairing or encryption mode is not supported or cannot be initiated at the time when the slave’s Security Request command is received, then the master shall respond with a Pairing Failed command with the reason set to “Pairing Not Supported.”

This device utilizes Increase Security Level feature immediately after connection, to ensure that unauthorized central cannot be connected to this peripheral - otherwise malicious central device could connect without trying to read any GATT value (therefore not initiating pairing) and stay connected, effectively blocking other central devices to connect.

I tested with Windows 10 yesterday and I was greeted with Windows notification that the device would like to pair (system pairing agent), while Bleak was waiting for pairing process to be completed. I will continue testing in the evening - I suspect adding DevicePairingKinds.PROVIDE_PIN (and implementing the usage of BaseBleakAgentCallbacks) instead of DevicePairingKinds.CONFIRM_ONLY will be enough, otherwise I will try with custom_pairing.add_pairing_requested(handler) before initiating connection.

manager = await get_global_bluez_manager()
props = manager.get_device_props(device_path)
return BLEDevice(
props["Address"], props["Alias"], {"path": device_path, "props": props}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
props["Address"], props["Alias"], {"path": device_path, "props": props}
props["Address"], props["Alias"], {"path": device_path, "props": props}, props.get("RSSI", -127)

because of bcae937#diff-d647d8e3f316fd939334bd8ed60d248095c8a6f69bc73dd0b369867609dc00a6R21

@bojanpotocnik
Copy link
Contributor

During testing on Windows, I discovered that my problem can be solved with completely different and much simpler approach, as described in #1133

@bojanpotocnik
Copy link
Contributor

@dlech do you intent to merge 9472a10 separatelly/earlier?

@dlech
Copy link
Collaborator Author

dlech commented Dec 5, 2022

I haven't had time to get back to any of this yet. I will have to take another good look at everything again to decide the best way to move forward. But that commit does seem useful on it's own.

@bojanpotocnik
Copy link
Contributor

Though I've not been able to closely follow all the updates and improvements made to bleak in the past months, I believe some "Issues to be resolved before merging" listed in the original post might already have been taken care of (albeit possibly in a different way).
@dlech, would you be able to update the original post to indicate what tasks still need to be completed, based on your vision of how this should be properly implemented? This would help me in revisiting this PR when I can find some spare time.

@abmantis
Copy link

abmantis commented Oct 7, 2023

Could it be an option to try to move this forward in smaller increments? For example, if Windows is still problematic, an initial version could support only bluez. Also, additional pairing methods could be added later.
That could also allow other people to contribute and implement those missing parts, since the groundwork would already be available :)

@someburner
Copy link

someburner commented Nov 25, 2023

@dlech

this seems to work for me on linux, mostly.

but on ubuntu 22 (bluez 5.64), pairing of a device im working with results in immediate disconnect before pin code handler is able to run. log:

2023-11-25 00:47:54,960 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96): ['org.bluez.Device1', {'UUIDs': <dbus_fast.signature.Variant ('as', ['00001800-0000-1000-8000-00805f9b34fb', '00001801-0000-1000-8000-00805f9b34fb', '0000180a-0000-1000-8000-00805f9b34fb', '0000fef5-0000-1000-8000-00805f9b34fb', '18424398-7cbc-11e9-8f9e-2a86e4085a59'])>, 'ServicesResolved': <dbus_fast.signature.Variant ('b', True)>}, []]
2023-11-25 00:47:54,961 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_55_F9_E7_C6_D4_05): ['org.bluez.Device1', {'RSSI': <dbus_fast.signature.Variant ('n', -96)>, 'TxPower': <dbus_fast.signature.Variant ('n', 11)>}, []]
2023-11-25 00:47:54,961 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_5D_A1_70_38_ED_EA): ['org.bluez.Device1', {'RSSI': <dbus_fast.signature.Variant ('n', -85)>}, []]
2023-11-25 00:47:54,962 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96): ['org.bluez.Device1', {'Trusted': <dbus_fast.signature.Variant ('b', True)>}, []]
2023-11-25 00:47:54,962 - Pairing to BLE device @ FB:FB:FB:FF:08:96
2023-11-25 00:47:55,285 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96): ['org.bluez.Device1', {'Modalias': <dbus_fast.signature.Variant ('s', bluetooth:v00D2p0580d0100)>}, []]
2023-11-25 00:47:56,367 - RequestPasskey /org/bluez/hci0/dev_FB_FB_FB_FF_08_96
pairing failed (bad pin?)
2023-11-25 00:47:56,370 - Disconnecting (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96)
2023-11-25 00:47:56,495 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96): ['org.bluez.Device1', {'ServicesResolved': <dbus_fast.signature.Variant ('b', False)>}, []]
2023-11-25 00:47:56,496 - received D-Bus signal: org.freedesktop.DBus.Properties.PropertiesChanged (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96): ['org.bluez.Device1', {'Connected': <dbus_fast.signature.Variant ('b', False)>}, []]
2023-11-25 00:47:56,496 - Device disconnected (/org/bluez/hci0/dev_FB_FB_FB_FF_08_96)
2023-11-25 00:47:56,496 - _cleanup_all(/org/bluez/hci0/dev_FB_FB_FB_FF_08_96)

removing ble_device = await self._create_ble_device(device) and removing the device arg from callback causes it to work as expected. Could just be something wonky with my laptop / ble driver, but I have a pretty common intel ax200. Maybe something to do with async trusted prop change?

As an aside- I think it would be great to be able to provide a pin in client.pair(pin=1234). Then if the device is unpaired, create a basic "predefined pin agent" that simply returns that pin. Maybe just me but I think that would handle a lot of use cases.

Also: If pairing fails, maybe unset trusted, assuming it was not already trusted prior to bleak attempt.

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

Successfully merging this pull request may close these issues.

programmatically accept pairing request in python without dialog interaction
4 participants