-
Notifications
You must be signed in to change notification settings - Fork 849
[BLE] Add support for decrypting PVVX, BTHome v2 and Victron BLE frames #2219
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
[BLE] Add support for decrypting PVVX, BTHome v2 and Victron BLE frames #2219
Conversation
…t drop down rather than checkbox
…eviceName and ForceDeviceName
…t drop down rather than checkbox
…teway using via_device as it should be the gateway mac address not name
I'll leave the final review to @1technophile and @h2zero . Just a few comments after briefly going over the PR. .I'm not a fan of the introduction of tempChar. And think the Metric and Imperial properties should be kept as they are for auto-discovery and JSON publishing. Explained in more detail in Currently it looks as if it's only checking for the presence of the "encr" property in the received JSON
but without checking for the actual encryption scheme number - "encr":2 for BTHome v2 encryption, but currently also all LYWSD03MMC with PVVX native encryption "encr":1, and Victron Energy devices "encr":3 would trigger this and fail miserably. I also see that you only have defined a single AES encryption bindkey as a build definition. What really is need there is the possibility to define MACaddress:Bindkey pairs, as implemented in Theengs Gateway https://gateway.theengs.io/use/use.html#reading-encrypted-advertisements Even then, defining it as a build definition would restrict this to users being able to create their won builds. Not and issue for you and me, but for decryption to be included in an OMG release build I think this would need to be usable for all users, also the ones only able to install pre-built binaries. So storing these MAC address:Bindkey pairs in flash would be one possibility, by sending them as an MQTT command and/or through the WebUI. Or someone else might have a better idea for it. I greatly appreciate your effort to bring decryption to OMG, and I'm sure this is working great for you and others only using BTHome v2 devices from your comments in other closed issues - if they don't come back stating that they cannot make their own build ;) For a releasable version though I think there are still some refinements required addressing the issues mentioned above. |
Thanks for this, I completely agree with all your feedback above about the Metric vs Imperial and will adjust that. To me the "Display Metric" just didn't make any sense without going to the documentation so a select drop down makes more sense in my view. Agree all three algorithms should be supported and it is something I will spend some time probably next weekend. I hadn't found the algorithms until you pointed me to the python code and now I know what is needed. https://github.com/theengs/gateway/blob/development/TheengsGateway/decryption.py Was just looking for where the when I have some time. The PVVX build modules can have their bindkey updated so having a shared encryption key is possible. But since the Victron have a static key then I will add a default + MACaddress:Bindkey option in the UI plus in the code would be the way to implement it. |
…ayBT for specific MACAddress AES Keys
…ert the documentation to Units of measurement displayed
Hi @DigiH I took onboard your feedback and have added support for PVVX and Victron. I have commented out my client side JavaScript which I created for field validation. The one issue I do see is the
|
Thanks for your efforts on this.
We can work on this in a separate PR |
Could you make this PR only about decryption and move the commits related to:
If you need help let me know. |
Checking the logic for the Victron decoder and I found out that the Theengs gateway code is wrong for https://github.com/theengs/gateway/blob/development/TheengsGateway/decryption.py#L145 It's actually needs to be 16 bytes as per: https://github.com/keshavdv/victron-ble/blob/main/victron_ble/devices/base.py#L1035 Tested with the example messages in the tests from victron_ble and was getting the expected decrypted value. Also shortened the logic for the client side Javascript code for the BLE Keys. This is commented out due to size. Also simplified he logic in there to reduce the size. Did you want me to commit the original html page I built to test? |
…ommented out due to image constrains on theengs-bridge-v11
@plambrechtsen Doesn't seem to be a byte count issue after all ;) As the Theengs Gateway code is running and decrypting actual Vitron devices fine in the wild, with nonce padding like 12ab should be padded to 12ab000000000000 I'm not quite sure what you mean with padding to 8 bytes is wrong and it should be 16 bytes. |
Great add, let me know if ready |
@DigiH when I got back home yesterday I thought that I should find some example encrypted Victron frames and make sure they decrypt correctly using
Then I should have got this plaintext
But I didn't. However when I padded the nonce to 16 bytes.
Then I got the correct results. I checked out a number of other implementations and they all have a 16 and not 8 zero padded string. I suspect it's because of the python library used in Theengs Gateway's that the decryption works so it is good luck rather than working correctly. That's why I needed to change the nonce length to be 2 bytes of he IV + 14 zero padded bytes for the I think it's good to merge @1technophile as I don't think there is anything further that needs to be added. The only thing would be around if you want the client side Javascript code included as part of the build or not. It adds 600 bytes or so but it does then make sure that the user enters the AES keypairs in the correct format. That is the only thing that you might need to think about the pros and cons of having that in the build or not. I also have a HTML page I built and tested the javascript in before I minified it. I didn't add that to the PR as I wasn't sure where it should go or if you wanted it at all. Let me know and I can add that in too if you want. |
I also used this online decryption tool to verify some of the examples given by the Victron user https://www.lddgo.net/en/encrypt/aes and this Decryptor there worked fine with the 8 bytes padding I got from the Victron forum. Testing it again now with 16 bytes it also works. Only when padding to less than 8 bytes does it come up with ![]() So different libraries indeed seem to behave slightly differently, especially since the padding, be it 8 bytes or 16 bytes or actually any byte number in between, is all with 0s ;) For bindkey matching and full consistency we could also extend the Theengs gateway Python padding to 16 bytes. It will work either way, as with the online decryptor ![]() |
Either way, thanks a lot for bringing this to OpenMQTTGateway! 👍 C++, Pythion, Java … as long as it works correctly on each platform :) FYI - theengs/gateway#308 I think it'll be a great addition to the next release, as some users have already been wishing for being able to use encrypted devices on OMG. |
@1technophile - You want to create a separate test dev build for this for the Victron user to also try out, as @plambrechtsen has already verified with his LYWSD03MMC, or just merge it and then the nightly build can be used for Victron verification? I've already contacted the Victron user. |
Thanks for that @DigiH confirming the nonce length issue in AES-CTR I have also started separating the previous changes around the display name into separate PRs. |
Yes, this has been mentioned before. I also want to look at the mix of written out and abbreviated property names in the discovery messages. Which apparently isn't officially supported, bu in HA itself seems to pass through, wile in other controllers, which (partially) support HA auto-discovery, actually breaks the discovery. That'll have to wait until I'm back at my proper dev station though. |
Thanks ! |
@DigiH I restarted the nightly build so that you have the development ready with the PR tomorrow morning |
I just installed the latest development build on one of my production BLE gateways with all the recent additions. I really do like the WebUI interface for entering the BLE AES Default Key and MAC:AES Key pairs 👍 Anything entered however is volatile and not persistent across restarts, which do happens occasionally on BLE gateways, more or less frequent depending on the individual setup. I mentioned before that these would be stored in flash (similar to all the other settings) and then reloaded if and when a restart happens, as otherwise the decryption functionality and the related devices will get lost at any time due to a random restart. |
ble_aes_keys = jsonBLEBuffer.to<JsonObject>(); | ||
} | ||
# ifndef ESPWifiManualSetup | ||
saveConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the saveConfig()
function is called @DigiH so as long as you don't have ESPWifiManualSetup
then it should work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plambrechtsen I overlooked that comment before, but that is what Iand many other users have, plus me also the parent MQTT_HTTPS_FW_UPDATE undefined ;)
} | ||
# endif | ||
} | ||
# ifdef BLEDecryptor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is where the config gets loaded back in @DigiH from the /config.json
Hi @DigiH it should be saving config.
It should be working as I have tested saving the config to the Try uncommenting the And add a Both should show the config being loaded and saved. |
Are they boing loaded at startup again though? Does it work for you when you enter a BLE AES Default Key different to the default one – I just changed the leading three octets to and a MAC:AES Key pair – I used a default MAC address without colons:Victron sample key. Then hit But issuing a restart command to my gateway, the BLE config page was all reset to the default. But as I do not have any encryption capable devices I couldn't try and see if the functionality might still be there and it is just a UI issue, which might be a possibility as well :) |
The default needs to be a 32 character hex string. And then key pairs needs to be 12 characters hex for the mac without colons and then a : as a delimiter, then 32 characters hex for the key. And a space (not new line) to delimit each entry. If you try uncommenting line https://github.com/1technophile/OpenMQTTGateway/blob/development/main/config_WebContent.h#L230 as that has client side validation javascript of both values in the browser for the default key and key pairs. Which I had to comment out due to size restrictions on the theengs plug, but it works on most other ESP32s. |
Code wise, all nice and good - not really looking at code tonight - in a Sunday evening TV series mode here right now ;) So I only quickly installed the latest dev build earlier tonight, and tried entering something in both fields. Fine when coming back from other UI pages – the entered details are still there, and I assume work correctly. Issue a restart command to the gateway, log into the WebUI again and the BLE config page is reset to the defaults. Is it a functionality issue or only a still confusing UI issue – I cannot tell without any decryption capable devices. How does this work for you on a gateway at runtime? Or have you only tested it with the default build coded default AES key, which I assume you are using for all your devices? You are submitting great new functionalities and fixes at incredible speed over the last days! So how does it behave at runtime for you with different details? |
Hmmmm - very strange. I'm still seeing it reset to defaults every single time. The only difference I can think of now is that I updated over an existing setup running gateway, whereas you might be running your development version with already dditional changes not merged into OMG development yet? Or did you also test with the current OMG development state – 6d1b793? I will look at this again tomorrow. Maybe @1technophile can also see which behaviour he is getting. |
I built from the latest development branch. Making only the tweaks I talked about enabling the client side JavaScript validation and logging the config file on save and load.
Update
Update
Then it worked fine for me. |
My misunderstanding then. I assumed the merged state was functional, and that the disabled JavaScript validation was only that, an optional validation of the entered keys, but not necessary for functionality. |
As the guy with the Victron devices wanted to install and run the development test build binaries I told him to hold off for the time being. @plambrechtsen Let us know when the development binaries will have the additions included to address this. |
Ok, retested this issue I was/am seeing, and pinned it down to the following. A default esp32dev-ble environment build of the latest OMG development state does actually work fine and keeps the entered Default Key and MAC:Binkey pair stored over restarts, BUT installing the same latest OMG development with my custom environment setup, as I did for the initial testing on one of my production BLE gateways, the volatility of the keys is reproducible. FYI I use
with that ifndef also being part of the larger
but I also undefine this for all my custom environments I'm also using '-DNetworkAdvancedSetup=true' with all its network definitions and several other build flags, but haven't looked any further where these might also affect this issue. The conclusion being - the keys will be saved and loaded fine for any user installing the pre-built binaries or building from the pre-defined environments, but for this to work for all possible custom environments the saving and loading of the details needs to be placed somewhere where it cannot mistakenly be disable by some unrelated build flag. Just for clarification ;) the save and load for the WiFi/MQTT Broker details will obviously need to stay as they are, just the new AES key/.pairs will need to be saved and loaded independently of any build flag setting. |
I'm sure the save/load issue will be addressed at some stage so that everyone will be able to use decryption. As the Victron user is using pre-built binaries I asked him to start testing today. With Trace logging it became apparent that there isn't even an attempt to try to decrypt the Victron devices ;) Small typo for which I submitted |
…es (1technophile#2219) * Changing WebUI to include display device name, and change it to select drop down rather than checkbox * Fix mqttDiscovery to require WebUI and ESP32 for displayDeviceName * Fix mqttDiscovery to require WebUI and ESP32 and ESP8266 for displayDeviceName and ForceDeviceName * Changing WebUI to include display device name, and change it to select drop down rather than checkbox * Fixes for WebUI and BT for supporting custom setting Display name * Fixes for WebUI and BT for supporting custom setting Display name * Move DISPLAY_DEVICE_NAME to User_config.h * Update docs to include change for Display temperature * Update docs to include change for Display temperature * Fix minor cosmetic bug where devices were not linking in HA to the gateway using via_device as it should be the gateway mac address not name * Add support for decrypting BTHome v2 frames * Add support for decrypting BTHome v2 frames * Add support for decrypting BTHome v2 frames * BTHome fix issue with theengs-plug * BTHome fix issue with theengs-plug * Adding support for all BLE encrypted methods, support in UI and gatewayBT for specific MACAddress AES Keys * Fix lint * Fix build issue with theengs-bridge-v11 and esp32dev-all-test and revert the documentation to Units of measurement displayed * Revert docs * Revert displayDeviceName and Units of measurement * Revert displayDeviceName and Units of measurement * Revert displayDeviceName and Units of measurement * Revert minor typo * Revert minor typo * Revert minor typo * Bug in Victron as nonce should be 16 bytes * Shortened the client side javascript for BLE key validation that is commented out due to image constrains on theengs-bridge-v11
Add support for decrypting PVVX, BTHome v2 and Victron BLE encrypted frames
Description:
This adds quite a few new features
macaddress:aeskey
mapping with field validationChecklist: