feat(crypto): allow running without PyCryptodome (for embedded targets)#94
Open
cgoudie wants to merge 1 commit into
Open
feat(crypto): allow running without PyCryptodome (for embedded targets)#94cgoudie wants to merge 1 commit into
cgoudie wants to merge 1 commit into
Conversation
Add an optional fallback to PyCA's `cryptography` library for the single AES-128-CTR decryption call in `Device.decrypt`. Behaviour is unchanged for users who already have PyCryptodome installed; the fallback only kicks in when PyCryptodome is unavailable. Motivation ---------- On some embedded platforms (notably Victron's own Venus OS), the `cryptography` package is part of the base image but PyCryptodome is not, and binary wheels are not published for the target architecture (armv7l). Installing PyCryptodome from source requires `gcc` plus Python development headers, neither of which is present on the device. Allowing `cryptography` as a backend lets `victron_ble` run on these platforms with no native build step. Implementation -------------- * Both libraries are detected at import time with independent `try / except ImportError` blocks setting `_HAVE_*` flags. An explicit `ImportError` is raised only if neither is available. * The PyCryptodome decrypt path is preserved verbatim as the reference implementation. * The `cryptography` path drives AES-ECB manually and generates the keystream block by block from a little-endian-encoded counter. This is necessary because `cryptography.modes.CTR` follows NIST SP 800-38A and increments big-endian, whereas the Victron Instant Readout protocol uses `Counter.new(128, initial_value=iv, little_endian=True)` which increments little-endian. The two backends produce byte-identical output when both are installed. * `Device.decrypt` calls the backend-agnostic `_aes_ctr_decrypt` helper. `requirements.txt` is intentionally left untouched; PyCryptodome remains the default install dependency. Users who would rather avoid PyCryptodome can install with `--no-deps` and provide `cryptography` themselves. Tests ----- Five new tests in `tests/test_base.py`: * `test_at_least_one_backend_available` — sanity check. * `test_known_vector` — captured PyCryptodome output, runs against whichever backend is selected at runtime. * `test_short_ciphertext_padded_to_block` — exercises `_pkcs7_pad16`. * `test_backends_agree_single_block` — when both libraries are installed, single-block output is identical. * `test_backends_agree_multi_block` — exercises 16 blocks of plaintext, catching any back-end that mis-handles counter increments. * `test_backends_agree_across_le_carry` — chooses an iv that triggers a low-byte LE carry on the first increment, which would diverge if the cryptography fallback used a big-endian counter. The full test suite passes (54 tests) and `make lint` is clean. Co-authored-by: Cursor <cursoragent@cursor.com>
cgoudie
pushed a commit
to TechBlueprints/venus-os-dbus-ble-sensors-py
that referenced
this pull request
May 8, 2026
The cryptography fallback in ext/victron_ble/devices/base.py used cryptography.modes.CTR with a 16-byte little-endian-encoded nonce. That is byte-correct for the *first* AES block but diverges from PyCryptodome on every block after that, because cryptography's CTR mode follows NIST SP 800-38A and increments the counter big-endian while PyCryptodome's Counter.new(..., little_endian=True) increments little-endian. In production the Victron Instant Readout payload always fits in a single block after the key-check byte, so this bug never manifested in the field. But the patch claimed byte-identical equivalence with PyCryptodome, and that claim was wrong. The fix: drive AES-ECB manually and generate each keystream block from a little-endian-encoded counter we increment ourselves. This matches PyCryptodome byte-for-byte across blocks and across the 8-bit / 16-bit LE carry boundaries. Also strengthen the cross-backend test in tests/test_vendored_victron_ble.py to cover three cases: - iv=0xABCD, single block (production case) - iv=0xABCD, 16 full blocks (counter increments) - iv=0xFF, 3 blocks (forces low-byte LE carry) The same fix has been submitted upstream as keshavdv/victron-ble#94. When that lands and ships in a release, we can revert ext/victron_ble/ to the unmodified upstream tarball and drop VENDORED.md's local-patch note. Co-authored-by: Cursor <cursoragent@cursor.com>
cgoudie
pushed a commit
to TechBlueprints/venus-os-dbus-ble-sensors-py
that referenced
this pull request
May 8, 2026
Document the NIST big-endian vs PyCryptodome little-endian counter quirk so future maintainers understand why the cryptography fallback drives ECB block-by-block instead of using modes.CTR directly. Link to the upstream PR (keshavdv/victron-ble#94) so we know when to revert the local modification and just ship the unmodified release. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add an optional fallback to PyCA's
cryptographylibrary for the single AES-128-CTR decryption call inDevice.decrypt. Behaviour is unchanged for users who already have PyCryptodome installed; the fallback only kicks in when PyCryptodome is unavailable.Motivation
On some embedded platforms — most concretely Victron's own Venus OS running on the Cerbo GX, where I've been deploying
victron_blefor fleet monitoring — thecryptographypackage ships with the base image (Victron's own services use it) but PyCryptodome does not, and binary wheels are not published on PyPI for the target architecture (armv7l). Installing PyCryptodome from source requiresgccplus Python development headers, neither of which is present on the device.Allowing
cryptographyas a backend letsvictron_blerun on these platforms with no native build step at install time.Implementation
try / except ImportErrorblocks setting_HAVE_*flags. An explicitImportErroris raised only if neither is available.cryptographypath drives AES-ECB manually and generates the keystream block-by-block from a little-endian-encoded counter. This is necessary becausecryptography.modes.CTRfollows NIST SP 800-38A and increments big-endian, whereas the Victron Instant Readout protocol usesCounter.new(128, initial_value=iv, little_endian=True)which increments little-endian. The two backends produce byte-identical output when both are installed.Device.decryptnow calls the backend-agnostic_aes_ctr_decrypthelper.requirements.txtis intentionally left untouched — PyCryptodome remains the default install dependency for everyone usingpip install victron-ble. Users who would rather avoid PyCryptodome can install with--no-depsand providecryptographythemselves.If you'd prefer, the next step could be to relax
requirements.txtso that PyCryptodome becomes one of two acceptable extras (e.g.pip install victron-ble[pycryptodome]orpip install victron-ble[cryptography]) — but I wanted to keep this PR minimal and let you decide that direction separately.Tests
Five new tests in
tests/test_base.py, written in the existing class-based style:test_at_least_one_backend_available— sanity check.test_known_vector— captured PyCryptodome output, runs against whichever backend is selected at runtime.test_short_ciphertext_padded_to_block— exercises_pkcs7_pad16for sub-block ciphertexts.test_backends_agree_single_block— when both libraries are installed, single-block output is byte-identical.test_backends_agree_multi_block— exercises 16 blocks of plaintext, catching any backend that mis-handles counter increments.test_backends_agree_across_le_carry— chooses anivthat triggers a low-byte LE carry on the first increment, which would diverge if the cryptography fallback used a big-endian counter.This last test caught a real bug in my first draft of the patch: a naive
cryptography.modes.CTR(nonce=iv.to_bytes(16, "little"))looked correct but disagreed with PyCryptodome past the first block, becausecryptographyincrements big-endian (per NIST) while PyCryptodome withlittle_endian=Trueincrements low byte first. The block-by-block ECB approach in this PR matches PyCryptodome byte-for-byte, including across the 8-bit and 16-bit carry boundaries.Local verification
Test plan for reviewers
pip install --no-deps victron-ble cryptography(no PyCryptodome) — verify decrypt still worksMade with Cursor