Milestone 2 #77

macor161 · 2018-11-20T17:13:37Z

File encryption
Extract ObjectACL to its own repo
Display gas usage in tests
Bytecode size script
AragonOS upgrade
Javascript library documentation
Functions to permanently delete files

Show gas usage when launching tests

…into dev

Datastore acl extract

Security alerts

coveralls · 2018-11-20T17:20:57Z

Pull Request Test Coverage Report for Build 116

79 of 79 (100.0%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.2%) to 98.824%

Totals
Change from base Build 104:	-1.2%
Covered Lines:	193
Relevant Lines:	193

💛 - Coveralls

bingen

I started reviewing this, will continue later as it's a huge PR ;-) but I want to let some comments so you can start reviewing them. As it happened before, I don't know your codebase as well as you, so feel free to correct me wherever I'm wrong, please ;-)

bingen · 2018-12-07T19:49:04Z

contracts/Datastore.sol


    modifier onlyFileOwner(uint256 _fileId) {
        require(permissions.isOwner(_fileId, msg.sender));
        _;
    }    

-    function initialize(address _datastoreACL) onlyInit public
-    {
+    function initialize(address _objectACL) onlyInit public {


It may be a good idea to enforce ObjectACL here instead of address.

Yup totally right! Done! :)

bingen · 2018-12-07T19:59:30Z

contracts/Datastore.sol

+    function getFileEncryptionKey(uint _fileId) external view returns(string) {
+        if (hasReadAccess(_fileId, msg.sender)) {
+            FileLibrary.File storage file = fileList.files[_fileId];
+            return file.cryptoKey;


I don't see how this cryptoKey is used. Is it the actual private key used to encrypt files? If so, anybody can see it as it's in the blockchain, right?

You are totally right! :) In the next two milestones, we will store the encryption key using a privacy layer (keep, enigma or NuCypher) but in the meantime we left it there for the sake of convenience.

Sounds good. Where are you now using that key?

here and here and a few other places I think

bingen · 2018-12-07T20:01:00Z

contracts/Datastore.sol

+    function deleteFile(uint _fileId, bool _isDeleted, bool _deletePermanently) public onlyFileOwner(_fileId) {
+        if (_isDeleted && _deletePermanently) {
+            fileList.permanentlyDeleteFile(_fileId);
+            emit DeleteFilePermanently(msg.sender);            


Shouldn't these delete events include the fileId?

Yeah we started doing a clean up of our events (#83)

Hm, I don't think I get it. In that PR it seems you are even removing more fields, up to the point of leaving events empty. How are you planning to use the events. How will you know which objects the events are related to?

Yes sorry Bigen I didn't explain myself very well. We started doing a big clean up of events because the majority of the parameters were not really useful and we didn't used them. So to optimize our bytecode size, we removed every params and a few events we thought were redundant. Now that the clean up is done in #83 we're supposed to start adding more params like fileId and groupId and more that we think can be useful for the users.

Ok, makes sense. I'll have another look at it when events are fulfilled.

We started adding event params in this pull request: #86 We are still debating on what to add next. Maybe for FileChange and SettingsChange we will add the type of change.

bingen · 2018-12-07T20:01:54Z

contracts/Datastore.sol

+    function deleteFilesPermanently(uint256[] _fileIds) public {
+        for(uint256 i = 0; i < _fileIds.length; i++)
+            fileList.permanentlyDeleteFile(_fileIds[i]);
+        emit DeleteFilePermanently(msg.sender);


In case this event finally includes fileId (see previous comment), then it should happen at per file level.

See new commits cherry picked from #86

bingen · 2018-12-07T20:02:27Z

contracts/Datastore.sol

+        require(hasWriteAccess(_fileId, msg.sender));
+
+        fileList.setEncryptionKey(_fileId, _cryptoKey);
+        emit FileContentUpdate(msg.sender);


Shouldn't this event include the fileId too?

Same thing as previous comment :)

bingen · 2018-12-07T20:03:36Z

contracts/Datastore.sol

+    function setEncryptionProvider(EncryptionProvider _encryptionProvider) public {
+        require(settings.encryptionProvider == EncryptionProvider.None);
+        settings.encryptionProvider = _encryptionProvider;
+        emit SettingsChanged(msg.sender);


Wouldn't it be better to emit a more specific event, in order to know exactly which setting was changed?

Yeah we are thinking of adding a param that will represent the type of change but we are still not sure if it's a good trade-off regarding the additional bytecode.

bingen · 2018-12-07T20:04:38Z

contracts/Datastore.sol

+     * @notice Change the encryption provider
+     * @param _encryptionProvider Encryption provider
+     */
+    function setEncryptionProvider(EncryptionProvider _encryptionProvider) public {


If the provider is changed, what happens with all the already existing files? Are they going to be encrypted? Is it possible to have unencrypted and encrypted files in the datastore simultaneously?

Yes what we have in mind (at least in the first couple releases) is to lock the choice of encryption and of storage provider when they are initially selected. also it is possible to have encrypted and unencrypted files in the datastore

But this isn't enforce yet, right? Where is this function being used?
Besides, the same comment could apply to setEncryptionKey. What happens if the file was already encrypted?

No you're right it is only enforced in the setSettings function. I think setEncryptionProvider and setStorageProvider were temporarily added for some tests. We should remove them shortly. setEncryptionKey can be called if we remove access to someone and need to re-encrypt the file.

Alright setEncryptionProvider and setStorageProvider are now removed: #91

@macor161 could you cherry-pick that one here too? thanks!

Those functions are now also removed in this pull request.

bingen · 2018-12-07T20:09:11Z

contracts/Datastore.sol

     * Since switching between storage providers is not supported,
-     * the method can only be called if storage isn't set or already IPFS
+     * the method can only be called if storage isn't set or already IPFS.
+     * Also sets AES as the encryption provider.


Why does it choose a fixed encryption provider? Also, see previous comment about already existing files.

We have a single encryption provider right now, supporting multiple modes and multiple key length. But maybe in the future we will support more providers. As for the encryption, you are totally right, this was a mistake to have the || settings.encryptionProvider == EncryptionProvider.Aes part. hmm.. but then it would prevent setting the storage provider once the encryption provider is set. We must review this function next week.

We reviewed the function and it would indeed make much more sense to have a parameter for the encryption provider: #92

Ok, I think eventually the storage provider and encryption provider parameters should be more general to be able to accommodate any type of provider, but as for now it's IPFS and AES I guess it's fine, and it can be addressed later.

Yeah I agree. We started with the idea of having a general function that accepts a Settings struct as parameter but it is not possible right now with Solidity 0.4.24. So we ended up with a function with all the parameters for every providers but it obviously dosn't scale.

bingen · 2018-12-07T20:10:38Z

contracts/Datastore.sol

-    function createGroup(string _groupName) external auth(DATASTORE_MANAGER_ROLE) returns (uint) {
-        uint id = groups.createGroup(_groupName);
+    function createGroup(string _groupName) external {
+        groups.createGroup(_groupName);
        emit GroupChange(msg.sender);


Maybe a more specific event for group creation would be better. Also, including the group id?

Yes now that the cleanup is done for the events, we are considering adding a NewGroup event or maybe add a parameter to GroupChange that will specify the type of change

bingen · 2018-12-07T20:14:03Z

contracts/Datastore.sol

-        fileList.files[_fileId].isPublic = _isPublic;
+        fileList.setPublic(_fileId, _isPublic);
+
+        if (!_isPublic || (_isPublic && keccak256(_encryptionKey) == keccak256(""))) {


Not sure if I understand this correctly. What does exactly _isPublic mean?

Public files will be unencrypted, and therefore readable by anyone inside or outside the DAO. Private files will be encrypted and only the addresses/groups specified will be able to access it's content.

Ok, I don't understand the purpose of the condition yet.

First part would mean that file is private. Then you are setting the encryption key. But if it was private I guess it already had a key. What happens then? Is the file going to be unencrypted with the old key and re-encrypted with the new one? Otherwise it could become unreadable, right?

Second part holds if it's public and it has an empty encryption key, which sounds redundant: why should it have a key if it's unencrypted? Anyway, then you are setting the key. What for if it's public, i.e., unencrypted?

Yes there seems to be something wrong with this line: if (!_isPublic || (_isPublic && keccak256(_encryptionKey) == keccak256(""))) {. We should have more details monday after our team meeting.

Ok after a review with the team, the condition was an attempt to verify that the user had the right permissions but it turns out we don't need it since there is already a onlyFileOwner check. So we removed it in #93

@macor161 cherry-pick this one here too, please

bingen

Waiting for a unified PR with all changes before continuing review.

bingen · 2019-01-02T15:42:00Z

contracts/Datastore.sol

@@ -55,29 +58,30 @@ contract Datastore is AragonApp {
        string protocol;        
    }

+    /** 
+     *  TODO: Use AesSettings inside Settings when aragon supports nested structs


What do you mean exactly? I guess you mean in the frontend, right? Meanwhile I guess you could comment this struct out, as you already have aesName and aesLength above.

Yes if I remember well nested structs were not supported in the frontend. But we can definitely remove them in the meantime. Done! :)

bingen · 2019-01-02T15:51:40Z

contracts/Datastore.sol

@@ -26,6 +25,7 @@ contract Datastore is AragonApp {
    event NewGroupPermissions(address indexed entity);
    event NewPermissions(address indexed entity);
    event DeleteFile(address indexed entity);
+    event DeleteFilePermanently(address indexed entity);
    event SettingsChanged(address indexed entity);
    event GroupChange(address indexed entity);
    event EntityPermissionsRemoved(address indexed entity);


I see you are always emitting msg.sender as entity for all these events (the name entity sounds a little bit confusing to me then, but no big deal). I still think that adding the modified entity here (File, Group, ...) could be useful.

Sorry for the confusion, the changes were applied in #83 and #86 which are in the development branch. I cherry picked those changes here.

bingen

I left some comments inline, but my main concern is with encryption key management: I'm aware that integration with other services like Keep or Enigma are pending, but I don't think meanwhile it makes any sense to store encryption key on-chain where anybody can access it.
Besides, how is the encryption of the file handled when the encryption key is set? Is it done in the frontend? Where? If there already was a previous key, is the content unencrypted with the old key and then encrypted with the new one? (sorry if this was already answered before)

bingen · 2019-01-03T09:42:35Z

contracts/libraries/FileLibrary.sol

        uint lastFileId;
        mapping (uint => FileLibrary.File) files;
    }

-
-    function addFile(FileList storage _self, string _storageRef, string _name, uint _fileSize, bool _isPublic) internal returns (uint fileId) {
+    function addFile(FileList storage _self, string _storageRef, string _name, uint128 _fileSize, bool _isPublic, string _encryptionKey) internal returns (uint fileId) {
        _self.lastFileId = _self.lastFileId.add(1);

        _self.files[_self.lastFileId] = FileLibrary.File({


I haven't tried it myself, but it seems it's more efficient to just set any non-zero variable one by one:
https://github.com/aragon/aragon-apps/pull/479/files#r231335088
(See line 403/519 if it doesn't show up or it appears hidden as outdated comment)

Nice! I didn't know that!

bingen · 2019-01-03T09:43:28Z

contracts/libraries/FileLibrary.sol

-        * Also represents the total number of files stored.
-        */
+         * Id of the last file added to the datastore. 
+         * Also represents the total number of files stored.


So this means the count starts at 1, not at 0, right? Just curious, are you using 0 to signal non-existing file or something like that?

0 will be used by the root folder when folders are enabled.

bingen · 2019-01-03T09:45:38Z

contracts/libraries/FileLibrary.sol

        return _self.lastFileId;
    }   

    function setFileName(FileList storage _self, uint _fileId, string _newName) internal {
        _self.files[_fileId].name = _newName;
-        _self.files[_fileId].lastModification = now;
+        _self.files[_fileId].lastModification = uint64(now);


It's very unlikely that this overflows, but you may want to consider using this:
https://github.com/aragon/aragonOS/blob/dev/contracts/common/TimeHelpers.sol#L45

Great tip, thanks! However to save some gas, we have a new implementation where we are not storing the lastModification and fileName in the smart contract anymore. We store them using the storage provider (IPFS or Swarm).

bingen · 2019-01-03T11:16:59Z

contracts/libraries/PermissionLibrary.sol

@@ -25,13 +24,10 @@ library PermissionLibrary {
        bytes32 FILE_READ_ROLE;
        bytes32 FILE_WRITE_ROLE;  


I wonder if these constants could be defined here instead of passing them on initialize

Good question! Usually ACL roles are defined in the main smart contract itself but these roles are strictly used with the ObjectACL, and only by the PermissionLibrary. I think it would indeed make more sense to have them in the library.

Done! :) I've also moved DATASTORE_GROUP to the GroupLibrary

macor161

Encryption is indeed done in javascript, here. If it is already encrypted, it uses the same key to re-encrypt new content. If a permission is changed for a user, let's say we remove access to someone, we generate a new key and re-encrypt the file so the user cannot use the old key even if they managed to intercept it in the frontend.

I agree that the key should not be stored in plain text in production. In fact, we are extracting all the encryption logic in another branch since encryption won't be enabled in the initial release. However we've already started the process and it would be pretty time consuming to cherry pick all those changes. Can we show you the final result next week?

macor161 · 2019-01-03T19:37:45Z

contracts/libraries/FileLibrary.sol

-        * Also represents the total number of files stored.
-        */
+         * Id of the last file added to the datastore. 
+         * Also represents the total number of files stored.


0 will be used by the root folder when folders are enabled.

macor161 · 2019-01-03T21:56:09Z

contracts/libraries/FileLibrary.sol

        uint lastFileId;
        mapping (uint => FileLibrary.File) files;
    }

-
-    function addFile(FileList storage _self, string _storageRef, string _name, uint _fileSize, bool _isPublic) internal returns (uint fileId) {
+    function addFile(FileList storage _self, string _storageRef, string _name, uint128 _fileSize, bool _isPublic, string _encryptionKey) internal returns (uint fileId) {
        _self.lastFileId = _self.lastFileId.add(1);

        _self.files[_self.lastFileId] = FileLibrary.File({


Nice! I didn't know that!

bingen · 2019-01-10T18:50:17Z

I agree that the key should not be stored in plain text in production. In fact, we are extracting all the encryption logic in another branch since encryption won't be enabled in the initial release. However we've already started the process and it would be pretty time consuming to cherry pick all those changes. Can we show you the final result next week?

Yes, sure. Actually I'm not sure it makes sense to add encryption until we can integrate with one of the expected services or find an alternative solution. So for me it's fine if you keep it in a separate branch until everything is ready, if that works for you too.

bingen

This looks good to me now, thanks.

juslar and others added 30 commits September 28, 2018 18:00

Start encryption implementation

dca90da

Progress on encryption

db00bab

Working encryption/decryption

d55ebea

Fully functionnal encryption/decryption

1f1791e

Add encryption provider

43b8c61

Encryption/decryption function call bugs

b42128b

Complete encryption but contains bugs

31a23e2

Fully functionnal encryption/decryption

e91baa0

Update AragonOS and change package name

ab3ab1e

Update solidity to 0.4.24

45ae121

Add FileLibrary to dev branch

177e774

Implement file upload side panel

4ca9b54

Add contract-size script

a650988

Initial merge

8c12887

Add solc optimizer + bug fix

b2fb253

Add param to addFile in tests

449f0ae

Add setEncryptionKey

582bd64

Fix FileContentUpdate

72ef435

permanentlyDeleteFile

69d4612

Fix access to _self.files

12fd548

Fix tests

38a9a06

Add setIsDeleted

e9feb22

Fix tests

59a80e9

Undelete file

c890b72

listFiles with includeDeletedFiles param

392a900

Remove includeDeletedFiles

3f15189

deleteFilesPermanently

e81e3b2

deleteFilesPermanently

243a4bb

Merge with dev

fe5a629

Add _deletePermanently param to deleteFiles

3f07214

juslar and others added 11 commits November 12, 2018 14:34

Merge pull request #72 from espresso-org/gas-usage

7548aa4

Show gas usage when launching tests

Fix docs

d00f487

Merge branch 'dev' of https://github.com/espresso-org/aragon-datastore …

a2daa43

…into dev

Using espresso-org/object-acl

34aa87c

Using ObjectACL

0c2cde4

Remove local DatastoreACL references

acf12dc

Use @espresso-org/object-acl 0.0.2

9b00c00

Merge pull request #74 from espresso-org/datastore-acl-extract

8a2f2d3

Datastore acl extract

Fix cached-path-relative

af12180

Upgrade webpack to fix randomatic security alert

327f6b1

Merge pull request #75 from espresso-org/security-alerts

4875e68

Security alerts

bingen reviewed Dec 7, 2018

View reviewed changes

macor161 added 2 commits December 14, 2018 15:13

Sets the initialize function param type to ObjectACL

8f303b7

Prevent storage provider change once it is set

19ecbcb

bingen reviewed Jan 2, 2019

View reviewed changes

macor161 added 3 commits January 2, 2019 15:26

Cherry pick events from development branch

dd1ca0b

Fix tests

bd312ba

Remove unused Settings structs

2664a42

bingen reviewed Jan 3, 2019

View reviewed changes

leftab and others added 6 commits January 3, 2019 09:51

Cherry pick "Remove setEncryptionProvider" f971bf2

22891e0

Cherry pick d306db7

89cef4b

Change setSettings description

4a2415b

Fix setSettings

fb5e27d

Cherry pick PR #93

1b49ed9

addFile optimization

dc18839

macor161 commented Jan 4, 2019

View reviewed changes

Move FILE_READ_ROLE, FILE_WRITE_ROLE, DATASTORE_GROUP to libraries

7b3bec2

bingen approved these changes Jan 10, 2019

View reviewed changes

		@@ -25,13 +24,10 @@ library PermissionLibrary {
		bytes32 FILE_READ_ROLE;
		bytes32 FILE_WRITE_ROLE;

Milestone 2 #77

Are you sure you want to change the base?

Milestone 2 #77

Conversation

macor161 commented Nov 20, 2018 • edited Loading

coveralls commented Nov 20, 2018 • edited Loading

Pull Request Test Coverage Report for Build 116

💛 - Coveralls

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macor161 Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macor161 Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macor161 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen commented Jan 10, 2019

bingen left a comment

Choose a reason for hiding this comment

macor161 commented Nov 20, 2018 •

edited

Loading

coveralls commented Nov 20, 2018 •

edited

Loading

macor161 Dec 14, 2018 •

edited

Loading

macor161 Dec 14, 2018 •

edited

Loading