-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: Secure document encryption key exchange #2891
feat: Secure document encryption key exchange #2891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial batch of comment bellow. There are a few parts I want to go over again but overall it looks really nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a couple todos to resolve before merge. Thanks Islam!
internal/kms/pubsub.go
Outdated
return nil, err | ||
} | ||
|
||
ctx := grpcpeer.NewContext(s.ctx, net.NewGRPCPeer(peerID)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: It doesn't make sense to create a dependency to the net package only for this. You can even create the grpcpeer.Peer
struct inline if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
net/server.go
Outdated
@@ -409,3 +409,47 @@ func (s *server) updateReplicators(evt event.Replicator) { | |||
} | |||
s.peer.bus.Publish(event.NewMessage(event.ReplicatorCompletedName, nil)) | |||
} | |||
|
|||
func (s *server) AddPubSubTopic(topicName string, handler rpc.MessageHandler) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: addPubSubTopic
already exists. You can simply call it from this function if you need it to be public.
Took into consideration all comments and got approval from Fred. Any further improvement can be handled later.
Relevant issue(s)
Resolves #2856 #2909
Description
This PR introduces a secure mechanism for exchanging document encryption keys in our decentralized network. It supports both whole-document and field-level encryption, enhancing data privacy without compromising the system's distributed nature.
Data Flow and Key Exchange Process
Document Creation:
Encryption
IPLD block is created, containing the docID, optional fieldName (for field-level encryption), and the encryption key itself.Encryption
block by storing its CID.Encryption Detection:
Encryption
block.Key Request and Retrieval:
Key Reception and Storage:
Decryption and Merging:
executeMerge
process, the node collects CIDs of allEncryption
blocks that are not available locally.Technical Details
Current Limitations and Future Work
Testing Improvements
Introduced an "assert stack" for integration tests, providing detailed failure contexts (e.g., 'path: commits[2].links[1].cid' instead of 'doc: 1').
These changes significantly enhance our system's security, enabling confidential data storage and transmission in a decentralized environment, while laying the groundwork for more advanced encryption and access control features. The introduction of the KMS abstraction provides flexibility for future improvements in key management.
Tasks
How has this been tested?
With integration tests.
Specify the platform(s) on which this was tested: