-
Notifications
You must be signed in to change notification settings - Fork 1
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
Avoid leaking core keys and pre-have messages to devices with the project key, but without project access #268
Comments
I went down a bit of a rabbit hole with this task trying to figure out the best way of doing it, and have gone down several "dead-ends" that resulted in lots of complicated code. In the end I realised that we need a way of subscribing to doc updates for |
@gmaclennan I "sketched" out a fix for this in #737. Could you give it a look? If my draft looks good, I'll fix it up properly. |
This is part of the solution for [#268]. [#268]: #268 Co-Authored-By: Gregor MacLennan <[email protected]>
#781) This is part of the solution for [#268]. [#268]: #268 Co-authored-by: Gregor MacLennan <[email protected]>
Previously, we would add cores without checking anything. Now, we only add them once (1) their role is OK (2) they have a good core ownership record. Partly addresses [#268]. [#268]: #268 Co-Authored-By: Gregor MacLennan <[email protected]>
The MVP parts of this work are done. |
Description
#264 implements sharing of all hypercore keys over extension messages. This introduces a small security risk: a bad actor who has a project key could make other clients add lots of cores that do not actually belong to the project and inject data.
Ideally we change this before the MVP because it avoids having to maintain backwards compatibility.
One solution to this is to verify whether cores belong to a device that is part of the project before indexing them, but this still means that disk space could be taken with cores that are not part of the project.
A more robust solution (but we should maybe also do the solution above) is to only add
auth
namespace cores via extension messages (as we were doing before), and to add other cores once we know that they are part of the project."being part of the project" means in this case that any capability record refers for a given device, because we still want to include cores that are from a device that was previously part of the project and is now "blocked" (if we want to exclude that data we should do so at index time, or avoid downloading their data at sync time, but we should still keep the cores in the project).
We will also need to check the
coreOwnership
records to get the actual core keys for a device.Unfortunately our indexing is unordered, so we might get core ownership records before capability records, or vice-versa.
One easy way to solve this problem is to listen for the
index-done
events, and for each event read all the capability records, look up the corresponding coreOwnership records, and add any missing cores. However this could add a lot of overhead -index-done
happens a lot, e.g. whenever a user adds or edits data, and on every sync, and in many cases there will be no cores to add.A more efficient way would be to add an event whenever a new
coreOwnership
orcapability
record is indexed. We could then listen on both events:coreOwnership
record added -> look up capability record for device -> if it exists then add cores listed incoreOwnership
record, otherwise ignorecapability
record added -> look up core ownership for device -> if it exists add cores, otherwise ignore.Tasks
Add aonValue()
method toDataType
- I think this is better than just emitting avalue
event for every record, because that would add a lot of unnecessary overhead emitting thousands of events.capabilities.on('update', updateCapabilities)
event. Internally it needs to listen for both newrole
records, but also newcoreOwnership
records, because we only know the role of the project creator via their core ownership record (because they own the auth core with the key that matches the project key).sync
capability for a particular namespaceThe text was updated successfully, but these errors were encountered: