Skip to content

Conversation

@kaichaosun
Copy link
Contributor

@kaichaosun kaichaosun commented Nov 21, 2025

What needs to be done,

  • fix chainKeySend in DH ratchet if remote key is different
  • make the invitee who accepts the invite to send out the first message (before send, the ratchet state is initialized with new DH key). Otherwise dh ratchet is not triggered with identity key as DH key.
  • Do not process outgoing messages with convo.doubleratchet.dhSelf.public == header.dhPublic.
  • clear logs
  • fix the error (decryption failed context: [code = errMessageAuthentication) in ping pong demo

@kaichaosun kaichaosun requested a review from jazzz November 21, 2025 04:24
Copy link
Collaborator

@jazzz jazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall great fixes.


proc initPrivateV1*(owner: Identity, participant: PublicKey, seedKey: array[32, byte],
discriminator: string = "default", isSender: bool, deliveryAckCb: proc(
discriminator: string = "default", inviter: bool, deliveryAckCb: proc(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving away from the "isSender" convention makes sense here.

[Sand] Perhaps "Initiator" would be a better fit as it keeps the language consistent.

Copy link
Contributor Author

@kaichaosun kaichaosun Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When reading the proto definition, I somehow need to think twice to reason the term participant (initiator seems fine though). The term inviter/invitee actually looks a bit clear and straightforward to me.

message InvitePrivateV1 {
    bytes initiator = 1;
    bytes initiator_ephemeral = 2;
    bytes participant = 3;
    int32 participant_ephemeral_id= 4;
    string discriminator = 5;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear unambiguous terms are important. Feel free to suggest new ones here: logos-messaging/specs#80

topic = convo.getConvoId()
client.addConversation(convo)

# TODO send a control frame instead
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why any frame needs to be sent here.

A Recipient ought to be able to initialize an inbound session and receive messages without revealing activity to the sender

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I miss something, here is my thought:

After both run initDoubleratchet
initiator(Alice), dh is id key, dh remote is bob's id pub key, send key is empty
recipient (Bob), dh is a new generated key,, dh remote is alice's id pub key, send key and new root key derived from DH.

Without sending a control message, if alice sends message to bob, alice will use message key derived from empty send key. This seems not what we want.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without sending a control message, if alice sends message to bob, alice will use message key derived from empty send key

You are correct in that the DH step requires a key from the remote party before it can occur.

Alice can use bobs static installation key as the initial DHr. This should ultimately be replaced with a signed pre-key equivalent once the work on identity is completed

Requiring a response from Bob to complete initialization, would remove the non-interactive protocol property

client.addConversation(convo)

# TODO send a control frame instead
discard convo.sendMessage(client.ds, initTextFrame("Hello").toContentFrame())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a tracking ticket to make sendMessage content Agnostic.
#21

Comment on lines 59 to 60


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a Linter that works well for nim? I've been looking for one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nph (https://github.com/arnetheduck/nph) used in nwaku, seems fine. Let's try it.


let dhOutputPost = self.dhSelf.dhExchange(self.dhRemote).get()
(self.rootKey, self.chainKeyRecv) = kdfRoot(self, self.rootKey, dhOutputPost)
(self.rootKey, self.chainKeySend) = kdfRoot(self, self.rootKey, dhOutputPost)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

I'd suggest getting rid of the direct kdf call all togther and use dhRatchetSend which also initializes msgCountSend

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dhRatchetRecv calls dhRatchetSend is pretty confusing, we may need to rename both terms and see how to reuse some code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a correct implementation, the sendingChainKey is not generated until it is needed to minimize key exposure.

dhRatchetRecv: Performs the first part of the ratchet step when receiving DHr
dhRatchetSend: Performs the second part once the client wants to send a message.

One path to clean this up, is to remove keyGeneration and subsequent KeyExchange from this function all together.

@kaichaosun
Copy link
Contributor Author

Closed since most of the fix is already merged in other PRs.

@kaichaosun kaichaosun closed this Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants