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

Avoid leaking core keys and pre-have messages to devices with the project key, but without project access #268

Open
5 of 6 tasks
gmaclennan opened this issue Sep 14, 2023 · 3 comments · May be fixed by #390
Open
5 of 6 tasks

Comments

@gmaclennan
Copy link
Member

gmaclennan commented Sep 14, 2023

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 or capability record is indexed. We could then listen on both events:

  1. coreOwnership record added -> look up capability record for device -> if it exists then add cores listed in coreOwnership record, otherwise ignore
  2. capability record added -> look up core ownership for device -> if it exists add cores, otherwise ignore.

Tasks

  • Wait for indexing to be done before returning data #228
  • Add a onValue() method to DataType - I think this is better than just emitting a value event for every record, because that would add a lot of unnecessary overhead emitting thousands of events.

    There isn't really an overhead for emitting a value for every record, since if no listeners are attached this is just a no-op, at least there is no more overhead than doing a check for listeners ourselves. However attaching a listener to a datatype with lots of records, like the observation type, would have a huge overhead, because it would be called hundreds or thousands of times after an initial sync, so we should probably not expose this to the API and just use it internally for records like core ownership and capabilities where we know there are not many records.

  • feat: dataType.on('updated-docs') when docs update #396
  • Add a capabilities.on('update', updateCapabilities) event. Internally it needs to listen for both new role records, but also new coreOwnership 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).
  • Add cores once capabilities are read and a device is confirmed as having a role in the project
  • Only send "pre-haves" to devices that have sync capability for a particular namespace
@tomasciccola tomasciccola self-assigned this Nov 16, 2023
@gmaclennan gmaclennan changed the title Add non-auth cores via coreOwnership and capability records Avoid leaking core keys and pre-have messages to devices with the project key, but without project access Nov 30, 2023
@gmaclennan
Copy link
Member Author

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 role and coreOwnership records, so I add a new event for all data types in #396. I'll stack a PR that uses this to emit updated capabilities events, and another PR that then adds cores and sends pre-haves based on capabilities.

@ximenabb ximenabb added the mvp Requirement for MVP label Feb 8, 2024
@EvanHahn EvanHahn assigned EvanHahn and unassigned gmaclennan May 22, 2024
EvanHahn added a commit that referenced this issue May 29, 2024
This avoids leaking "have" data to unapproved peers. Most likely, this
is peers with the project key that aren't in the project.

Partly addresses [#268].

[#268]: #268
@EvanHahn
Copy link
Contributor

@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.

EvanHahn added a commit that referenced this issue Aug 27, 2024
This is part of the solution for [#268].

[#268]: #268

Co-Authored-By: Gregor MacLennan <gmaclennan@digital-democracy.org>
EvanHahn added a commit that referenced this issue Aug 27, 2024
#781)

This is part of the solution for [#268].

[#268]: #268

Co-authored-by: Gregor MacLennan <gmaclennan@digital-democracy.org>
EvanHahn added a commit that referenced this issue Aug 28, 2024
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 <gmaclennan@digital-democracy.org>
@EvanHahn
Copy link
Contributor

The MVP parts of this work are done.

@EvanHahn EvanHahn removed their assignment Aug 28, 2024
@EvanHahn EvanHahn removed the mvp Requirement for MVP label Aug 28, 2024
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 a pull request may close this issue.

4 participants