nostr-java icon indicating copy to clipboard operation
nostr-java copied to clipboard

The client to validate the message before sending it to the relay

Open tcheeric opened this issue 1 year ago • 12 comments

See spec: https://github.com/nostr-protocol/nips/blob/master/01.md#from-client-to-relay-sending-events-and-creating-subscriptions

tcheeric avatar Dec 17 '24 23:12 tcheeric

hi, eric. perhaps a good software engineering consideration re: above-

if/when you've got a nostr-java message validator in place, seems sensible for SC to use that same mechanism instead of coding up a separate one... and if you haven't yet started an implementation and/or have other/urgent/etc TODO's, feel free to assign this task to me.

avlo avatar Dec 21 '24 23:12 avlo

Ok, noted. I am assigning to you, as I have not touched yet. Thanks.

tcheeric avatar Jan 02 '25 22:01 tcheeric

starting this item now & btw- do you have a label/equivalent to indicate an item/issue/etc is "in progress"? i don't see one/similar in the current set:

labels-03

if not, i think worthwhile & informative to add one.

avlo avatar Jan 03 '25 17:01 avlo

Good idea. I thought I did, but I will create the necessary labels, if missing

Secured with Tuta Mail: https://tuta.com/free-email

3 Jan 2025, 17:58 by @.***:

starting this item now & btw- do you have a > label https://github.com/tcheeric/nostr-java/issues/labels> /equivalent to indicate an item/issue/etc is "in progress"? i don't see one/similar in the current set:

labels-03.png (view on web) https://github.com/user-attachments/assets/10b0f41b-f44c-41df-ab69-7dc53bcf0186

if not, i think worthwhile & informative to add one.

— Reply to this email directly, > view it on GitHub https://github.com/tcheeric/nostr-java/issues/187#issuecomment-2569612106> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/ABQMG7FL2OMS4WVGT75XCAT2I3FVTAVCNFSM6AAAAABTZPISDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNRZGYYTEMJQGY> . You are receiving this because you were assigned.> Message ID: > <tcheeric/nostr-java/issues/187/2569612106> @> github> .> com>

tcheeric avatar Jan 03 '25 18:01 tcheeric

PR #195 submitted

avlo avatar Jan 08 '25 02:01 avlo

This looks good, thanks!

Re. the test failures.

[ERROR] Errors: [ERROR] ApiEventTest.testNIP04EncryptDecrypt:144 » IllegalArgument Invalid hex string: [0256adf01ca1aa9d6f1c35953833bbe6d99a0c85b73af222e6bd305b51f2749f6f], length: [66], target length: [64] [ERROR] ApiEventTest.testNIP04SendDirectMessage:107 » IllegalArgument Invalid hex string: [0256adf01ca1aa9d6f1c35953833bbe6d99a0c85b73af222e6bd305b51f2749f6f], length: [66], target length: [64]

I think the validation is not appropriate or incorrect in this specific scenario. If you look at the code sample in the spec: https://github.com/nostr-protocol/nips/blob/master/04.md#encrypted-direct-message

let sharedPoint = secp.getSharedSecret(ourPrivateKey, '02' + theirPublicKey)

It expects that you add the '02' prefix to the public key when calculating the sharedpoint, which is why the length of 66 (and not 64).

My implementation is in nostr.crypto.nip04.EncryptedDirectMessage

    private static byte[] getSharedSecret(String privateKeyHex, String publicKeyHex) {

        SecP256K1Curve curve = new SecP256K1Curve();
        ECPoint pubKeyPt = curve.decodePoint(NostrUtil.hexToBytes("02" + publicKeyHex));
        BigInteger tweakVal = new BigInteger(1, NostrUtil.hexToBytes(privateKeyHex));
        return pubKeyPt.multiply(tweakVal).getEncoded(true);
    }

In general, if my understanding is correct, private and public key hex representations have different lengths: Private keys: 64 Public keys: 66

tcheeric avatar Jan 12 '25 17:01 tcheeric

gotcha & thx for pointing out that specification variant. to accommodate it, i've added a NostrUtil NIP04-specific (66-character, under the hood) method:

public static byte[] nip04PubKeyHexToBytes(String s)

and updated the PR to reflect its addition.

note: if testing message-validation related changes using SC, you'll need the most recent version of req_message-filters_validation branch


per your last paragraph, quick disambiguation on my part re: "in general":

In general, if my understanding is correct, private and public key hex representations have different lengths: Private keys: 64 Public keys: 66

the "in general" context you're referring to is constrained to NIP-04, correct?

if i'm mistaken, pls advise as "in general (as per NIP-01)", public key lengths are spec'd as:

"pubkey": <32-bytes lowercase hex-encoded public key of the event creator>,

where each hex-encoded byte is composed of 2 hex digits (00, 01, ..., FE, FF) resulting in total string length of 32 * 2 == 64 characters as illustrated throughout the documentation:

["p", "f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca"]

"pubkey": "b0635d6a9851d3aed0cd6c495b282167acf761729078d975fc341b22650b07b9"

"pubkey":"00000000827ffaa94bfea288c3dfce4422c794fbb96625b6b31e9049f729d700"

"pubkey":"97c70a44366a6535c145b333f973ea86dfdc2d7a99da618c40c64705ad98e322"

pubkey: 8e0d3d3eb2881ec137a11debe736a9086715a8c8beeeda615780064d68bc25dd

"pubkey": "477318cfb5427b9cfc66a9fa376150c1ddbc62115ae27cef72417eb959691396"

avlo avatar Jan 13 '25 00:01 avlo

perhaps worth noting, an obvious alternative to:

public static byte[] nip04PubKeyHexToBytes(String s)

could be a more generalized method requriing hex-string length as 2nd parameter:

public static byte[] hexToBytes(String s, int length)

which, although more flexible, i'd decided against since its potential misuse yields downstream repercussions that are safeguarded against by the submitted implementation

avlo avatar Jan 13 '25 01:01 avlo

per your last paragraph, quick disambiguation on my part re: "in general":

In general, if my understanding is correct, private and public key hex representations have different lengths: Private keys: 64 Public keys: 66

the "in general" context you're referring to is constrained to NIP-04, correct?

if i'm mistaken, pls advise as "in general (as per NIP-01)", public key lengths are spec'd as:

"pubkey": <32-bytes lowercase hex-encoded public key of the event creator>,

where each hex-encoded byte is composed of 2 hex digits (00, 01, ..., FE, FF) resulting in total string length of 32 * 2 == 64 characters as illustrated throughout the documentation:

["p", "f7234bd4c1394dda46d09f35bd384dd30cc552ad5541990f98844fb06676e9ca"]

"pubkey": "b0635d6a9851d3aed0cd6c495b282167acf761729078d975fc341b22650b07b9"

"pubkey":"00000000827ffaa94bfea288c3dfce4422c794fbb96625b6b31e9049f729d700"

"pubkey":"97c70a44366a6535c145b333f973ea86dfdc2d7a99da618c40c64705ad98e322"

pubkey: 8e0d3d3eb2881ec137a11debe736a9086715a8c8beeeda615780064d68bc25dd

"pubkey": "477318cfb5427b9cfc66a9fa376150c1ddbc62115ae27cef72417eb959691396"

You are definitely correct regarding key lengths. I had based my comment on cashu's implementation, and it seems there is a slight difference there... Thanks.

tcheeric avatar Jan 13 '25 07:01 tcheeric

just noticed this NIP-04 deprecation notification, if of any relevance to you:

Screenshot_20250113_053143_GitHub~01.png

wherein NIP-17 event spec appears to use 64-character pubkey hex strings (non-conclusive, brief examination) among additional encryption/security options & their related considerations.

avlo avatar Jan 13 '25 10:01 avlo

Good to know, thanks. Nip 04, until recently, was still used in nip 46 to encrypt contents, but I've just checked and noticed 46 has now been updated to use 44 encryption instead.  We will probably have to completely remove 04 from the codebase...

Secured with Tuta Mail: https://tuta.com/free-email

13 Jan 2025, 10:42 by @.***:

just noticed this NIP-04 deprecation notification, if of any relevance to you:

Screenshot_20250113_053143_GitHub.01.png (view on web) https://github.com/user-attachments/assets/f05d6084-4ce7-438a-a71e-051d19af4373

wherein NIP-17 uses 64-character length pubkey hex-string lengths.

— Reply to this email directly, > view it on GitHub https://github.com/tcheeric/nostr-java/issues/187#issuecomment-2586756112> , or > unsubscribe https://github.com/notifications/unsubscribe-auth/ABQMG7FHONUCYSU2UYHRHKL2KOKA3AVCNFSM6AAAAABTZPISDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBWG42TMMJRGI> . You are receiving this because you were assigned.> Message ID: > <tcheeric/nostr-java/issues/187/2586756112> @> github> .> com>

tcheeric avatar Jan 13 '25 11:01 tcheeric

You are definitely correct regarding key lengths. I had based my comment on cashu's implementation, and it seems there is a slight difference there... Thanks.

i wonder if implementation/spec-enforcement shortcomings will have downstream data-validity/integrity consequences for cashu/users... with further concern correlated relay implementations must therefore necessarily also exist with incorrect/absent spec enforcement.

props to you pre-empting that defect early for us w/ validations. i'll do the same moving forward.

avlo avatar Jan 13 '25 12:01 avlo

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

github-actions[bot] avatar Aug 13 '25 00:08 github-actions[bot]

Closing this issue due to prolonged inactivity.

github-actions[bot] avatar Aug 21 '25 00:08 github-actions[bot]