[WIP] Collections support #1940
Conversation
|
A possible use case for collection key generation in more apps (i.e. add key generation to blockstack.js) would be to collections of data that are shared with a group of people across apps. |
There was a problem hiding this comment.
Generally, I have read through the code, and can understand what is happening. There are two things I'd like to see before I would approve this:
- More tests. I see one test for ensuring that the
collectionsNodeKeyis set on an identity keypair - that is good. But there is a lot more that is untested, like everything incollection-utils. - Some end-to-end way to test this. Something like a demo app that simply requests the collection scope. Then, I could at least verify that the collections info is stored in the app's storage.
app/js/utils/collection-utils.js
Outdated
| } | ||
|
|
||
| function writeCollectionKeysToAppStorage(appPrivateKey, hubConfig, keyFile) { | ||
| const compressedAppPrivateKey = `${appPrivateKey}01` |
There was a problem hiding this comment.
Can you explain why you're doing it this way? I would think you would just use getPublicKeyFromPrivate from blockstack.js. This seems like you're doing some other mechanism for getting the public key.
There was a problem hiding this comment.
You're right, I missed that some how. I addressed it in a new commit.
There was a problem hiding this comment.
@hstove testing can be performed using the basic test page here: https://blockstack.github.io/blockstack-collections/page_test/
And running the that same repo locally with npm run start. Should be showing the same files for both apps when using same account.
| || settings.collections[collectionName].length > 0) { | ||
| const defaultCollectionSettings = DEFAULT_NEW_COLLECTION_SETTING_ARRAY | ||
| updateIdentityCollectionSettings(collectionName, defaultCollectionSettings) | ||
| return Promise.resolve(defaultCollectionSettings[0].encryptionKeyIndex) |
There was a problem hiding this comment.
Trying to make sure I understand what's going on here -- is this encryptionKeyIndex intended to be used for bumping in order to perform a key revocation for a given collection type/scope?
There was a problem hiding this comment.
Yes that's correct. This gives us a deterministic way of generating encryption keys for collections.
There was a problem hiding this comment.
I've been testing a local build of this in beta mode, with the blockstack.js dependency bumped to the latest preview, and with the https://staging-hub.blockstack.xyz Gaia hub.
Testing contacts collections using the demo apps:
- https://blockstack.github.io/blockstack-collections/page_test/
- A local build of that ^
- And local build of https://github.com/yknl/blockstack-contacts
Everything seems to be working as expected. The settings.json file is encrypted and stored next to profile.json. Each app origin receives a .collection.keys file within a bucket unique to each app and collection type, and is able to encrypt & decrypt collection files from other apps.
The collection key derivation logic is a bit hard to follow. It looks correct, but I left a comment where I was unclear on the inputs going into the collection node.
| .update(`${collectionTypeName}${collectionIdentifier}${this.salt}`) | ||
| .digest('hex') | ||
| const collectionIndex = hashCode(hash) | ||
| const collectionNode = this.hdNode.deriveHardened(collectionIndex) |
There was a problem hiding this comment.
We should probably using a longer derivation path here. All of these inputs (collection name, collection ident, salt) end up as only 32 bits of uniqueness.
This is the same problem with app-private-keys that PR #1496 was trying to solve.
This is the blockstack.js function that is able to perform better derivation, which we can leverage for collection HD nodes: https://github.com/blockstack/blockstack.js/blob/e4864ad41f376d963d4917b95996a6032f992789/src/wallet.ts#L289-L312
| .then(results => { | ||
| const collectionKeys = results[0] | ||
| const collectionHubConfigs = results[1] | ||
| return updateAppCollectionKeys( |
There was a problem hiding this comment.
@yknl This adds a hard requirement to the blockstack auth flow to be online. In the current flow it is possible to sign-in while offline (for android this works offline, for the web the manifest is needed when not cached).
This PR implements handling of collection scopes, collections storage bucket and encryption key generation based on the design proposal. It makes use of the identity settings feature for storage of encryption key indexes. The key indexes are used to deterministically generate encryption keys for collection buckets. (Credits to @friedger for the suggestion) Key rotation is accomplished by incrementing the index. This reduces the impact of data corruption if randomly generated keys are used instead. This is changed from the original design where encryption keys are generated based on the list of apps granted access.
Some open questions are whether the key generation logic should exist in blockstack.js or in the browser.