Add registerdescriptors command for registering a descriptor with a device

[achow101]

The Ledger, BitBox02, Jade, and Coldcard all support registering descriptors with the device to enable signing txs involving complex scripts. This PR adds a registerdescriptor command which takes a name and the descriptor to register. The command returns a registration which may contain data that must be provided to the device at a later time to remind it of the registered descriptor.

For the BiBox02, Jade, and Coldcard, this command returns an empty string for the registration as these devices store the registration and do not need a reminder.

For the Ledger, the registration is the HMAC that the device returns.

Some devices take a descriptor, others take a BIP 388 policy that is stuffed into their protocol. The Descriptor and PubkeyProvider classes are modified to allow produce BIP 388 compatible strings that can be provided as needed.

Each device does different validation of the descriptor it is provided. We will not do any validation - the user may provide a descriptor that a device refuses to register and any such registration errors will be propagated from the device.

The test case includes a descriptor that should work on all devices.

[Sjors]

Concept ACK

[Sjors]

In 8db5557 ledger: Implement register_descriptor: in #791 I added the policy name to the registration result, so you don't have to provide it again with signing and address display.

A simple [hmac length]hmac bytes[name length][name bytes] serialization means Ledger can change the length later, devices that only need a name can easily omit it, and we add additional fields later.

[achow101]

Still thinking on what we should actually be returning.

[Sjors]

I just updated #841 to be based on this PR. For now it adds one commit to add the name. It's sloppy, but should go away depending on what you decide here. I do think it's nice to avoid --policy-name.

[Sjors]

In 3f5da8c bitbox02: Implement register_descriptor: why not None? (or echo the name, see my other suggestion)

[Sjors]

This should fix the ledger tests:

diff --git a/test/data/speculos-automation.json b/test/data/speculos-automation.json
index 2b273e7..5618fef 100644
--- a/test/data/speculos-automation.json
+++ b/test/data/speculos-automation.json
@@ -40,12 +40,5 @@
         },
         {
-            "regexp": "^. of . Multisig$",
-            "actions": [
-                [ "button", 2, true ],
-                [ "button", 2, false ]
-            ]
-        },
-        {
-            "regexp": "^(Address|Review|Account name|Amount|External amounts|You spend|You receive|Fee|Confirm|The derivation|Derivation path|Reject if|The change path|Change path|Register wallet|Policy map|Key|Path|Public key|Our key|Their key|Unspendable key|Spend from|Spending policy|Primary spending path|Spending path|Transaction output|Wallet name|Wallet policy|Descriptor template|Verify [Bb]itcoin|Output|Warning).*",
+            "regexp": "^(Address|Review|Account name|Amount|External amounts|You spend|You receive|Fee|Confirm|The derivation|Derivation path|Reject if|The change path|Change path|Register wallet|Policy map|Key|Path|Public key|Our key|Their key|Unspendable key|Spend from|Spending policy|Primary spending path|Spending path|Transaction output|From account|Wallet name|Wallet policy|Descriptor template|Verify [Bb]itcoin|Output|Warning).*",
             "actions": [
                 [ "button", 2, true ],

This fix should prevent a double click, tested on both legacy and new app. Related: 5f0edff, 9ba9a17, #812, #818

[achow101]

I've decided to create a new RegisteredDescriptor serialized string. It's the base64 of a compact size version number, length prefixed descriptor string, length prefixed device type string, and length prefixed registration data (hmac on the ledger, empty string for everything else).

[Sjors]

ACK a7a69f8

Lightly reviewed the code. I mainly tested by rebasing #841 and #792 on top of this.

In 6a5a496 bitbox02: Implement register_descriptor: it would save me a refactor commit if you put register_descriptor up to the btc.BTCScriptConfig call in a helper method.

[Sjors]

In e2bd86e descriptors: Actually parse the derivation path: could give this a bit more teeth:

diff --git a/hwilib/descriptor.py b/hwilib/descriptor.py
index 0e85311..f590d67 100644
--- a/hwilib/descriptor.py
+++ b/hwilib/descriptor.py
@@ -168,7 +168,17 @@ class PubkeyProvider(object):
             pubkey = s[:slash_idx]
             path_str = s[slash_idx + 1:]
-            ranged = path_str.endswith("*")
-            if ranged:
+
+            if not path_str:
+                raise ValueError("Derivation path cannot be empty")
+
+            if path_str == "*":
+                ranged = True
+                path_str = ""
+            elif path_str.endswith("/*") and path_str.count("*") == 1:
+                ranged = True
                 path_str = path_str[:-2]
+            elif "*" in path_str:
+                raise ValueError("Wildcard must be the final derivation path element")
+
             if len(path_str) > 0:
                 deriv_path = parse_multipath(path_str)
diff --git a/hwilib/key.py b/hwilib/key.py
index 506fb1a..5ce2fb3 100644
--- a/hwilib/key.py
+++ b/hwilib/key.py
@@ -335,10 +335,24 @@ def _parse_path(nstr: str, allow_multipath: bool) -> List[List[int]]:

     def str_to_harden(x: str) -> int:
+        original = x
+        hardened = False
+
         if x.startswith("-"):
-            return H_(abs(int(x)))
-        elif x.endswith(("h", "'")):
-            return H_(int(x[:-1]))
-        else:
-            return int(x)
+            hardened = True
+            x = x[1:]
+        elif x.endswith(("h", "H", "'")):
+            hardened = True
+            x = x[:-1]
+
+        # Avoid accepting syntax supported by int() but not by BIP32 paths,
+        # such as whitespace, signs, underscores, and non-ASCII digits.
+        if not x or not x.isascii() or not x.isdecimal():
+            raise ValueError(f"Invalid BIP32 path index: {original}")
+
+        index = int(x)
+        if index >= HARDENED_FLAG:
+            raise ValueError(f"BIP32 path index out of range: {original}")
+
+        return H_(index) if hardened else index

     def parse_index(x: str, seen_multipath: bool) -> Tuple[List[int], bool]:
@@ -354,5 +368,8 @@ def _parse_path(nstr: str, allow_multipath: bool) -> List[List[int]]:
                 raise ValueError(f"Invalid multipath specification, less than 2 indexes specified: {x}")
             seen_multipath = True
-            return [str_to_harden(p) for p in mp], seen_multipath
+            indexes = [str_to_harden(p) for p in mp]
+            if len(set(indexes)) != len(indexes):
+                raise ValueError(f"Duplicate indexes in multipath specification: {x}")
+            return indexes, seen_multipath
         return [str_to_harden(x)], seen_multipath

@@ -373,5 +390,5 @@ def parse_path(nstr: str) -> List[int]:
     """
     Convert BIP32 path string to list of uint32 integers with hardened flags.
-    Several conventions are supported to set the hardened flag: -1, 1', 1h
+    Several conventions are supported to set the hardened flag: -1, 1', 1h, 1H

     e.g.: "0/1h/1" -> [0, 0x80000001, 1]
@@ -379,5 +396,5 @@ def parse_path(nstr: str) -> List[int]:
     :param nstr: path string
     :return: list of integers
-    :raises ValueError: If the path contains any invalid characters
+    :raises ValueError: If an index has invalid syntax or is outside the uint31 range
     """
     return [i[0] for i in _parse_path(nstr, False)]
diff --git a/test/test_bip32.py b/test/test_bip32.py
index 5a88d82..b5e3ce7 100755
--- a/test/test_bip32.py
+++ b/test/test_bip32.py
@@ -6,4 +6,5 @@
 from hwilib.key import (
     ExtendedKey,
+    parse_multipath,
     parse_path,
 )
@@ -123,4 +124,28 @@ class TestBIP32(unittest.TestCase):
                 self.assertEqual(xprv_der.to_string(), child_xpub)

+    def test_parse_path_validation(self):
+        self.assertEqual(
+            parse_path("m/0/2147483647/2147483647h/-1/1H"),
+            [0, 0x7fffffff, 0xffffffff, 0x80000001, 0x80000001],
+        )
+
+        for path in (
+            "m/",
+            "+1",
+            "1_0",
+            " 1",
+            "١",
+            "2147483648",
+            "2147483648h",
+            "-2147483648",
+        ):
+            with self.subTest(path=path):
+                with self.assertRaises(ValueError):
+                    parse_path(path)
+
+    def test_parse_multipath_validation(self):
+        with self.assertRaisesRegex(ValueError, "Duplicate indexes"):
+            parse_multipath("<0;0>")
+

 if __name__ == "__main__":
diff --git a/test/test_descriptor.py b/test/test_descriptor.py
index 828e246..ec339d8 100755
--- a/test/test_descriptor.py
+++ b/test/test_descriptor.py
@@ -4,4 +4,5 @@ from hwilib.descriptor import (
     parse_descriptor,
     MultisigDescriptor,
+    PubkeyProvider,
     SHDescriptor,
     TRDescriptor,
@@ -278,4 +279,10 @@ class TestDescriptor(unittest.TestCase):
         self.assertEqual(d, desc.to_string_no_checksum())

+    def test_invalid_derivation_path_wildcards(self):
+        for key_expr in ("xpub/", "xpub/a*", "xpub/0*", "xpub/*/0", "xpub/**"):
+            with self.subTest(key_expr=key_expr):
+                with self.assertRaises(ValueError):
+                    PubkeyProvider.parse(key_expr, 0)
+
     def test_invalid_multipath_descriptors(self):
         with self.assertRaisesRegex(ValueError, "Cannot have multiple multipath specifiers"):
@@ -289,9 +296,9 @@ class TestDescriptor(unittest.TestCase):
         with self.assertRaisesRegex(ValueError, "Invalid multipath specification, less than 2 indexes specified: <>"):
             parse_descriptor("wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<>/*)")
-        with self.assertRaisesRegex(ValueError, "invalid literal for int()"):
+        with self.assertRaisesRegex(ValueError, "Invalid BIP32 path index"):
             parse_descriptor("wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0>/*)")
         with self.assertRaisesRegex(ValueError, "Invalid multipath specification, missing trailing '>'"):
             parse_descriptor("wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0/*)")
-        with self.assertRaisesRegex(ValueError, "invalid literal for int()"):
+        with self.assertRaisesRegex(ValueError, "Invalid BIP32 path index"):
             parse_descriptor("wpkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;>/*)")

[achow101]

I don't think these are necessary, and your suggestions are both too strict and deviate from the specs. Not all descriptors must have derivation paths. H is not an allowed hardened indicator.

[Sjors]

H is not an allowed hardened indicator.

Oops.

And mixed notations isn't allowed either (at least in Bitcoin Core). So yeah, ignore.

The ledger hmac is based on the exact string, but it's probably better to rely on the client to be very strict about it, rather than enforce a canonical variant on the HWI side. And worst case it's easy to register the policy again.

[Sjors]

In d9f384e coldcard: Implement register_descriptor:

if self.device.is_simulator:
           self.device.send_recv(CCProtocolPacker.sim_keypress(b'y'))

[Sjors]

You'll need 12ccec2 too.

[Sjors]

While testing I noticed this PR emits @0/<0;1>/* rather than @0/**. The latter is a bit easier on the eyes when checking on a device, but the BIP doesn't (strongly) recommend one or the other. Annoyingly a Ledger hmac generated with one form won't work with the other form, so whatever we pick has to stay that way.

[achow101]

While testing I noticed this PR emits @0/<0;1>/* rather than @0/**.

This is intentional. I consider /** to be a bad syntax. Derivation paths should be explicitly given.