feat(cardano): Message signing #88

jaskp · 2023-12-28T16:44:58Z

No description provided.

common/protob/messages.proto

python/src/trezorlib/cardano.py

core/src/apps/cardano/layout.py

core/src/apps/cardano/helpers/utils.py

common/protob/messages-cardano.proto

core/src/apps/cardano/sign_message.py

python/src/trezorlib/cardano.py

core/src/apps/cardano/sign_message.py

davidmisiak

The ChunkIterator is an improvement I think 👍

Apart from the comments, there are some formatting errors reported by make pystyle_check.

core/src/apps/cardano/helpers/chunks.py

core/src/apps/cardano/helpers/utils.py

core/src/apps/cardano/addresses.py

core/src/apps/cardano/layout.py

davidmisiak · 2024-01-20T14:32:42Z

core/src/apps/cardano/layout.py

    displayed_bytes = first_chunk[:MAX_DISPLAYED_SIZE]
    bytes_optional_plural = "byte" if data_size == 1 else "bytes"
-    props: list[tuple[str, bytes | None]] = [
+    props: list[PropertyType] = [
        (
            f"{title} ({data_size} {bytes_optional_plural}):",
            displayed_bytes,
        )
    ]
    if data_size > MAX_DISPLAYED_SIZE:
        props.append(("...", None))


This means that we allow signing up to 1024 bytes without hashing, but we only display the first 56 bytes anyway. Is that the inteded behavior, or should we limit the maximum unhashed message length more (e.g. only the 160 bytes mentioned in the doc) but display all bytes?
cc @janmazak

Hmm, I think it would be best if we showed the whole ASCII message even if it is long.
For bytes in hex, showing the whole of the message does not really add security for most users --- they are likely to just be annoyed and skip it quickly. And the more paranoid users can avoid the feature (and ask us to show everything --- I doubt there will be demand for that, but we will see) or only use it when long messages are hashed.

core/src/apps/cardano/sign_message.py

core/src/apps/cardano/helpers/chunks.py

janmazak · 2024-01-28T10:42:49Z

common/protob/messages-cardano.proto

+ * @next CardanoMessageItemAck
+ */
+message CardanoSignMessageInit {
+    required uint32 protocol_magic = 1;                           // network's protocol magic


What's the purpose of this? I don't think we are doing anything with Byron addresses for message signing. And isn't network id part of CardanoAddressParametersType? (If not, why is it required while the address is optional?)

@janmazak Network id isn't part of CardanoAddressParametersType, but you are right in that protocol magic and network id should be as optional as address parameters.

As for byron address, our discussion about them in slack hasn't reached a conclusion yet, please chime in

janmazak · 2024-01-28T10:44:44Z

core/src/apps/cardano/layout.py

+        props = _get_data_chunk_props(
+            "Empty message", payload_first_chunk, payload_size
+        )
+    elif not prefer_hex_display and is_unambiguous_ascii(payload_first_chunk):


I'm wary of doing any guesswork here. Why don't we let the client choose the "display mode" (ascii/hex) explicitly and just validate it's OK?

core/src/apps/cardano/helpers/chunks.py

janmazak · 2024-01-28T10:58:24Z

core/src/apps/cardano/layout.py

    displayed_bytes = first_chunk[:MAX_DISPLAYED_SIZE]
    bytes_optional_plural = "byte" if data_size == 1 else "bytes"
-    props: list[tuple[str, bytes | None]] = [
+    props: list[PropertyType] = [
        (
            f"{title} ({data_size} {bytes_optional_plural}):",
            displayed_bytes,
        )
    ]
    if data_size > MAX_DISPLAYED_SIZE:
        props.append(("...", None))


Hmm, I think it would be best if we showed the whole ASCII message even if it is long.
For bytes in hex, showing the whole of the message does not really add security for most users --- they are likely to just be annoyed and skip it quickly. And the more paranoid users can avoid the feature (and ask us to show everything --- I doubt there will be demand for that, but we will see) or only use it when long messages are hashed.

janmazak · 2024-01-28T11:38:54Z

common/protob/messages-cardano.proto

+    repeated uint32 signing_path = 3;                             // BIP-32-style path to derive the signing key from master node
+    required uint32 payload_size = 4;                             // size of the payload to be signed
+    required bool hash_payload = 5;                               // whether to hash the payload before signing
+    repeated uint32 key_path = 6;                                 // mutually exclusive with address_parameters


This field seems not really useful --- if we use key hash for the address field, it will be the key used to sign the msg, so it seems we can just use signing_path?

I assumed it was implied by this part of the doc that we want to separate the two paths.

Since the address field might be unrelated to the key used for signing and it seems impossible to completely determine what the existing applications use, we don’t want to impose restrictions on the derivation paths used.

Did I understand it wrong?

The sentence you quote is somewhat ambiguous. What I meant was that in general according to CIP-8 it might be unrelated and some historical use could be there in case of address. The usage of just key hash in the address field is new (AFAIK --- discussed with Ryan, gitmatchtl etc.), so we do not need to support arbitrary keys different from signing keys there. I'd be OK if you kept it implemented like this (if it works now). What makes sense to me is not to display the key path twice if it is the same --- i.e. if there is just a key hash in the address field and the key is used for signing, only show that once (I'm not sure at which place, because I don't know how people use the arbitrary msg signing feature, but I'd perhaps put more emphasis on the key that is being used to sign and always explicitly say what that key path is, and hide info about the address field from the user if it does not say anything new.)

I guess I won't add "arbitrary key in addr field (different from signing key)" to Ledger because it would cut into APDU space. I'll see today/tomorrow.

janmazak

Do we always show the message hash? (Even for ascii messages that are signed raw, not as hash.) We were told that the hash is typically displayed by dapp/sw wallet, so it's useful to the users to compute and show it.

jaskp · 2024-01-31T12:34:25Z

Do we always show the message hash? (Even for ascii messages that are signed raw, not as hash.) We were told that the hash is typically displayed by dapp/sw wallet, so it's useful to the users to compute and show it.

Yes

github-actions · 2024-01-31T21:35:07Z

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T2B1 Safe 3	3280	test(screens) main(screens)	2724
T3T1	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

github-actions · 2024-01-31T21:35:08Z

legacy UI changes	device test(screens) main(screens)

davidmisiak · 2024-02-02T14:39:50Z

core/src/apps/cardano/layout.py

+        if not is_unambiguous_ascii(payload_first_chunk):
+            raise ProcessError(
+                "Payload cannot be decoded as ASCII or its decoding leads to a visually ambiguous string"
+            )


I think it would be better to do this validation in sign_message.py (and then just assert it here) - currently, there is no validation performed as late as when displaying something by the UI code.

davidmisiak · 2024-02-02T16:21:01Z

common/tests/fixtures/cardano/sign_message.json

+      }
+    }
+  ]
+}


Please also add a long ascii message (so that we have a UI test for that).

gabrielKerekes · 2024-02-12T11:03:01Z

core/src/apps/cardano/sign_message.py

+    )
+
+    return CardanoSignMessageFinished(
+        signature=signature, address=address, pub_key=node.public_key()


🙏

Suggested change

signature=signature, address=address, pub_key=node.public_key()

signature=signature, address=address, pub_key=remove_ed25519_prefix(node.public_key())

Show signing path, validate with keychain, show address parameters and increase max displayed bytes of first payload chunk unless signing hash. Also add issue number to changelog.

This makes the behavior more consistent with Ethereum message signing.

This ends up being useful for software wallets.

jaskp requested a review from davidmisiak December 28, 2023 16:44

jaskp self-assigned this Dec 28, 2023

jaskp force-pushed the feat/cardano/message-signing branch 2 times, most recently from a0427cb to fc9f2b3 Compare December 28, 2023 20:24

davidmisiak requested changes Jan 4, 2024

View reviewed changes

jaskp requested a review from davidmisiak January 13, 2024 22:33

jaskp commented Jan 15, 2024

View reviewed changes

core/src/apps/cardano/sign_message.py Outdated Show resolved Hide resolved

davidmisiak reviewed Jan 20, 2024

View reviewed changes

jaskp force-pushed the feat/cardano/message-signing branch from 218b6a0 to 9d44b58 Compare January 27, 2024 15:08

jaskp commented Jan 27, 2024

View reviewed changes

core/src/apps/cardano/helpers/chunks.py Outdated Show resolved Hide resolved

janmazak reviewed Jan 28, 2024

View reviewed changes

janmazak reviewed Jan 31, 2024

View reviewed changes

jaskp force-pushed the feat/cardano/message-signing branch 3 times, most recently from 0865cb3 to 3a13795 Compare January 31, 2024 21:34

jaskp force-pushed the feat/cardano/message-signing branch from d14b219 to 17be8c3 Compare January 31, 2024 22:16

jaskp changed the base branch from main to master January 31, 2024 22:17

davidmisiak approved these changes Feb 2, 2024

View reviewed changes

gabrielKerekes reviewed Feb 12, 2024

View reviewed changes

jaskp force-pushed the feat/cardano/message-signing branch 3 times, most recently from e3c4907 to 8381708 Compare February 13, 2024 16:10

jaskp force-pushed the feat/cardano/message-signing branch from 8381708 to 8ceefbf Compare February 27, 2024 14:40

davidmisiak force-pushed the feat/cardano/message-signing branch from 8ceefbf to b3b317a Compare May 16, 2024 23:20

jaskp added 2 commits May 23, 2024 13:43

feat(cardano): Message signing

8c0fddd

feat(cardano): Show path, address params and longer payload chunk

8412b16

Show signing path, validate with keychain, show address parameters and increase max displayed bytes of first payload chunk unless signing hash. Also add issue number to changelog.

jaskp and others added 4 commits May 23, 2024 13:45

fix(cardano): Change display_ascii flag to prefer_hex_display

c6af57a

This makes the behavior more consistent with Ethereum message signing.

fix(cardano): Return pubkey

3bc70a8

This ends up being useful for software wallets.

chore(cardano): add missing translations

12d6454

chore(cardano): update UI fixtures

4bb7871

davidmisiak force-pushed the feat/cardano/message-signing branch from b3b317a to 4bb7871 Compare May 23, 2024 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cardano): Message signing #88

feat(cardano): Message signing #88

jaskp commented Dec 28, 2023

davidmisiak left a comment

davidmisiak Jan 20, 2024

janmazak Jan 28, 2024

janmazak Jan 28, 2024

jaskp Jan 29, 2024

janmazak Jan 28, 2024

janmazak Jan 28, 2024

janmazak Jan 28, 2024

jaskp Jan 29, 2024

janmazak Jan 30, 2024

janmazak Jan 30, 2024

janmazak left a comment

jaskp commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading

davidmisiak Feb 2, 2024

davidmisiak Feb 2, 2024

gabrielKerekes Feb 12, 2024

	signature=signature, address=address, pub_key=node.public_key()
	signature=signature, address=address, pub_key=remove_ed25519_prefix(node.public_key())

feat(cardano): Message signing #88

Are you sure you want to change the base?

feat(cardano): Message signing #88

Conversation

jaskp commented Dec 28, 2023

davidmisiak left a 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

Choose a reason for hiding this comment

janmazak left a comment

Choose a reason for hiding this comment

jaskp commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 • edited Loading

github-actions bot commented Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading