WIP: Hybrid Groups Initial Sketch #545

Draft
sarah wants to merge 1 commits from hybridgroups into master
Owner

Ok, this is part 1.

This implements the most basic version of hybrid groups where a group manager can proxy messages between peers with some very basic filtering.

What is done:

  • Fix a bug in Event Manager that was resulting in duplicate queues / events
  • Get/Set Conversation Number Vars (used a lot in hybrid groups, cuts down on code)
  • Initial Cut of functionality.Hybrid for both Clients and the Manager
  • New Cwtch level APIs for Internal Conversation Management (NewConversation, InternalInsertMessage and SignMessage - these are needed because Extensions now need to manage inserting messages on behalf of a conversation.
  • Hybrid Groups integration test that simple sets up a group and has alice post a message and check that 1) bob receives the message and 2) alice acks the message
  • Channel routing based on Overlay Number (see code comments)
  • Moving some Test Utils around to make writing integration tests cleaner. There will probably be more of these.

Things that are definitely missing:

  • Group Level Encryption / Key Management
  • ACL / Membership Checking
  • Offline Delivery
  • Queue for Offline Delivery (essential for above)
  • Group Metadata Management (add/edit/rotate key etc.)
  • Any checking that clients are following the protocol defined by the formal model

However, even without all these things, I think now is the right time to review this PR and get a feel for this code. All those other things are nuanced and will require detailed review and I don't want them lost while reviewing all the stuff in this PR - which is mostly a structural sketch.

Ok, this is part 1. This implements the most basic version of hybrid groups where a group manager can proxy messages between peers with some very basic filtering. What is done: - [X] Fix a bug in Event Manager that was resulting in duplicate queues / events - [X] Get/Set Conversation Number Vars (used a lot in hybrid groups, cuts down on code) - [X] Initial Cut of functionality.Hybrid for both Clients and the Manager - [X] New Cwtch level APIs for Internal Conversation Management (`NewConversation`, `InternalInsertMessage` and `SignMessage` - these are needed because Extensions now need to manage inserting messages on behalf of a conversation. - [X] Hybrid Groups integration test that simple sets up a group and has alice post a message and check that 1) bob receives the message and 2) alice acks the message - [X] Channel routing based on Overlay Number (see code comments) - [X] Moving some Test Utils around to make writing integration tests cleaner. There will probably be more of these. Things that are definitely missing: - [ ] Group Level Encryption / Key Management - [ ] ACL / Membership Checking - [ ] Offline Delivery - [ ] Queue for Offline Delivery (essential for above) - [ ] Group Metadata Management (add/edit/rotate key etc.) - [ ] Any checking that clients are following the protocol defined by the formal model However, even without all these things, I think now is the right time to review this PR and get a feel for this code. All those other things are nuanced and will require detailed review and I don't want them lost while reviewing all the stuff in this PR - which is mostly a structural sketch.
sarah added 1 commit 2024-01-26 21:42:23 +00:00
continuous-integration/drone/pr Build is pending Details
25e86426b2
WIP: Hybrid Groups Initial Sketch
dan was assigned by sarah 2024-01-26 21:57:06 +00:00
dan reviewed 2024-02-05 08:27:44 +00:00
dan left a comment
Owner

Ok I got through the first few files only, then the cat let me know it was bed time. Will pick up and continue soon!

Ok I got through the first few files only, then the cat let me know it was bed time. Will pick up and continue soon!
@ -405,3 +408,3 @@
log.Debugf("restartFlow: Creating a New Protocol Engine...")
app.engines[profile.GetOnion()] = engine
eventBus.Publish(event.NewEventList(event.ProtocolEngineCreated))
eventBus.Publish(event.NewEventList(event.ProtocolEngineCreated, event.Handle, profile.GetOnion()))
Owner

Can you add/update the comment above event.ProtocolEngineCreated in common.go. We don't type arguments to events but I've been trying to at least leave comments there about how to use them

Can you add/update the comment above event.ProtocolEngineCreated in common.go. We don't type arguments to events but I've been trying to at least leave comments there about how to use them
@ -515,6 +518,7 @@ func (app *application) eventHandler() {
log.Infof("peer appearing offline, not launching listen threads or connecting jobs")
app.ConfigureConnections(onion, false, false, false)
} else {
log.Infof("configuring online connections for peer")
Owner

add onion address else this line becomes noise when multiple peers activating (or remove log)

add onion address else this line becomes noise when multiple peers activating (or remove log)
@ -54,6 +54,7 @@ func (q *infiniteQueue) resize() {
// Add puts an element on the end of the queue.
func (q *infiniteQueue) Add(elem Event) {
Owner

delete

delete
@ -0,0 +56,4 @@
// This file contains code for the Hybrid Group / Managed Group types..
type HybridGroupMessage struct {
Author string // the authors cwtch address
MemberGroupID uint32
Owner

why not GroupID and MessageID?


ah is it not the ID of hte group but some member ID in the group? i see

and the MemeberMessageID?

why not GroupID and MessageID? --- ah is it not the ID of hte group but some member ID in the group? i see and the MemeberMessageID?
@ -0,0 +67,4 @@
func AuthenticateMessage(message HybridGroupMessage) bool {
messageCopy := message
messageCopy.Signature = []byte{}
// Otherwise we derive the public key from the sender and check it against that.
Owner

otherwise?

otherwise?
@ -0,0 +72,4 @@
if err == nil {
data, err := json.Marshal(messageCopy)
if err == nil && len(decodedPub) >= 32 {
return ed25519.Verify(decodedPub[:32], data, message.Signature)
Owner

only the first 32 bytes of an decoded onion address are the public key? can we make a like onion utils file and functions somewhere so it's a little easier to read / write with out having to remember / google the structure of an onion address?

only the first 32 bytes of an decoded onion address are the public key? can we make a like onion utils file and functions somewhere so it's a little easier to read / write with out having to remember / google the structure of an onion address?
dan reviewed 2024-02-09 17:57:47 +00:00
dan left a comment
Owner

we have messages from an "admin" with ManageGroup true in the ACL but how does this get setup or changed? every new member is added with DefaultACL, we're missing messages about adjusting the ACL of members so we could create new admins, demote old ones, etc?

... reads top comment showing things still undo... ahh..

we have messages from an "admin" with ManageGroup true in the ACL but how does this get setup or changed? every new member is added with DefaultACL, we're missing messages about adjusting the ACL of members so we could create new admins, demote old ones, etc? ... reads top comment showing things still undo... ahh..
dan reviewed 2024-02-09 18:40:20 +00:00
@ -0,0 +121,4 @@
// handleRotateKeyEvent rotates the encryption key for a given group
// assumes we are called after an event provided by an authorized peer (i.e. ManageGroup == true)
func (f *ManagedGroupFunctionality) handleRotateKeyEvent(profile peer.CwtchPeer, conversation model.Conversation, rke RotateKeyEvent) {
Owner

Do we need to store the old keys so we can reconstruct message history? or is it that if we have the invite, we'll have the first key, and subequently can read the rotate messages in order, etc, and reconstruct, but only if we have full history?

Do we need to store the old keys so we can reconstruct message history? or is it that if we have the invite, we'll have the first key, and subequently can read the rotate messages in order, etc, and reconstruct, but only if we have full history?
@ -0,0 +127,4 @@
profile.SetConversationAttribute(conversation.ID, keyScope, keyB64)
}
func (f *ManagedGroupFunctionality) handleNewMessageEvent(profile peer.CwtchPeer, conversation model.Conversation, nme NewMessageEvent) {
Owner

so a group manager can also send messages? relay them? what's the imagined use case here? group manager replacing servers?

so a group manager can also send messages? relay them? what's the imagined use case here? group manager replacing servers?
@ -0,0 +141,4 @@
log.Errorf("unable to decrypt hybrid group message: %v", err)
return
}
//
Owner

ah we dont do anything with it yet?

ah we dont do anything with it yet?
@ -0,0 +145,4 @@
}
}
func (f *ManagedGroupFunctionality) handleNewClearMessageEvent(profile peer.CwtchPeer, conversation model.Conversation, nme NewClearMessageEvent) {
Owner

what is the difference between receiving a message and a clear message from a group manager?

what is the difference between receiving a message and a clear message from a group manager?
@ -0,0 +129,4 @@
if err != nil {
return err
}
// enable channel 2 on this conversation (hybrid groups management channel)
Owner

what are channels?

what are channels?
@ -88,0 +96,4 @@
return true
}
if requestedChannel == 1 {
return false // channel 1 is mapped to channel 0 for backwards compatibility
Owner

? this hasn't launched? backwards compatibility with what? and why return false if it's "mapped to 0"? we have 0 so isnt this kinda true?

? this hasn't launched? backwards compatibility with what? and why return false if it's "mapped to 0"? we have 0 so isnt this kinda true?
@ -19,1 +26,4 @@
const OverlayFileSharing = 200
// ManageGroupEvent is the canonical identifier for the manage group overlay
const OverlayManageGroupEvent = 0x302
Owner

shouldn't there also be a 0x300 for the admin channel 0 then?

I'm not sure I get why we're overloading channel into overlay here and not just another byte sized field in managed group messages? don't this mean adding binary logic somewhere to check an overlay contains 0x300 then?

shouldn't there also be a 0x300 for the admin channel 0 then? I'm not sure I get why we're overloading channel into overlay here and not just another byte sized field in managed group messages? don't this mean adding binary logic somewhere to check an overlay contains 0x300 then?
@ -424,0 +443,4 @@
}
func (cp *cwtchPeer) resolveChannel(overlay int) int {
requestedChannel := overlay & 0xFF
Owner

right like this. we use big json blob structs for so much, why bit packing for this?

also, is there an envisioned use for the range between 0xFF and 0x300? that seems abandoned?

right like this. we use big json blob structs for so much, why bit packing for this? also, is there an envisioned use for the range between 0xFF and 0x300? that seems abandoned?
@ -439,0 +469,4 @@
var cm model.MessageWrapper
err = json.Unmarshal([]byte(message), &cm)
// we are now explictly rejecting invalidly encoded messages...
Owner

was this happening a lot?

seems like that's a failing of our API then. perhaps the exposed send message shouldn't take a pre encoded message wrapper but the arguments ti construct one?

was this happening a lot? seems like that's a failing of our API then. perhaps the exposed send message shouldn't take a pre encoded message wrapper but the arguments ti construct one?
@ -439,0 +476,4 @@
requestedChannel := cp.resolveChannel(cm.Overlay)
// if this channel has not been registered for a particular conversation
// then default to channel = 0;
if conversationInfo.HasChannel(requestedChannel) {
Owner

resolveChannel will turn all legacy overlays to have channel 0 and hasChannel auto trues 0.

for managed groups was it 0 or 2 that is the admin channel? do we need this manual setting? wont managed group messages have a channel explicitly set?

resolveChannel will turn all legacy overlays to have channel 0 and hasChannel auto trues 0. for managed groups was it 0 or 2 that is the admin channel? do we need this manual setting? wont managed group messages have a channel explicitly set?
@ -1482,9 +1558,23 @@ func (cp *cwtchPeer) storeMessage(handle string, message string, sent time.Time)
}
}
var cm model.MessageWrapper
Owner

this check again? at least make it a private helper function like validateMessageWrapper

this check again? at least make it a private helper function like `validateMessageWrapper`
@ -0,0 +79,4 @@
// ***** Cwtch Server management *****
app := app2.NewApp(acn, "./storage", app2.LoadAppSettings("./storage"))
managerApp := app2.NewApp(acn, "./managerstorage", app2.LoadAppSettings("./managerstorage"))
Owner

why a second app for the manager profile?

why a second app for the manager profile?
@ -0,0 +122,4 @@
t.Fatalf("could not create hybrid contact (bob): %v", err)
}
// Now we can allow alice, bob and carol to create a new hybrid group...
Owner

nit: there is no carol

nit: there is no carol
Some checks are pending
continuous-integration/drone/pr Build is pending
This pull request has changes conflicting with the target branch.
  • .gitignore
  • model/conversation.go
  • peer/cwtch_peer.go
Sign in to join this conversation.
No description provided.