Refactor access controller by haadcode · Pull Request #128 · orbitdb-archive/ipfs-log

haadcode · 2018-06-25T11:20:07Z

This PR refactors the access controller in a way that the ACL handles all keys, signing and verification related functionality. The general idea is that Log/Entry don't know anything about the keys or keystore and only gets and acl object (the access controller) which encapsulates the functionality.

I did the previous commits in a bit of a rush, so there may be some weirdness here. Would love to hear your feedback @coyotespike, @shamb0t and @vvp.

I'm unsure if the ACL api is what we want to do in the end, but at least this would give us a "clean" separation of the acl and the log.

I'll try to check this again myself in the next couple of weeks to make sure everything makes sense, but for now, I hope this helps you to proceed with the refactoring / acl work that you're doing.

* added docs and tests for custom verification * fix sync join calls in replication test * added verifyEntry constructor argument * add verifyEntry param to Factory functions. Don't use setter for it

haadcode · 2018-06-25T11:22:32Z

@fazo96 would love to hear your feedback too on this. Your PR #123 has been open for a long time (sorry about that! :/) and this one is based on yours but with a very different code as to how to do the custom verification. I believe this would still give you that functionality but with the difference that instead of passing a custom verification function, you'd have to write a custom access controller (which I think would make sense generally if you're planning to check the ACL against a smart contract).

fazo96 · 2018-06-25T11:34:12Z

hey @haadcode, thanks for involving me in this :)

I think this ACL refactor looks much better designed than my PR and will get me the same functionality.

Also thrilled by how easy it looks to pass a custom key to the ACL 👍

One thing I can't wrap my head around is what happens if an ACL is not passed? No more signing of the entries by default? I'm probably just missing something

haadcode · 2018-06-25T12:01:45Z

One thing I can't wrap my head around is what happens if an ACL is not passed? No more signing of the entries by default? I'm probably just missing something

I left out that part and we should discuss what the default behaviour should be. You're correct that right now the entries don't get signed if an acl is not passed (which prolly doesn't have a lot of use cases as it breaks the integrity of the log). I suppose we could either throw an error or defer to a default access controller (like it used to be / is now). Any thoughts?

@fazo96 do you have any links to how you're using the custom verification in your code? Would love to understand your use case better so that we can take that into consideration designing all this.

fazo96 · 2018-06-25T12:27:56Z

@haadcode sure, here is the custom OrbitDB Store we use in the current Chlu implementation:

https://github.com/ChluNetwork/chlu-ipfs-support/blob/master/src/modules/orbitdb/store.js

For our validation needs, there's three points at which we can validate content before accepting it:

before joining logs (using either my PR Custom verification and custom signing #127 or this PR and a custom ACL)
in the updateIndex function, so we don't put invalid data in the index
when reading from the index
after reading from the index

We haven't much validation going on related to orbit-db data, but the ReviewRecords we store are supposed to be independently validatable objects and they are, so the validation for now occures in the code that comes after reading from the orbit-db index (point 4).

This is not ideal because the index can get polluted by any fake information, and also we allow review records to be updated and the current implementation makes it possible for anyone to create a bogus update and thus making the review record invalid for everyone.

The point of my PR #127 was to make point 1 possible and orbitdb-archive/orbit-db-store#21 already made point 2 possible. point 3 is easy because we have a custom index and point 4 does not involve orbit-db.

So we have an external, async validation function that can be called with the CID of the review record (passed in the payload of the orbit-db operation). This function will return true if the data is valid and should be kept in orbit-db.

In conclusion, our use case was making sure we have the means of keeping invalid data out of the index or even out of the oplog, depends on the user/application of our Chlu protocol (maybe it is preferable to keep invalid data in the oplog for further analysis and debugging, for example, but still keep it out of the index).

Another important point in favour is that to authenticate users we use a Decentralized Identifier (DID) that has its own keypair and the custom ACL in this PR makes it easier for us to reuse DID keypairs for orbit-db signing and validating.

coyotespike

This PR makes log.js so much cleaner! Nice! I have a questions about append, noted below, and will think about how this interacts with other PRs.

coyotespike · 2018-06-25T21:02:26Z

   * @return {Promise<Log>}       New Log
   */
-  static fromEntry (ipfs, sourceEntries, length = -1, exclude, onProgressCallback) {
+  static fromEntry (ipfs, sourceEntries, length = -1, exclude, onProgressCallback, verifyEntry) {


Log no longer takes verifyEntry, and so neither should fromEntry. That argument can be removed.

Thank you! 👍Will fix all the verifyEntry args as per your comments.

coyotespike · 2018-06-25T21:02:55Z

    // TODO: need to verify the entries with 'key'
    return LogIO.fromEntry(ipfs, sourceEntries, length, exclude, onProgressCallback)
-      .then((data) => new Log(ipfs, data.id, data.values))
+      .then((data) => new Log(ipfs, data.id, data.values, null, null, null, verifyEntry))


And same here, Log now takes six arguments, with the seventh being an ACL.

coyotespike · 2018-06-25T22:10:20Z

   * @return {Promise<Log>}      New Log
   */
-  static fromMultihash (ipfs, hash, length = -1, exclude, key, onProgressCallback) {
+  static fromMultihash (ipfs, hash, length = -1, exclude, acl, onProgressCallback, verifyEntry) {


verifyEntry not necessary

coyotespike · 2018-06-25T22:10:26Z

   * @param {string} hash        Multihash (as a Base58 encoded string) to create the log from
   * @param {Number} [length=-1] How many items to include in the log
   * @param {Function(hash, entry, parent, depth)} onProgressCallback
+   * @param {Function} [verifyEntry]   Custom entry verification function


verifyEntry not necessary

coyotespike · 2018-06-25T22:10:35Z

    // TODO: need to verify the entries with 'key'
    return LogIO.fromMultihash(ipfs, hash, length, exclude, onProgressCallback)
-      .then((data) => new Log(ipfs, data.id, data.values, data.heads, data.clock, key))
+      .then((data) => new Log(ipfs, data.id, data.values, data.heads, data.clock, acl, verifyEntry))


verifyEntry not necessary

coyotespike · 2018-06-25T22:10:48Z

   * @return {Promise<Log>}      New Log
   */
-  static fromEntryHash (ipfs, hash, id, length = -1, exclude, key, keys, onProgressCallback) {
+  static fromEntryHash (ipfs, hash, id, length = -1, exclude, acl, onProgressCallback, verifyEntry) {


verifyEntry not necessary

coyotespike · 2018-06-25T22:10:51Z

   * @param {string} hash        Multihash (as a Base58 encoded string) of the Entry from which to create the log from
   * @param {Number} [length=-1] How many entries to include in the log
   * @param {Function(hash, entry, parent, depth)} onProgressCallback
+   * @param {Function} [verifyEntry]   Custom entry verification function


verifyEntry not necessary

coyotespike · 2018-06-25T22:10:59Z

   * @return {Promise<Log>}      New Log
   */
-  static fromJSON (ipfs, json, length = -1, key, keys, timeout, onProgressCallback) {
+  static fromJSON (ipfs, json, length = -1, acl, timeout, onProgressCallback, verifyEntry) {


verifyEntry not necessary

coyotespike · 2018-06-25T22:11:05Z

-    return LogIO.fromJSON(ipfs, json, length, key, timeout, onProgressCallback)
-      .then((data) => new Log(ipfs, data.id, data.values, null, null, key, keys))
+    return LogIO.fromJSON(ipfs, json, length, timeout, onProgressCallback)
+      .then((data) => new Log(ipfs, data.id, data.values, null, null, acl, verifyEntry))


verifyEntry not necessary

coyotespike · 2018-06-26T20:55:58Z

@@ -198,20 +200,13 @@ class Log extends GSet {
   * @return {Log}   New Log containing the appended value
   */
  async append (data, pointerCount = 1) {


[edit: this comment previously worried that we weren't checking for write permissions in append, but now I see that Entry.create calls sign which calls canAppend 👍 ]

Your previous comment was a good point though. I was thinking if we should make it (calling canAppend or sign) explicit and call it in the log instead of in Entry?

I already forgot it a second time 😂 so we could either make it explicit, or make a generalized testing suite for the access controller API to ensure that all access controllers conform to this API.

coyotespike · 2018-06-27T15:50:25Z

ipfs-log just takes an acl - so it would seem that we can still pass an acl down from orbit-db through orbit-db-store.

Do you intend acl.js to move to orbit-db? I am curious about the split of responsibilities. The access controllers on the dynamic permissions branch are setting and getting capabilities. acl.js uses those keys, and the keystore, to check permissions with sign and verify etc.

The smart contract access controller needs to do both, because it can't provide a list of keys. I suppose we'll just pass down the smart contract access controller.

coyotespike · 2018-07-03T19:30:45Z


-  static async signEntry (keystore, entry, key) {
-    const signature = await keystore.sign(key, Buffer.from(JSON.stringify(entry)))
+  static async sign (entry, acl) {


A smart contract access controller will want to augment the entry with a chainSignature and chainPublicKey, or something like that.

So perhaps acl.sign should return the entry, and the default acl is responsible for adding the key and signature.

thiagodelgado111 · 2018-07-06T15:47:12Z

@@ -0,0 +1,53 @@
+class ACL {
+  constructor (keystore, key, keys = []) {
+    this._keystore = keystore


I think here it should only get sign, getPubKey and verify functions instead of direct access to the keys. We should keep sign, getPubKey and verify here though so we can handle encoding/parsing on ipfs-log

thiagodelgado111 · 2018-07-06T15:51:30Z

@@ -0,0 +1,53 @@
+class ACL {


Maybe we could call it IntegrityChecker to avoid confusion around orbit-db vs ipfs-log ACL

thiagodelgado111 · 2018-07-06T15:53:18Z

+    return this._key.getPublic(format)
+  }
+
+  canAppend (signingKey) {


If we pass functions from orbit-db access controller to it, we wouldn't need this anymore

thiagodelgado111 · 2018-07-06T16:02:00Z

    const nexts = Object.keys(this.traverse(this.heads, pointerCount))
    // Create the entry and add it to the internal cache
-    const entry = await Entry.create(this._storage, this._keystore, this.id, data, nexts, this.clock, this._key)
+    const entry = await Entry.create(this._storage, this._acl, this.id, data, nexts, this.clock)


Instead of passing the whole ACL object here, the Entry.create method could be broken up into create/sign/put. It might make it easier to understand how it actually works – since one might assume this is simply creating an entry instance – and then we can pass just the public key to be used as a fallback for the clockId property. Maybe a TODO for another PR?

thiagodelgado111 · 2018-07-06T16:03:42Z

+    return this._key
+  }
+
+  getPublicSigningKey (format = 'hex') {


I think ipfs-log always uses this format. Is it going to change or can we assume it'll always be 'hex'?

thiagodelgado111 · 2018-07-06T16:13:02Z

    return entries.reduce(reduceTailHashes, [])
  }
+
+  static difference (a, b) {


Is it similar to utils/difference?

thiagodelgado111 · 2018-07-06T16:17:07Z

-    this._key = key 
-    this._keys = Array.isArray(keys) ? keys : [keys]
+    // Access Controller
+    this._acl = acl


Like @fazo96 mentioned in his comment (#128 (comment)), maybe we should add some validation here that checks if the ACL exists and if it follows the "interface" we need. If it doesn't, we throw an error. Is there a scenario where it wouldn't be necessary?

thiagodelgado111 · 2018-07-07T02:35:06Z

-    if (keystore && signKey) {
-      entry = await Entry.signEntry(keystore, entry, signKey) 
+    // If acl was passed, sign the entry (=authorize)
+    if (acl) {


I think this should be a breaking change, providing an ACL should be mandatory from now on

haadcode · 2018-09-03T14:29:15Z

Closing as obsolete in favor of #159.

fazo96 and others added 3 commits April 24, 2018 12:05

implement custom verification function for Log entries

46a1848

* added docs and tests for custom verification * fix sync join calls in replication test * added verifyEntry constructor argument * add verifyEntry param to Factory functions. Don't use setter for it

updated dev deps

6b99791

Refactor access control

91ddac6

haadcode added the In Progress label Jun 25, 2018

haadcode mentioned this pull request Jun 25, 2018

Use refactored ipfs-log orbitdb-archive/orbit-db-store#25

Closed

coyotespike reviewed Jun 27, 2018

View reviewed changes

haad added 2 commits July 3, 2018 10:41

Remove unnecessary await from Entry.verify

fc64e13

Remove obsolete verifyEntry arguments from Log

d45fbc9

coyotespike reviewed Jul 3, 2018

View reviewed changes

thiagodelgado111 reviewed Jul 6, 2018

View reviewed changes

thiagodelgado111 reviewed Jul 7, 2018

View reviewed changes

RichardLitt mentioned this pull request Jul 10, 2018

Start a community call orbitdb/orbitdb#394

Closed

thiagodelgado111 mentioned this pull request Jul 10, 2018

WIP: Move signing and verification implementation out of ipfs-log #131

Closed

11 tasks

crazybuster mentioned this pull request Jul 18, 2018

Make use of verifyEntry result #141

Merged

thiagodelgado111 mentioned this pull request Aug 10, 2018

Custom signing, verification and permission checking for at the log #145

Closed

9 tasks

thiagodelgado111 mentioned this pull request Aug 30, 2018

Custom signing, verification and permission checking at the log #159

Merged

2 tasks

haadcode removed the In Progress label Sep 3, 2018

haadcode closed this Sep 3, 2018

haadcode deleted the feat/acl-refactor branch September 4, 2018 10:51

Uh oh!

Uh oh!

Conversation

haadcode commented Jun 25, 2018

Uh oh!

haadcode commented Jun 25, 2018

Uh oh!

fazo96 commented Jun 25, 2018

Uh oh!

haadcode commented Jun 25, 2018

Uh oh!

fazo96 commented Jun 25, 2018

Uh oh!

coyotespike left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coyotespike Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coyotespike commented Jun 27, 2018

Uh oh!

coyotespike Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiagodelgado111 Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiagodelgado111 Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haadcode commented Sep 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

coyotespike left a comment •

edited

Loading

coyotespike Jun 26, 2018 •

edited

Loading

coyotespike Jul 3, 2018 •

edited

Loading

thiagodelgado111 Jul 6, 2018 •

edited

Loading

thiagodelgado111 Jul 6, 2018 •

edited

Loading