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

Use generic encrypted payloads #108

Merged
merged 8 commits into from
Nov 22, 2021
Merged

Use generic encrypted payloads #108

merged 8 commits into from
Nov 22, 2021

Conversation

hackergrrl
Copy link
Member

Hey @cblgh, this is per our earlier discussion! Here is my new proposal for the structure of private messages & encrypted messages in general:

  1. Use a generic encryption format (no private/chat, just use text/chat so existing logic can be more easily reused).
  2. Continue using 'type: "encrypted" on ciphertext payloads. This means encrypted messages won't affect any existing views, since it's a type unknown to them. Views can opt-in to supporting encrypted messages as needed.
  3. I decided against a cipher_type key on the ciphertext message, since we didn't have a clear decision on the pros/cons of that. However, we can add later /wo breaking backwards compatibility.

1:1 private text message format

It's just the same as regular chat message, but encrypted, and the inner payload has a recipients key instead of channel key.

{
  type: 'chat/text',
  content: {
    text: 'hello',
    recipients: [keypair.publicKey.toString('hex')]
  }
}

Does this sound good to you? I kinda suspect that as you're implementing you'll discover any holes in this design. 😈

@hackergrrl hackergrrl requested a review from cblgh November 3, 2021 22:03
@hackergrrl hackergrrl marked this pull request as draft November 3, 2021 22:04
@cblgh
Copy link
Member

cblgh commented Nov 4, 2021

thanks for putting up this pr @hackergrrl!

started looking at this a bit late so feedback may not be of the utmost quality at this stage, some thoughts follow regardless:

  • if using chat/text might be nice to keep channel (and not use recipients) as-is for compat with existing logic for handling chat/text messages. at least in my current wip work on cabal-client i've got some logic to massage the previous private/text into the more generic format, and i think that is one of the parts that it massages (in addition to the type)
  • we can add later /wo breaking backwards compatibility

    • great realization!
  • will probably have more to say re: code once i rebase my existing work ontop of this!

some more thoughts re the private message usecase: i think it makes sense to start with only supporting 1-1 PMs atm; there's a lot more work that needs to happen for 1-many PMs to be deployed (in cli, in desktop, in cabal-client). additionally, i think there's a good case to make for the UX of 1-1 PMs being fairly well-understood by folx and users of chat apps in general, so starting there feels like a nice incremental step instead of a giant leap :3

1-many PMs with the secret box caveats and constraints (e.g. better make the max #recpts small as a tradeoff to make message overhead more reasonable) makes less sense, especially when thinking about how to communicate them to users of apps, than having a generic secret channel mechanism and using that for "group PMs"

a lot of words to say "let's keep channel: string and omit recipients: array 😊

@hackergrrl
Copy link
Member Author

hackergrrl commented Nov 7, 2021 via email

@cblgh
Copy link
Member

cblgh commented Nov 8, 2021

thanks for the patch @hackergrrl!

regarding your concern: yup! totally valid. in my current cabal-client code i've taken some precautions wrt validation. for instance making sure nobody can create/join a channel if it conforms to the pub key regex && if there's a user known with that id—it probably needs to be stricter. this validation also doesn't help if you are building ontop of cabal-core directly of course.

i was thinking in bed earlier re the problems of having exactly the same structure as regular messages, and thought that maybe we can add a smol bit of metadata to private messages like private: true. using that bit of information we can make doubly sure in public (non-private) channels that we only ever display messages that are not marked as private (and only display private messages in channels that are marked as private). curious to hear what you think abt that!

@hackergrrl
Copy link
Member Author

hackergrrl commented Nov 8, 2021 via email

@cblgh
Copy link
Member

cblgh commented Nov 10, 2021

@hackergrrl added the metadata (and am using it in my cabal-client code :)

also pushed the cabal-core fix i had locally for making sure the ready event fires when all indexes have been successfully loaded (and not only the sync ones)—hope that's not terribly out of scope, just felt a bit easier to do that since it was already in my working code

@hackergrrl
Copy link
Member Author

Great. A last nice thing would be to update the test/private.js tests to check for private: true, but looks good to me. 👍🏻

@cblgh
Copy link
Member

cblgh commented Nov 11, 2021

@hackergrrl thanks for the nudge! i'll get that done next week 😊

@cblgh
Copy link
Member

cblgh commented Nov 17, 2021

@hackergrrl alright, there we go!

@cblgh
Copy link
Member

cblgh commented Nov 18, 2021

@hackergrrl do you have any further comments? if not, i will go ahead and merge this + issue a new release (as i guess a new major? bc we changed the private messages api)

@hackergrrl
Copy link
Member Author

@cblgh No comments!

@cblgh cblgh marked this pull request as ready for review November 19, 2021 11:12
@cblgh cblgh merged commit 86b93bd into master Nov 22, 2021
@cblgh cblgh deleted the generic-private-payloads branch November 22, 2021 10:07
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.

2 participants