Conversation
Pull Request Test Coverage Report for Build 23728712726Details
💛 - Coveralls |
Mshehu5
left a comment
Mshehu5
left a comment
There was a problem hiding this comment.
Thank you for taking this on!
Just took a quick look at the approach as this is still in draft will take better look later
Even though this is in draft just incase this is something you will keep working on please make sure to add commits messages as decribed in https://github.com/payjoin/rust-payjoin/blob/master/.github/CONTRIBUTING.md#commits
this will help reviewers and also incase when this gets merged to keep track of changes easier
| /// Override the generated receiver keypair with a specific keypair. | ||
| /// | ||
| /// Added for AS-aware relay selection: the receiver pubkey must | ||
| /// be known before the session is created to use as a deterministic seed | ||
| /// shared with the sender. | ||
| /// | ||
| /// This is the minimal change to unblock the PoC. Exposing keypair | ||
| /// on the public API may not be the right long-term design | ||
| pub fn with_receiver_keypair(self, keypair: HpkeKeyPair) -> Self { | ||
| Self(SessionContext { receiver_key: keypair, ..self.0 }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Though minimal I agree this is not the right long term design cause it might be a footgun by exposing/overiding the receiver key to downstream and also this may have privacy concerns.
I think thats why in the API, ReceiverBuilder (line 311) owns key generation internally by calling HpkeKeyPair::gen_keypair() (line 321)
Also this might cause issues for other protocol values that use it e.g:
proposal_mailbox_id , pj_uri , Incoming v2 payloads are decrypted with the receiver secret key,
Outgoing replies are encrypted with the same receiver keypair at create_post_request.
There was a problem hiding this comment.
hmm, why would the receiver key leak in this case? i do agree that the "make invalid state unrepresentable" maxim applies in this case, so i agree with the feedback but i could not follow the rationale
| async fn unwrap_relay_or_else_fetch( | ||
| &self, | ||
| directory: Option<impl payjoin::IntoUrl>, | ||
| receiver_pubkey: Option<&[u8; 33]>, | ||
| role: RelayRole, |
There was a problem hiding this comment.
one thing to note is relay choice is still app global in this PR so the new per-session AS aware selection can be overridden by other sessions because unwrap_relay_or_else_fetch in payjoin-cli keeps one shared relaymanager for all sessions. Once one session sets selected_relay later sessions will probably skip the receiver_pubkey/role-based ordering this may defeat the purpose of this PR
There was a problem hiding this comment.
the selection logic implemented here should work for per session relay selection as well, and part of the original idea was for the receiver and sender to use the same shared secret to derive opposing orders so the role would still be important, so while the interface might change, the purpose of this PR would not be obviated, it implements stronger protection against a passive adversary than just random selection that still makes sense even if only for retrying logic.
if two relays from the same compromised AS are chosen by both sender and receiver then their network metadata will be linkable, and their combined activity will be temporally linkable to potential payjoin transactions. if the network metadata is linkable to PII that is in turn related to clustering information, this could cause a complete breakdown of the payjoin privacy model.
There was a problem hiding this comment.
Sorry I may not have been clear in my earlier comment. I am not saying the AS-aware role ordering is unnecessary. I agree that still makes sense as a privacy improvement.
My concern is narrower and specific to the current payjoin-cli wiring. The chosen relay is kept in RelayManager.selected_relay and App holds a single shared RelayManager for the whole process. In unwrap_relay_or_else_fetch() we first check get_selected_relay() and return it immediately if it is already set. So once one session sets selected_relay later sessions may just reuse that relay and never re-run the per-session AS-aware role-based ordering. The issue I meant to point out is the scope of that cached relay state in this PR’s current implementation which I think would need to be addressed rather than the value of the selection logic itself.
|
thanks for the review @Mshehu5
imo, the test analysis still valid, because this would only manifests after the first relay is selected. since I worked on this PoC just to follow my curiosity, its not my actual focus and I didn't know it was in your scope, if needed you can feel totally free to take it on and keep going, ok? |
4 participants
this is a proof of concept based on #919
first of all, I want to congrats @nothingmuch , this is a very smart algorithm! its really interesting
the proposal of this PoC is to unblock the discussion, I think its a great and important privacy path
Bitcoin core uses a pre-compiled binary trie (.dat, bitcoin-core/asmap-data), but to simplify I used Kartograf text file as the Asmap source (https://github.com/asmap/kartograf)
I made a manual ASN field that user can write to filter directories and relays by AS
the code enables multiple directories
left some notes around it
running:
test analysis:
I used claude and big pickle to research and code assistance
Logs