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())

[no changelog]

- change button label and gradient - add success screen after changing device name [no changelog]

- use header builder functions where possible - update fixtures [no changelog]

[no changelog]

- update result enum with new options [no changelog]

[no changelog]

- align LED colors with firmware [no changelog]

[no changelog]

It is currently unused, since `should_show_pairing_dialog` is always `True`. [no changelog]

[no changelog]

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.

[no changelog]

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 2 times, most recently from b3b317a to 4bb7871 Compare May 23, 2024 12:22

jaskp force-pushed the feat/cardano/message-signing branch from 4bb7871 to a162d8d Compare November 29, 2024 13:39

bieleluk and others added 29 commits August 23, 2025 15:43

chore(core/eckhart): enable menu separators by default

73f3a6d

[no changelog]

chore(core/eckhart): update apply settings label flow

9ac1dbd

- change button label and gradient - add success screen after changing device name [no changelog]

chore(core/eckhart): extend header buttons

d8cff7a

- use header builder functions where possible - update fixtures [no changelog]

chore(core/eckhart): device menu translations

97396ff

[no changelog]

chore(core/eckhart): update device menu about screen

18b2005

[no changelog]

chore(core/eckhart): extend device menu FwUI param list

9dfa7f4

- update result enum with new options [no changelog]

chore(core): add new menu length

6e4b343

[no changelog]

chore(core): add secmon keys to trezorlib

a484420

[no changelog]

chore(core): add LED enable/disable functionality

eecd1e8

- align LED colors with firmware [no changelog]

refactor(python): refactor THP-related exception types

c693963

[no changelog]

fix(core): correct ticks_diff arguments' order

dc82cd1

[no changelog]

chore(core): exclude logging on non-debug builds

c95f5b5

[no changelog]

refactor(core): inline THP PairingContext confirmation methods

af2e787

[no changelog]

chore(core): remove thp.pairing._skip_pairing_dialog()

d680f9e

It is currently unused, since `should_show_pairing_dialog` is always `True`. [no changelog]

chore(core): move show_pairing_dialog() to trezor.wire.thp.ui module

ff18b00

[no changelog]

feat(cardano): Message signing

d5ebdd8

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

13cf5c3

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.

fix(cardano): Change display_ascii flag to prefer_hex_display

6d42732

This makes the behavior more consistent with Ethereum message signing.

fix(cardano): Return pubkey

5bc7576

This ends up being useful for software wallets.

chore(cardano): add missing translations

2703ba6

chore(cardano): update UI fixtures

063b621

chore(cardano): changes after rebase

2a3153c

feat(cardano): stop hashing in message signing

7311704

feat(cardano): replace chunking with length+offset

5343ea9

feat(cardano): reject probable hashes

f1c4d45

fix: changelog formatting

0f94f34

chore(cardano): rename payload

e1ae222

[no changelog]

fix(cardano): stop trimming long messages

ed8cbe5

[no changelog]

chore(tests): update fixtures

30eb997

[no changelog]

ibz force-pushed the feat/cardano/message-signing branch from 6fb97f0 to 30eb997 Compare August 25, 2025 11:23

	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

Uh oh!

Conversation

jaskp commented Dec 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidmisiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janmazak left a comment

Choose a reason for hiding this comment

Uh oh!

jaskp commented Jan 31, 2024

Uh oh!

github-actions bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Jan 31, 2024 •

edited

Loading