bootloader: add OP_HARDWARE api endpoint #1347

Beerosagos · 2024-12-23T14:19:35Z

This will allow the bbapp to flash different FW versions, depending on the PCB variants. The initial implementation will respond with a single byte that identifies the Secure Chip: 0 for ATECC, 1 for Optiga.

Note: Based on the changes from #1341

benma

Obligatory 😄

benma · 2024-12-23T20:18:40Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

@@ -94,6 +94,15 @@ def versions(self) -> typing.Tuple[int, int]:
        firmware_v, signing_pubkeys_v = struct.unpack("<II", response[:8])
        return firmware_v, signing_pubkeys_v

+    def hardware(self) -> str:


Please no stringly types. An enum like this would be better imho:

https://github.com/BitBoxSwiss/bitbox02-firmware/blob/master/py/bitbox02/bitbox02/communication/bitbox_api_protocol.py#L187

Also this should return the ATECC variant if the bootloader version does not support this endpoint. Right now this call fails for old bootloaders.

benma · 2024-12-23T20:28:55Z

src/bootloader/bootloader.c

+    uint8_t type = 0;
+    if (memory_get_securechip_type() == MEMORY_SECURECHIP_TYPE_OPTIGA) {
+        type = 1;
+    }
+    memcpy(output + BOOT_OP_LEN, &type, 1);
+    return BOOT_OP_LEN + 1;


The second byte of the output for all api commands is the success/error byte, but it's missing here. The bootloader ._query() call checks the second byte for error. It seems that you put the hardware type into this byte, so the Python client does not actually work as expected and raise an exception on the Optiga chip (maybe, haven't tested, I might be missing sth).

You should set it by using return _report_status(OP_STATUS_OK, output) + ...;

The memcpy of a single byte is a bit awkward. You can just do output[2] = type

Looking at some other api calls, _api_set_show_firmware_hash is borked - it says it returns a disabled/enabled byte and sets it like this

output[BOOT_OP_LEN] = data.fields.show_firmware_hash; return _report_status(result, output) + 1;

But _report_status call overwrites the second byte with the success byte. The client libraries also don't return any result here. Could you also add a commit or PR that cleans this up?

_api_versions is also a bit borked: returns the size of three versions, but only returns two versions 🤦

Looking at some other api calls, _api_set_show_firmware_hash is borked - it says it returns a disabled/enabled byte and sets it like this

output[BOOT_OP_LEN] = data.fields.show_firmware_hash; return _report_status(result, output) + 1;

But _report_status call overwrites the second byte with the success byte. The client libraries also don't return any result here. Could you also add a commit or PR that cleans this up?

I don't think this is right. BOOT_OP_LEN is 2, so the show_firmware_hash result is in the 3rd byte of the array. Am I missing something?

_api_versions is also a bit borked: returns the size of three versions, but only returns two versions 🤦

Right 😅 The Py client is expecting only two versions. For backward compatibility, I guess it's better to change the return size instead of adding the third version. Wdyt?

[EDIT] On the other side, we need to be able to tell the bootloader version, I guess. Also because calling the OP_HARDWARE on an older bootloader shows an error on the display, so it would be better to avoid calling it, if possible.

The bootloader version is available through the HID descriptor serial field.

Fixed in a second commit

Beerosagos · 2025-01-07T16:11:57Z

Obligatory 😄

Let's make a PR template that includes a changelog checkbox, pls pls pls 🙏 😂

benma · 2025-01-14T08:36:58Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

+                return Hardware.OPTIGA
+        except:
+            # If the bootloader does not support the call, it must have the ATECC SC.
+            pass


You said the device will show an error on the screen in this case. Better to check the bootloader version as you suggested in the other comment.

While you are at it, you should bump the bootloader version to v1.1.0 then, as it's a new API call and not a patch.

benma · 2025-01-14T08:37:21Z

py/bitbox02/bitbox02/communication/bitbox_api_protocol.py

@@ -191,6 +191,13 @@ class BitBox02Edition(enum.Enum):
    BTCONLY = "btconly"


+class Hardware(enum.Enum):


why is this here instead of in bootloader.py?

benma · 2025-01-14T08:48:47Z

CHANGELOG.md

@@ -150,6 +150,9 @@ customers cannot upgrade their bootloader, its changes are recorded separately.

 ## Bootloader

+### [Unreleased]
+- Implement OP_HARDWARE endpoint, to identify the SC model


SC=secure chip. Imho abbreviations just make it harder to read, not everyone knows what it means.

Beerosagos · 2025-01-14T16:15:33Z

rebased

Beerosagos · 2025-01-14T16:52:48Z

@benma this should be ready for a new review, PTAL 🙏

benma · 2025-01-15T10:17:42Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

+        Returns (hardware variant).
+        """
+        # Previous bootloader versions does not support the call and have ATECC SC.
+        if self.version > "1.0.6":


">= 1.1.0".

I did this initially, but it wasn't working well with dev version, e.g. 1.1.0-pre+48561ef480.dev >= 1.1.0, seemed to return false in my tests. I'll double check, tho.

I checked again, it doesn't work for versions 1.1.0 with prerelease. I'd keep > 1.0.6, if you agree

Check this out:

bitbox02-firmware/py/bitbox02/bitbox02/communication/bitbox_api_protocol.py

Lines 557 to 562 in c016621

# Delete the prelease part, as it messes with the comparison (e.g. 3.0.0-pre < 3.0.0 is

# True, but the 3.0.0-pre has already the same API breaking changes like 3.0.0...).

self.version = semver.VersionInfo(

self.version.major, self.version.minor, self.version.patch, build=self.version.build

)

The >1.0.6 way does not work consistently, so imho better to apply the above here as well.

Ah, that's nice! But why >1.0.6 doesn't work consistently? Anyway, your suggestion sounds better 👍

Fixed. I found a more readble/compact way to remove the pre-release self.version.replace(prerelease=None). Should I update also the code you linked while I'm here?

Ah, that's nice! But why >1.0.6 doesn't work consistently? Anyway, your suggestion sounds better 👍

1.0.7 could be released that patches something in 1.0.6 before 1.1.0 goes out, for example. Less likely in the bootloader, but did happen in the firmware a few times.

@benma Good point, I didn't think about it. Anyway, I updated the code. Hopefully this could be the last round of review 🙏

benma · 2025-01-15T10:17:54Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

+        # Previous bootloader versions does not support the call and have ATECC SC.
+        if self.version > "1.0.6":
+            response = self._query(b"W")
+            if response[:2] == b"\x01":


how can two bytes equal one byte? 😁

Shoot, how did I miss this? 🙈

Lol, it worked by coincidence because the query call already strips the first 2 bytes, so response[:2] was actually 1 byte long

The endpoint provides firmware and signing pubkeys versions, but the function was returning the lenght of three versions struct, instead of two.

benma

Looking good, but I still want to test it. Left a tiny nit.

benma · 2025-01-16T10:35:59Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

@@ -75,6 +84,9 @@ def __init__(self, transport: TransportLayer, device_info: DeviceInfo):
            "bb02btc-bootloader": SIGDATA_MAGIC_BTCONLY,
            "bitboxbase-bootloader": SIGDATA_MAGIC_BITBOXBASE_STANDARD,
        }.get(device_info["product_string"])
+        self.version = parse_device_version(device_info["serial_number"])
+        # we remove the prerelease as it messes up same-version comparisons.


I thought the other comment was clearer 😇

# Delete the prelease part, as it messes with the comparison (e.g. 3.0.0-pre < 3.0.0 is # True, but the 3.0.0-pre has already the same API breaking changes like 3.0.0...).

Copied the same comment here 👍

benma

tACK

benma · 2025-01-21T11:03:16Z

py/bitbox02/bitbox02/bitbox02/bootloader.py

@@ -94,6 +107,17 @@ def versions(self) -> typing.Tuple[int, int]:
        firmware_v, signing_pubkeys_v = struct.unpack("<II", response[:8])
        return firmware_v, signing_pubkeys_v

+    def hardware(self) -> Hardware:


When adding the same API to the Go package I realized that this endpoint could return more HW info in the future, so having Hardware==Securechip like here will be a breaking change when that happens. It's not that important as I think there is no production use of the bootloader Python API, but technically Hardware should be an object (TypedDict?) with a SecureChipModel enum inside.

BitBoxSwiss/bitbox02-api-go#116

Done, I also updated the check of the response to print unexpected values, so it behaves the same as your PR

This will allow the bbapp to flash different FW versions, depending on the PCB variants. The initial implementation will respond with a single byte that identifies the Secure Chip: 0 for ATECC, 1 for Optiga.

benma

LGTM thanks!

Beerosagos force-pushed the bootloader-sc branch 2 times, most recently from 16ab407 to fbf257d Compare December 23, 2024 14:52

Beerosagos marked this pull request as ready for review December 23, 2024 15:11

Beerosagos requested a review from benma December 23, 2024 15:11

benma requested changes Dec 23, 2024

View reviewed changes

Beerosagos force-pushed the bootloader-sc branch from fbf257d to ff9d269 Compare January 8, 2025 15:25

benma requested changes Jan 14, 2025

View reviewed changes

benma reviewed Jan 14, 2025

View reviewed changes

Beerosagos force-pushed the bootloader-sc branch 2 times, most recently from d977ff5 to 48561ef Compare January 14, 2025 16:15

benma requested changes Jan 15, 2025

View reviewed changes

Beerosagos force-pushed the bootloader-sc branch 3 times, most recently from 21de44c to 941c857 Compare January 15, 2025 11:09

Beerosagos requested a review from benma January 15, 2025 11:10

Beerosagos force-pushed the bootloader-sc branch from 941c857 to 626ec5d Compare January 15, 2025 11:37

bootloader: fix wrong return for versions endpoint

d94935e

The endpoint provides firmware and signing pubkeys versions, but the function was returning the lenght of three versions struct, instead of two.

Beerosagos force-pushed the bootloader-sc branch 2 times, most recently from 53efaf9 to fc64fee Compare January 15, 2025 15:17

benma reviewed Jan 16, 2025

View reviewed changes

Beerosagos force-pushed the bootloader-sc branch from fc64fee to 6428c9e Compare January 16, 2025 12:51

benma approved these changes Jan 21, 2025

View reviewed changes

benma reviewed Jan 21, 2025

View reviewed changes

Beerosagos force-pushed the bootloader-sc branch from 6428c9e to b16fa93 Compare January 21, 2025 15:40

bootloader: add OP_HARDWARE api endpoint

699fba6

This will allow the bbapp to flash different FW versions, depending on the PCB variants. The initial implementation will respond with a single byte that identifies the Secure Chip: 0 for ATECC, 1 for Optiga.

Beerosagos force-pushed the bootloader-sc branch from b16fa93 to 699fba6 Compare January 21, 2025 15:46

benma approved these changes Jan 22, 2025

View reviewed changes

Beerosagos merged commit 10f2229 into BitBoxSwiss:master Jan 22, 2025
3 checks passed

		@@ -191,6 +191,13 @@ class BitBox02Edition(enum.Enum):
		BTCONLY = "btconly"


		class Hardware(enum.Enum):


	# Delete the prelease part, as it messes with the comparison (e.g. 3.0.0-pre < 3.0.0 is
	# True, but the 3.0.0-pre has already the same API breaking changes like 3.0.0...).
	self.version = semver.VersionInfo(
	self.version.major, self.version.minor, self.version.patch, build=self.version.build
	)

bootloader: add OP_HARDWARE api endpoint #1347

bootloader: add OP_HARDWARE api endpoint #1347

Conversation

Beerosagos commented Dec 23, 2024 • edited Loading

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

benma Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beerosagos Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beerosagos commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beerosagos commented Jan 14, 2025

Beerosagos commented Jan 14, 2025

Choose a reason for hiding this comment

Beerosagos Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma left a comment

Choose a reason for hiding this comment

Beerosagos commented Dec 23, 2024 •

edited

Loading

benma Dec 23, 2024 •

edited

Loading

benma Dec 23, 2024 •

edited

Loading

Beerosagos Jan 8, 2025 •

edited

Loading

Beerosagos Jan 15, 2025 •

edited

Loading