Make Session Initialization Implicit #364

DanGould · 2024-10-11T16:17:03Z

Close #363

A session is now initialized by generating keys and sharing them out
of band.

A POST request pushes data to a subdirectory, by convention defined as the receiver's public key. GET requests attempt to fetch data. For the sender, this means they poll GET requests instead of POST requests so bandwidth use is reduced.

Payjoin v1 backwards compatibility is supported by adding v1 aware handlers, one for POST requests directly at the relay, since v1 is unaware of OHTTP, and a PUT handler on the subdirectory's OHTTP endpoint that saves the updated Payjoin PSBT to another Redis column. The v1 POST handler is waiting on an update to that column to repond to the v1 sender before the request times out.

Lastly, Message A is now encrypted with an ephemeral "encapsulation key" so that the sender's "reply key," used to identify the sender subdirectory where Message B is stored is encrypted inside Message A's ciphertext in order to prevent the Payjoin Directory from finding it. This prevents the Payjoin Directory from being able to relate the sender and receiver subdirectories by searching for the subdirectory identifying "reply key" in Message A's associated data. All they'll find is the ephemeral key that has no relation to the subdirectory id.

nothingmuch

just some nits. still familiarizing myself with the code, so not yet comfortable expressing stronger opinions

payjoin/src/receive/v2/mod.rs

payjoin/src/send/mod.rs

nothingmuch · 2024-10-15T16:12:53Z

oh, one more thing: maybe consider renaming ActiveSession to Session because the distinction is now meaningless

DanGould · 2024-10-15T16:29:33Z

oh, one more thing: maybe consider renaming ActiveSession to Session because the distinction is now meaningless

Perhaps even just Receiver to map against the Sender

DanGould · 2024-10-15T17:45:11Z

The tests appear to be flakier than they were before, getting "Ohttp relay is long running" from the ohttp_relay shut down before the test completes on occasion 🤔

0xBEEFCAF3

This is a rather large PR. Probably going to review in chunks. ApproachACK. Leaving Nits in a few places. Please feel free to ignore or tell me why i'm wrong about certain comments. This is the first time im looking at a PR for this repo. Thanks

payjoin-cli/src/app/v2.rs

0xBEEFCAF3 · 2024-10-15T18:36:57Z

payjoin-directory/src/db.rs

@@ -4,8 +4,8 @@ use futures::StreamExt;
 use redis::{AsyncCommands, Client, ErrorKind, RedisError, RedisResult};
 use tracing::debug;

-const RES_COLUMN: &str = "res";
-const REQ_COLUMN: &str = "req";
+const DEFAULT_COLUMN: &str = "";


Something descriptive maybe useful here when debugging in prod. even "default-col"

payjoin/src/hpke.rs

spacebear21

Loving this protocol upgrade.

I'm confused by the sender polling in long_poll_post - it looks like they're polling POST requests but based on my understanding of the BIP77 updates and PR description shouldn't they be polling GETs?

payjoin-directory/src/lib.rs

payjoin/tests/integration.rs

spacebear21 · 2024-10-17T20:55:47Z

payjoin-cli/src/app/v2.rs

+                loop {
+                    let (req, ohttp_ctx) = v2_ctx.extract_req(self.config.ohttp_relay.clone())?;
+                    let response = http
+                        .post(req.url)


Shouldn't the sender be polling GET requests here?

ohttp requests are always POSTs so that they're indistinguishable. Only when the OHTTP encapsulation is removed does the target server see the inner GET request.

see https://github.com/payjoin/rust-payjoin/pull/364/files#diff-e1a2a0997d5241c211b358d2ea2be04fc1a4aa680ac543480188d7b4ab63ff72R448

I wonder if there's a better way to represent this in the state machine than POST/GET to avoid this confusion

I see now the GET request is constructed in extract_req, makes sense. It might help if the OHTTP post was extracted into a make_ohttp_request(req) function or something along those lines.

Good idea I plan to write for both v2.rs and integration.rs in a follow up

nothingmuch

messages a and b are distinguishable due to the use of Auth mode in message a

payjoin/src/hpke.rs

DanGould · 2024-10-21T01:20:09Z

After 9f5a170 this should close #371 in implementation. We'll have to add the details to BIP77 but I think assuming this passes review it should be ready to merge.

A session is now initialized by generating keys and sharing them out of band. The semantics of the protocol are otherwise unchanged. Because sessions are implicit the typestate is now called `Receiver`.

DanGould · 2024-10-21T17:41:46Z

rebased

Encrypting Message A with an ephemeral "encapsulation key" allows the sender "reply key" corresponding to its subdirectory to be hidden from the directory.

Since the encapsulation keypair was ephemeral and not known to the receiver, but used in the Auth pattern it was included as authenticated associated data in the payload. This means that encrypt_message_a and encrypt_message_b had distinguishable bit patterns, the former starting with two uncompressed curve points (one for the DHKEM and one for this auth key), whereas the latter only had one (the DHKEM point). Since the sender's first message establishes a reply key, that key could be used in a second Auth HPKE setup after the Base setup, in order to prove that the sender can decrypt the receiver's reply. However, incentives are for the sender to provide a valid point, and the reply key is included in AEAD ciphertext, so this would add complexity without meaningful improving security or incentive compatibility.

The constants PADDED_PLAINTEXT_{A,B}_LENGTH now represent the maximum payload size for the input, whereas before the message A constant included the reply key size. This makes the PayloadTooLarge error represent a maximum and actual size that correspond to the inputs to encrypt_message_a and encrypt_message_b.

spacebear21

ACK 96c0eb6. I haven't looked very closely at the hpke changes.

nothingmuch

utACK 96c0eb6

I coudln't review everything in detail since I'm still familiarizing myself with the codebase, and as such I can't really call my fiddling "tested". However the parts I was able to scrutinize I'm comfortable with.

Perhaps consider adding a randomized test for the encrypted payloads' bit uniformity:

generate 8 each of messages a and b, and 16 each of only a, and only b in separate runs
for each of the 256 pairwise combinations, ensure their lengths are equal, XOR the two messages together, and OR this into an accumulator that starts as all 0x00s
if at the end any of the bytes of this accumulator are still 0x00, fail the test

Such a test will generate a false negative with negligible probability if all encrypted messages share an identical byte at a given position by chance. It should fail deterministically before the ellswift changes due to the first byte being 0x01 in all messages in all runs (a only, b only, a+b). It should fail deterministically for a only messages because of the second 0x01 byte at position 65 (the previously aad'd reply key).

@nothingmuch

close #371 This randomized test will generate a false negative with negligible probabilityif all encrypted messages share an identical byte at a given position by chance. It should fail deterministically if any bit position has a fixed value. re #364 (review) from @nothingmuch I did check that this test would indeed fail before the ellswift changes by cherry-picking the test on 9c4880c

DanGould force-pushed the implicit-sessions branch 2 times, most recently from 8f5aca5 to a6c2a8a Compare October 14, 2024 21:48

DanGould requested a review from spacebear21 October 14, 2024 21:49

DanGould force-pushed the implicit-sessions branch 2 times, most recently from b307fa2 to f050e77 Compare October 15, 2024 15:23

nothingmuch reviewed Oct 15, 2024

View reviewed changes

payjoin/src/receive/v2/mod.rs Outdated Show resolved Hide resolved

payjoin/src/send/mod.rs Outdated Show resolved Hide resolved

payjoin/src/send/mod.rs Outdated Show resolved Hide resolved

DanGould force-pushed the implicit-sessions branch 3 times, most recently from 60e41fb to c2eafff Compare October 15, 2024 17:38

DanGould changed the title ~~WIP: Make session initialization implicit~~ Make Session Initialization Implicit Oct 15, 2024

DanGould requested a review from nothingmuch October 15, 2024 17:51

DanGould marked this pull request as ready for review October 15, 2024 17:51

DanGould force-pushed the implicit-sessions branch from 927d742 to 88d5da7 Compare October 15, 2024 18:06

0xBEEFCAF3 reviewed Oct 15, 2024

View reviewed changes

nothingmuch mentioned this pull request Oct 16, 2024

Change the Directory Semantics to Support Multiparty Payjoin #365

Open

spacebear21 reviewed Oct 17, 2024

View reviewed changes

nothingmuch suggested changes Oct 17, 2024

View reviewed changes

payjoin/src/hpke.rs Outdated Show resolved Hide resolved

This was referenced Oct 18, 2024

Consider Making pjv2 payloads and IDs Uniform so they Might be Indistinguishable from Future Protocols' #371

Closed

make payloads uniform by removing sender auth key DanGould/rust-payjoin#6

Merged

DanGould force-pushed the implicit-sessions branch 7 times, most recently from ed39bac to 3756c47 Compare October 21, 2024 01:16

DanGould requested review from spacebear21 and nothingmuch October 21, 2024 01:20

DanGould added 2 commits October 21, 2024 13:34

Make session initialization implicit

8b80fcf

A session is now initialized by generating keys and sharing them out of band. The semantics of the protocol are otherwise unchanged. Because sessions are implicit the typestate is now called `Receiver`.

Request with implicit initialization pattern

4e7cd28

DanGould force-pushed the implicit-sessions branch from 3756c47 to 5d83737 Compare October 21, 2024 17:41

DanGould and others added 10 commits October 21, 2024 13:41

Separate hpke and ohttp v2 modules

86e3781

Encrypt Message A with ephemeral key like Noise IK

84bec83

Encrypting Message A with an ephemeral "encapsulation key" allows the sender "reply key" corresponding to its subdirectory to be hidden from the directory.

Remove obsolete Sender keypair

9b2635a

Seek in hpke en/decryption instead of magic numbers

affb88b

Redact hpke secret key from Debug

38d8777

Define INFO_x msg as constant size array ref

9c4880c

Ellswift Encode public keys in hpke wire protocol

69c7800

Adjust send documentation to use Sender

96c0eb6

spacebear21 approved these changes Oct 22, 2024

View reviewed changes

nothingmuch approved these changes Oct 22, 2024

View reviewed changes

DanGould merged commit 5f4fe76 into payjoin:master Oct 22, 2024
4 checks passed

DanGould mentioned this pull request Oct 22, 2024

Test that the encrypted payloads are uniform. #374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Session Initialization Implicit #364

Make Session Initialization Implicit #364

DanGould commented Oct 11, 2024 •

edited

Loading

nothingmuch left a comment

nothingmuch commented Oct 15, 2024

DanGould commented Oct 15, 2024

DanGould commented Oct 15, 2024

0xBEEFCAF3 left a comment

0xBEEFCAF3 Oct 15, 2024

spacebear21 left a comment

spacebear21 Oct 17, 2024

DanGould Oct 17, 2024 •

edited

Loading

spacebear21 Oct 21, 2024

DanGould Oct 22, 2024

nothingmuch left a comment

DanGould commented Oct 21, 2024

DanGould commented Oct 21, 2024

spacebear21 left a comment

nothingmuch left a comment •

edited

Loading

Make Session Initialization Implicit #364

Make Session Initialization Implicit #364

Conversation

DanGould commented Oct 11, 2024 • edited Loading

nothingmuch left a comment

Choose a reason for hiding this comment

nothingmuch commented Oct 15, 2024

DanGould commented Oct 15, 2024

DanGould commented Oct 15, 2024

0xBEEFCAF3 left a comment

Choose a reason for hiding this comment

0xBEEFCAF3 Oct 15, 2024

Choose a reason for hiding this comment

spacebear21 left a comment

Choose a reason for hiding this comment

spacebear21 Oct 17, 2024

Choose a reason for hiding this comment

DanGould Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

spacebear21 Oct 21, 2024

Choose a reason for hiding this comment

DanGould Oct 22, 2024

Choose a reason for hiding this comment

nothingmuch left a comment

Choose a reason for hiding this comment

DanGould commented Oct 21, 2024

DanGould commented Oct 21, 2024

spacebear21 left a comment

Choose a reason for hiding this comment

nothingmuch left a comment • edited Loading

Choose a reason for hiding this comment

DanGould commented Oct 11, 2024 •

edited

Loading

DanGould Oct 17, 2024 •

edited

Loading

nothingmuch left a comment •

edited

Loading