Skip to content
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

Medium groups with sender keys (essentials) #1117

Merged
merged 5 commits into from May 14, 2020

Conversation

msgmaxim
Copy link

@msgmaxim msgmaxim commented May 4, 2020

This is not a complete implementation, but most of it is done and this PR is getting big.

Implemented:

  • Polling for more than one id (group ids in addition to our identity)
  • lastHash is now conversationId-aware
  • Group creation and sender key management
  • Messages with sender keys are correctly associated with the right conversation in GUI
  • A brand new ratchet implementation for use with sender keys (different from Signal's in that it is missing the DH component impractical for groups)

For testing:

async function createMediumSizeGroup(groupName, members) { ... }

Example:

window.createMediumSizeGroup("GroupName", ["0531715c5f8476108b2481c9f42d62943417d795da96f0a7346644401294dba319", "05aaf884bef56a7d0dc044e2c8a330c4d0e2cb3e2cb785b6c169bc9c0beb0fcf21"])

Still missing:

  • Multidevice support
  • More robust sender key exchange (better interplay with session-request)
  • Better error handling

config/swarm-testing.json Outdated Show resolved Hide resolved
Comment on lines +211 to +214
if (senderIdentity === ourNumber) {
// Ignoring our own message
return request.respond(200, 'OK');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

for some server calls, we reply only when the message is processed really. I think it's a timing security stuff. Shouldn't we do the same here?

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't actually do anything at the moment:

this.respond = (status, message) => {
  // Mock websocket response
  window.log.info(status, message);
};


return plaintext;
})
.then(plaintext => this.postDecrypt(envelope, plaintext))
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the envelope (first arg) set when calling postDecrypt after the case textsecure.protobuf.Envelope.Type.PREKEY_BUNDLE or textsecure.protobuf.Envelope.Type.CIPHERTEXT

Copy link
Author

Choose a reason for hiding this comment

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

envelope is already set at the top of async decrypt(envelope, ciphertext) right?


proto.mediumGroupUpdate = update;

// TODO: send to our linked devices too?
Copy link
Collaborator

Choose a reason for hiding this comment

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

a linked device should receive this message too normally

Copy link
Author

Choose a reason for hiding this comment

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

Correct. I removed the comment already in the upcoming PR.

};

if (numbers.length === 0) {
return Promise.resolve({
return {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return of an object here and a promise just below
return this.sendMessage(attrs, options);
it's not causing any issue?

Copy link
Author

Choose a reason for hiding this comment

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

The method is async so every return is implicitly wrapped in a promise.

Comment on lines +1197 to +1204
const attachment = await this.makeAttachmentPointer(avatar);

proto.group.avatar = attachment;
// TODO: re-enable this once we have attachments
proto.group.avatar = null;
await this.sendGroupProto(recipients, proto, Date.now(), options);

return proto.group.id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you just don't like the .then syntax?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. For one, it makes it hard to see what is being returned.

@@ -12,6 +12,7 @@ message Envelope {
PREKEY_BUNDLE = 3; //Used By Signal. DO NOT TOUCH! we don't use this at all.
RECEIPT = 5;
UNIDENTIFIED_SENDER = 6;
MEDIUM_GROUP_CIPHERTEXT = 7;
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding was that we added stuff of our own after 100 in the case we have to pull some stuff from signal. MEDIUM_GROUP_CIPHERTEXT is considered a signal stuff?

Copy link
Author

Choose a reason for hiding this comment

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

I deliberately chose not to use 100+ values, because I don't think we should compromise our codebase because of Signal. If Signal ever adds a new entry for 7 (and we ever pull changes), we'll just give it the next available value.

Copy link

@vincentbavitz vincentbavitz left a comment

Choose a reason for hiding this comment

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

Looks good

@msgmaxim msgmaxim merged commit f3a8f43 into oxen-io:clearnet May 14, 2020
@msgmaxim msgmaxim deleted the sender-keys branch July 7, 2020 06:25
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.

None yet

5 participants