refactor: do not use service_data mac

[IvoGruber]

This is for discussion first. Do not understand, why this was used. The spec on bthome.io does not even talk about the flag (at least i did not found anything about it).

@Ernst79:
Can you explain why it exists in the code base?
Can we even get rid of checking for this flag?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.30%. Comparing base (107808e) to head (885eb01).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
- Coverage   83.55%   83.30%   -0.26%     
==========================================
  Files           6        6              
  Lines         663      653      -10     
  Branches       99       96       -3     
==========================================
- Hits          554      544      -10     
- Misses         84       85       +1     
+ Partials       25       24       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Ernst79]

What I remember is that we had an option to include a different MAC address in the message, for a device that is sending the data from a second device, e.g. the indoor temperature sensor is sending the data from the outdoor temperature sensor.

In the V1 docs, I still see something in the docs, in the table in the BTHome Data format section (type 4) (although I agree there isn’t any additional information how this should work).

Moreover, looking at the current code, this isn’t functional in v1.

In v2, in the test, test_bthome_with_mac is a test which includes a MAC address in the payload. Apparanty 0x42 means Mac included in the payload, but I don’t see anything in the v2 docs. The test isn’t really good, as the Mac of the device is the same as what is added in the payload.

Question is, should we remove it, or add it to the docs. I think removing it should be ok, as we already have the option to have multiple sensors. The only downside it that using a different Mac will create a different device in HA, which is no longer possible than.

[IvoGruber]

Moreover, looking at the current code, this isn’t functional in v1.

This is another thing I do not understand. Current code does not use adv_info for V1 at all. It is even not removed when creating the payload for the sensor data. So reading the code I would assume V1 never hat that extra byte in contrast to the spec.

Maybe refactoring things broke that, but I doubt, since all the v1 tests are still running.

[IvoGruber]

Question is, should we remove it, or add it to the docs. I think removing it should be ok, as we already have the option to have multiple sensors. The only downside it that using a different Mac will create a different device in HA, which is no longer possible than.

Also not sure. I doubt that current code would create a different device in HA yet, since that mac is used for logging only and for the nonce for decryption. So removing it would maybe break some devices using encryption if they use that feature.

[Ernst79]

Moreover, looking at the current code, this isn’t functional in v1.

This is another thing I do not understand. Current code does not use adv_info for V1 at all. It is even not removed when creating the payload for the sensor data. So reading the code I would assume V1 never hat that extra byte in contrast to the spec.

Maybe refactoring things broke that, but I doubt, since all the v1 tests are still running.

I don’t think this was functional in v1. In 3.15.0, this was the code

bthome-ble/src/bthome_ble/parser.py

Line 244 in 5b9e2ce

mac_readable = service_info.address

[Ernst79]

Maybe check this issue. #42

On macOS iOS systems apparently the MAC address isn’t included. There is also a note that I should still update the docs :-). I think this is related to the fact that some devices use a rotating MAC address, that changes every x minutes, to prevent tracking. In these cases, you can use a alternative fixed MAC as a workaround.

[IvoGruber]

Maybe check this issue. #42

On MAC OS systems apparently the MAC address isn’t included. There is also a note that IShould still update the docs :-)

So use case is following: They want to use bthome on Mac OS, but can not access the MAC. So to have encrypted data, they want to put that again in the non encrypted part of the service_data. The even want to do that on a mobile device.

Reading on some famous LLM why MacOS is hiding/randomizing MACs makes me even more skeptical whether we should do that. A user that want's encryption very likely also wants privacy. So instead of having MAC exposed even more we should stop using the MAC for encryption.

[IvoGruber]

Maybe check this issue. #42
On MAC OS systems apparently the MAC address isn’t included. There is also a note that IShould still update the docs :-)

So use case is following: They want to use bthome on Mac OS, but can not access the MAC. So to have encrypted data, they want to put that again in the non encrypted part of the service_data. The even want to do that on a mobile device.

Reading on some famous LLM why MacOS is hiding/randomizing MACs makes me even more skeptical whether we should do that. A user that want's encryption very likely also wants privacy. So instead of having MAC exposed even more we should stop using the MAC for encryption.

@Ernst79 Decision is up to you but this would be my proposal:

1. We keep the code as is.
2. We do not describe that feature in the spec.
3. We add a comment to the specific code to declare it as undocumented and not recommended feature (due to privacy reasons) for issue Add data type for MAC/hardware id #42, .
4. We discuss an alternative way, to support that use case that comes without privacy issues.

[IvoGruber]

4. We discuss an alternative way, to support that use case that comes without privacy issues.

This would be my first proposal to solve use case described in issue #42:

1. Generally do not use MAC address of the device in the nonce.
2. Instead put it as the first 6 bytes of the unencrypted payload.
3. Encrypt this payload if requested by associated flag.
4. Thus the MAC is hidden in encrypted mode.
5. When decrypting, MAC address is not needed anymore.
6. After decryption the MAC address is known now.

We can burn another bit in the adv_info to support that mode.
With V3 of BTHome we make that obligatory and can free that bit again.
Thus we will have the MAC address in encrypted and unencrypted mode. In unencrypted mode and environments that hide/randomize the MAC like MacOS we can still map the advertisement to the correct device. In encrypted mode we automatically will have privacy if the environment supports that by hiding/randomizing the MAC address. Encryption will be recommended if privacy is needed.

[IvoGruber]

Also not sure. I doubt that current code would create a different device in HA yet, since that mac is used for logging only and for the nonce for decryption. So removing it would maybe break some devices using encryption if they use that feature.

@Ernst79 can you explain how HA decides when to create a new device based on the bthome code base?

[Ernst79]

Also not sure. I doubt that current code would create a different device in HA yet, since that mac is used for logging only and for the nonce for decryption. So removing it would maybe break some devices using encryption if they use that feature.

@Ernst79 can you explain how HA decides when to create a new device based on the bthome code base?

I don’t know exactly (anymore), but it all starts with the Bluetooth integration, which filters the uuid. If it is a BTHome uuid, the message gets forwarded to the BTHome integration.

I think a device gets it unique id here.

https://github.com/home-assistant/core/blob/2d1a672de5c464a313eab82da331a14acb610635/homeassistant/components/bthome/config_flow.py#L52

however, I don’t see the get_mac_readable function being used to set it as a different device in HA. I think you are right, it isn’t being used to create a second device. It looks like it is only being used in the nonce for encryption.

[Ernst79]

I agree with your proposal, let’s use another bit in the adv_info to create a better implementation. After finishing, I do propose to add the documentation for the non-documented option with the MAC address, and add a warning block that the method contains some security flaws, after which we can tell the user to use the new method instead.

[IvoGruber]

I agree with your proposal, let’s use another bit in the adv_info to create a better implementation. After finishing, I do propose to add the documentation for the non-documented option with the MAC address, and add a warning block that the method contains some security flaws, after which we can tell the user to use the new method instead.

Shall we invite authors in issue #42 to also discuss this or is this automatically done by mentioning the issue here?!

[Ernst79]

It’s always good to invite them. Not sure if they are already informed.

[IvoGruber]

@Ernst79 invited authors of issue #42 manually now. Opened a new PR #328 as draft to continue discussion. Closing this one without merging, since we want to keep the feature for now.

[pvvx]

Reading on some famous LLM why MacOS is hiding/randomizing MACs makes me even more skeptical whether we should do that. A user that want's encryption very likely also wants privacy. So instead of having MAC exposed even more we should stop using the MAC for encryption.

Bluetooth 5.4 standard implements the Encrypted Advertising Data (EAD) function.
There is no point in creating your own formats.