-
-
Notifications
You must be signed in to change notification settings - Fork 673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Nostr #4436
base: main
Are you sure you want to change the base?
Introduce Nostr #4436
Conversation
|
de9d4e4
to
bd13fed
Compare
9ef8c6a
to
feabb03
Compare
We should check that the Also I prefer changing the CLI interface from using Otherwise great work! 🎉 |
core/src/apps/bitcoin/keychain.py
Outdated
@@ -91,6 +91,9 @@ class MsgWithAddressScriptType(Protocol): | |||
# SLIP-44 coin type for all Testnet coins | |||
SLIP44_TESTNET = const(1) | |||
|
|||
# SLIP-44 "coin type" for Nostr | |||
SLIP44_NOSTR = const(1237) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this is not used anywhere.
core/src/apps/nostr/sign_event.py
Outdated
content = msg.content | ||
|
||
node = keychain.derive(address_n) | ||
pk = node.public_key()[-32:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this? Don't we care about the public key parity? Or does nostr use x-only keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this? Don't we care about the public key parity? Or does nostr use x-only keys?
From what I understand, Nostr uses X-only keys, indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it is not guaranteed that keychain.derive
returns an x-only key, right?
I think we need to convert the key if a non x-only key is returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't conversion to x-only basically dropping the first byte if the key is 33 bytes long? Which is the same as taking the last 32 bytes always?
d66d0af
to
8bc5409
Compare
Yup! dc1a257#diff-3dd8aea4110592e07746c9d3233736246c7083e001c9d29db7c1b03f7423f93bR19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some points for discussion.
|
||
|
||
@pytest.mark.setup_client(mnemonic=LEAD_MONKEY_MNEMONIC) | ||
def test_sign_event_lead_monkey(client: Client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a better way to write these tests:
LEAD_MONKEY = pytest.mark.setup_client(...)
WHAT_BLEAK = pytest.mark.setup_client(...)
VECTORS = ( # pubkey_hex
pytest.param("cafecacebeef", mark=LEAD_MONKEY),
pytest.param(...)
)
@pytest.mark.parametrize("pubkey_hex", VECTORS)
def test_get_pubkey(client, pubkey_hex):
...
dtto for sign_event
except i'd prefer also hardcoding the signature bytes, in addition to verifying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7f493ea#diff-1a61ba923388ca946c730e9cec2ca549a97e355ff7a53673617057d39f95f0f5R44
The signature cannot be hardcoded because every time an event is signed it gets a different signature. That is the reason to use verify
rather than comparing to a known signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signature cannot be hardcoded because every time an event is signed it gets a different signature.
what is the source of this nondeterminism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a feature of ECDSA - a random value is included. This is to prevent deriving of the private key if you have many signatures that you can analyze.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what RFC 6979 is for -- i was under the impression that this is built into ecdsa signing on library level...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... I think that in theory we should be allowed to use the deterministic signatures, but from what I noticed Nostr signatures are usually of the non-deterministic kind. Do you think it should be otherwise? I guess the library can do both, if needed, but isn't the non-deterministic version better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the non-deterministic version better?
i'm not aware of any argument why it would be.
in our particular case, there's another issue on top: if the nonce is not deterministic, attacks like Dark Skippy can use the signatures to smuggle out secret bits without the user being able to detect it. (with rfc6979 at least you can verify the signature if you also have the seed on the host side)
so please change to deterministic signatures, unless you have a spec explicitly saying you shouldn't do that. (and if so let's see it and talk to R&D about this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think RFC 6979 would work actually because we need Schnorr signatures. So I used BIP-340 to sign and I got deterministic signatures now. 45d4ee0
|
||
|
||
@expect(messages.NostrPubkey) | ||
def get_pubkey( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function should return just bytes
of the public key.
perhaps it's a good idea to rebase on #4490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't think it should be bytes
but rather the hex-encoded bytes, which is human readable and the (most) standard way to encode Nostr keys. Another encoding commonly used is NPUB - and I should probably add an option to return that encoding if desired, but at the most fundamental level it is the hex encoding, which is what all code dealing with Nostr is using. bytes
seems strange, because it looks bad, can't even be copy-pasted, and is of no use...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes
seems strange, because it looks bad, can't even be copy-pasted, and is of no use...
I mean you're deep inside a library, nobody is supposed to be copy-pasting the return value of this.
If there is a canonical representation of the pubkey as readable string, the NostrPubkey
message should send that from Trezor. If there isn't (you apparently have two choices, npub / hex), and you're returning raw bytes, the library function should return raw bytes.
Consumers of trezorlib.nostr.get_pubkey
are expected to do their own human readable conversion if they need to; specifically, if encoding to npub is simple, feel free to implement that in cli/nostr.py
and add an option to the user-exposed function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fwiw if you return bytes from a click-exposed function, there's some sort of conversion machinery that will automagically print it as hex for the user)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is a canonical representation of the pubkey as readable string, the
NostrPubkey
message should send that from Trezor
hex is the standard way to represent keys in code interacting with Nostr events. npub is a more "human friendly" standard, but it always has to be converted back to hex after being received from user input. So I think hex is the most appropriate standard to use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, if that's the case, please modify the Trezor side to return the hex string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It's all str
now: a95dc04 !
[no changelog]
[no changelog]
[no changelog]
python/src/trezorlib/cli/nostr.py
Outdated
# If not, see <https://www.gnu.org/licenses/lgpl-3.0.html>. | ||
|
||
import json | ||
from typing import TYPE_CHECKING, Dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead:
from __future__ import annotations
# ...
import typing as t
# ...
if t.TYPE_CHECKING:
# ...
# ...
def foo() -> dict[str, str]:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python/src/trezorlib/nostr.py
Outdated
).pubkey.hex() | ||
|
||
|
||
@expect(messages.NostrEventSignature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per #4490 please drop the @expect
and replace with client.call(..., expect=NostrEventSignature)
dtto for get_pubkey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_sign_event(client, pubkey_hex): | ||
response = nostr.sign_event( | ||
client, | ||
messages.NostrSignEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to move this construction to top level directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR adds Nostr support to the Trezor firmware,
trezorlib
andtrezorctl
.Firmware
The Trezor firmware accepts the
NostrGetPubkey
andNostrSignEvent
messages, as defined in common/protob/messages-nostr.proto.To avoid adding complexity to the firmware, it does not parse the event JSON. The event needs to be parsed beforehand and its individual fields passed via the protobuf message. The return value contains the pubkey, event ID and signature, which need to be added back to the JSON by the caller before forwarding the event to relays.
UI
A simple UI exists to confirm the event being signed on your Trezor's screen. It will be improved later, but for now it just shows all the fields of the event being signed:
Trezor Safe 5
Main screen shows the event kind and the content:
From the menu you can view the
created_at
and the tags:Trezor Safe 3
Trezor Model T
trezorctl
trezorctl
accepts thenostr get-pubkey
andnostr sign-event
commands which correspond to the protobuf messages described above.Note: by passing
--account
(which defaults to0
- the "first" account) totrezorctl nostr get-pubkey
ortrezorctl nostr sign-event
you can generate a virtually infinite amount of Nostr keypairs from your Trezor - all derived from your seed phrase using them/44'/1237'/<account>'/0/0
derivation path, as defined by NIP-06.get-pubkey
Example:
ibz@localhost:~/dev/trezor-firmware$ trezorctl nostr get-pubkey
Output:
pubkey: 2dc0dd32b499f7bd428156334c3431893c18e6016cf1bf00187d76513368dba7
sign-event
trezorctl nostr sign-event
will parse the event JSON, send theNostrSignEvent
to the Trezor device and re-assemble the event JSON with theid
,pubkey
andsig
obtained from the Trezor.Example:
ibz@localhost:~/dev/trezor-firmware$ trezorctl nostr sign-event "{\"kind\": 1, \"created_at\": 1733917737, \"tags\": [[\"e\", \"5c83da77af1dec6d7289834998ad7aafbd9e2191396d75ec3cc27f5a77226f36\", \"wss://nostr.example.com\"], [\"p\", \"f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca\"], [\"a\", \"30023:f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca:abcd\", \"wss://nostr.example.com\"], [\"alt\", \"reply\"]], \"content\": \"Hello world\"}"
Output:
signed_event: {"kind": 1, "created_at": 1733917737, "tags": [["e", "5c83da77af1dec6d7289834998ad7aafbd9e2191396d75ec3cc27f5a77226f36", "wss://nostr.example.com"], ["p", "f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca"], ["a", "30023:f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca:abcd", "wss://nostr.example.com"], ["alt", "reply"]], "content": "Hello world", "id": "c2ea8f08093caebf6d73bfe4edbb7e8825bac8eb4308ab634e23161db18b78f8", "pubkey": "2dc0dd32b499f7bd428156334c3431893c18e6016cf1bf00187d76513368dba7", "sig": "4176eaa45730a617f7393ba53df009278504911b3b30d35e125cb1c0f0d1e92102f468705eaed7d3ae5bb3bb4267d9411f0184a7ab6c451110553c2fca97b39c"}