Feature/e2ee v2 foldersharing #6350

allexzander · 2024-01-15T09:20:19Z

No description provided.

mgallien

@allexzander compilation on Linux is broken as far as I can say
see

/home/mgallien/work/nextcloud/desktop/test/testclientsideencryptionv2.cpp: In member function ‘void TestClientSideEncryptionV2::testInitializeNewRootFolderMetadataThenEncryptAndDecrypt()’:
/home/mgallien/work/nextcloud/desktop/test/testclientsideencryptionv2.cpp:144:86: error: ‘QByteArray OCC::FolderMetadata::decryptDataWithPrivateKey(const QByteArray&) const’ is private within this context
  144 |                 const auto decryptedMetadataKey = metadata->decryptDataWithPrivateKey(encryptedMetadataKey);
      |                                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

mgallien · 2024-01-15T15:51:26Z

@allexzander what do you need on server to be able to test ?

allexzander · 2024-01-15T19:49:41Z

@allexzander what do you need on server to be able to test ?

I need server to support the following:

I lock the top encrypted folder
I use that lock token to update the metadata of the top encrypted folder
I used the same token to update the nested folders' metadata

It worked before, but now I again have an issue, while updating the metadata of nested folders using the token from the top folder which is locked seems to succeed, in reality, the metadata for each nested folder remains old, hence, next time I try to decrypt them using already new metadatakey - it fails. Louis told me it works but it seems to be not the case. Going to need to find some time and create logs for him to see the issue. He thinks it is the issue in the desktop client's code, but the code executes properly, last time I debugged it (last week) it did behave that way, but I noticed the metadata returned from subfolders is still the one that was before I uploaded the new one, while server replies with 200.

mgallien · 2024-01-16T09:34:00Z

@alex can you look at the build failures in the docker CI check ?
there are many compilation warnings and they should be removed

mgallien

early feedback
when testing against v28, I got this

src/libsync/CMakeLists.txt

allexzander · 2024-01-16T10:28:40Z

early feedback when testing against v28, I got this

@mgallien Kindly, create a new user for tests. Existing users have all kinds of different metadata flavours that was used during the development and debugging.

mgallien · 2024-01-16T10:55:00Z

early feedback when testing against v28, I got this

@mgallien Kindly, create a new user for tests. Existing users have all kinds of different metadata flavours that was used during the development and debugging.

this is being done with a fresh user and fresh server running v28 (end-to-end encryption v1.2)

allexzander · 2024-01-16T11:29:51Z

early feedback when testing against v28, I got this

@mgallien Kindly, create a new user for tests. Existing users have all kinds of different metadata flavours that was used during the development and debugging.

this is being done with a fresh user and fresh server running v28 (end-to-end encryption v1.2)

@mgallien Must be V2 https://try.nextcloud.com/ltd/e2e2 not e2e but e2e2

mgallien · 2024-01-16T16:54:40Z

early feedback when testing against v28, I got this

@mgallien Kindly, create a new user for tests. Existing users have all kinds of different metadata flavours that was used during the development and debugging.

this is being done with a fresh user and fresh server running v28 (end-to-end encryption v1.2)

@mgallien Must be V2 https://try.nextcloud.com/ltd/e2e2 not e2e but e2e2

@allexzander the issue is that if someone is not running the very last release of server and end-to-end encryption app, they will be able to open the share dialog and get to see the error in my screenshot
keep in mind that we should be compatible with server releases as old as v16.0.0

mgallien · 2024-01-17T17:56:31Z

src/libsync/discoveryphase.h

+    QSharedPointer<FolderMetadata> _e2EeFolderMetadata;
+    QSharedPointer<FolderMetadata> _topLevelE2eeFolderMetadata;
+
+    QSet<QString> _listRootE2eeFolders;


I do not get why you need this here
are you doing discovery action in many root e2ee folder from a single one ?
I do not get why it cannot be told what its root encrypted folder is

@mgallien I have removed some leftovers, and now only _topLevelE2eeFolderPaths is left, it is required as we need to fetch the top-level folder's metadata from inside the nested folder's metadata, as the data for decryption is only stored in top-level folders as per E2EE V2. See comments, and check comments I've added

src/libsync/discoveryphase.h

src/libsync/owncloudpropagator.h

src/libsync/foldermetadata.h

src/libsync/propagatedownload.h

src/libsync/syncfileitem.h

src/libsync/propagateremotemkdir.cpp

src/libsync/vfs/cfapi/hydrationjob.h

src/common/syncjournaldb.h

allexzander · 2024-01-19T11:57:58Z

early feedback when testing against v28, I got this

@mgallien Kindly, create a new user for tests. Existing users have all kinds of different metadata flavours that was used during the development and debugging.

this is being done with a fresh user and fresh server running v28 (end-to-end encryption v1.2)

@mgallien Must be V2 https://try.nextcloud.com/ltd/e2e2 not e2e but e2e2

@allexzander the issue is that if someone is not running the very last release of server and end-to-end encryption app, they will be able to open the share dialog and get to see the error in my screenshot keep in mind that we should be compatible with server releases as old as v16.0.0

@mgallien Fixed this. Now it is impossible to share if opened on a folder that does not support sharing (version is less then 2.0)

src/common/checksums.h

src/common/syncjournaldb.h

mgallien · 2024-01-26T11:14:38Z

src/gui/filedetails/sharemodel.h

@@ -33,6 +33,7 @@ class ShareModel : public QAbstractListModel
    Q_PROPERTY(bool publicLinkSharesEnabled READ publicLinkSharesEnabled NOTIFY publicLinkSharesEnabledChanged)
    Q_PROPERTY(bool userGroupSharingEnabled READ userGroupSharingEnabled NOTIFY userGroupSharingEnabledChanged)
    Q_PROPERTY(bool canShare READ canShare NOTIFY sharePermissionsChanged)
+    Q_PROPERTY(bool isShareDisabledFolder READ isShareDisabledFolder NOTIFY isShareDisabledFolderChanged)


I am not sure what is the meaning of folder in this context
is it the synchronization folder or a specific folder inside a synchronization folder ?
can you try clarifying this ?

@mgallien specific folder inside a synchronization folder, for example, we only allow top-level encrypted folders to be shared with all the contents inside them, but not for nested folders e.g.: "e2ee-folder-toplevel/nested", we only allow sharing when interacting with "e2ee-folder-toplevel" but not with "nested"

so this is a duplicate from canShare or what is the extra need to have to check canShare and isShareDisabledFolder ?
I still would be unable to understand what isShareDisabledFolder means without having to read the review on the PR again
if this is really needed, please try to find a name that is easier to understand ?

mgallien · 2024-01-26T11:15:12Z

src/gui/filedetails/sharemodel.h

@@ -240,6 +245,7 @@ private slots:
    SharedItemType _sharedItemType = SharedItemType::SharedItemTypeUndefined;
    SyncJournalFileLockInfo _filelockState;
    QString _privateLinkUrl;
+    QByteArray _folderId;


same question to clarify the meaning of folder in this context

@mgallien folderId as returned from the PROPFIND LsColJob

so the id of _folder class attribute from line 239 ?

src/libsync/basepropagateremotedeleteencrypted.h

src/libsync/foldermetadata.h

src/libsync/syncfileitem.h

mgallien · 2024-01-29T09:56:03Z

src/csync/csync.h

 };

 Q_ENUM_NS(ItemEncryptionStatus)

+OCSYNC_EXPORT Q_NAMESPACE


This is a duplicate from line 53
please remove

mgallien · 2024-01-29T09:58:57Z

src/gui/filedetails/sharemodel.h

@@ -33,6 +33,7 @@ class ShareModel : public QAbstractListModel
    Q_PROPERTY(bool publicLinkSharesEnabled READ publicLinkSharesEnabled NOTIFY publicLinkSharesEnabledChanged)
    Q_PROPERTY(bool userGroupSharingEnabled READ userGroupSharingEnabled NOTIFY userGroupSharingEnabledChanged)
    Q_PROPERTY(bool canShare READ canShare NOTIFY sharePermissionsChanged)
+    Q_PROPERTY(bool isShareDisabledFolder READ isShareDisabledFolder NOTIFY isShareDisabledFolderChanged)


so this is a duplicate from canShare or what is the extra need to have to check canShare and isShareDisabledFolder ?
I still would be unable to understand what isShareDisabledFolder means without having to read the review on the PR again
if this is really needed, please try to find a name that is easier to understand ?

mgallien · 2024-01-29T10:01:20Z

src/gui/filedetails/sharemodel.h

@@ -240,6 +245,7 @@ private slots:
    SharedItemType _sharedItemType = SharedItemType::SharedItemTypeUndefined;
    SyncJournalFileLockInfo _filelockState;
    QString _privateLinkUrl;
+    QByteArray _folderId;


so the id of _folder class attribute from line 239 ?

src/libsync/encryptedfoldermetadatahandler.h

mgallien · 2024-01-29T10:15:19Z

src/libsync/foldermetadata.h

+        QByteArray cipherText;
+        QByteArray nonce;
+        QByteArray authenticationTag;
+        FileDropEntryUser currentUser;


then, I am confused
do we get a FileDropEntry class instance for every file and every user having access ?
I mean if user a and b have access to all files inside a file drop folder, we would get an instance for:

file file1 and user a

file file1 and user b

file file2 and user a

file file1 and user b

...

@mgallien, yes, there is file-entry for each file and each file-entry contains an array of users that have access to that file, see RFC: https://github.com/nextcloud/end_to_end_encryption_rfc/blob/v2.1/RFC.md#create-metadata-file
but I only store current user (the one logged in and associated with E2EE from the array of users, if present for current user)

"filedrop": { "<uid>": { "ciphertext": "encrypted metadata (AES/GCM/NoPadding, 128 bit key size) of folder (see below for the plaintext structure). first gzipped, then encrypted, then base64 encoded.", "nonce": "123", "authenticationTag": "123", "users": [ // The following contains the reference to all users who have access to filedrop. // The metadata-key is encrypted with RSA/ECB/OAEPWithSHA-256AndMGF1Padding { "userId": "testUser" "encryptedFiledropKey": "encrypted filedrop-key then base64", } ], }, <uid>": { "ciphertext": "enc rypted metadata (AES/GCM/NoPadding, 128 bit key size) of folder (see below for the plaintext structure). first gzipped, then encrypted, then base64 encoded.", "nonce": "123", "authenticationTag": "123", "users": [...

src/libsync/rootencryptedfolderinfo.h

src/libsync/updatee2eefolderusersmetadatajob.h

allexzander · 2024-01-29T13:31:27Z

#6350 (comment) -> no it's a http://owncloud.org/ns:fileid, I renamed folder and folderid for clarity

allexzander · 2024-01-29T13:38:11Z

@mgallien #6350 (comment) -> No, those are 2 different things, the newly added property concerns encrypted folders and special conditions to allow/forbid sharing for those, while canShare is for share permissions for all folders, I do not think we should combine them, I renamed the new property now to make it more clear

mgallien · 2024-01-29T14:50:50Z

src/libsync/discoveryphase.h

-    explicit DiscoverySingleDirectoryJob(const AccountPtr &account, const QString &path, QObject *parent = nullptr);
+    explicit DiscoverySingleDirectoryJob(const AccountPtr &account,
+                                         const QString &path,
+                                         const QSet<QString> &topLevelE2eeFolderPaths,


I still disagree with this
even if no data are copied, the lookup of the info relevant for a single folder hierarchy should happen when starting the job doing the discovery on the single top encrypted folder, not each time we discover a folder inside
so this should eventually be changed

mgallien · 2024-01-29T14:52:06Z

src/libsync/encryptedfoldermetadatahandler.h

+    explicit EncryptedFolderMetadataHandler(const AccountPtr &account,
+                                                 const QString &folderPath,
+                                                 SyncJournalDb *const journalDb,
+                                                 const QString &pathForTopLevelFolder,
+                                                 QObject *parent = nullptr);


indentation is wrong

mgallien · 2024-01-29T14:54:07Z

src/libsync/updatee2eefolderusersmetadatajob.h

+    explicit UpdateE2eeFolderUsersMetadataJob(const AccountPtr &account,
+                                        SyncJournalDb *journalDb,
+                                        const QString &syncFolderRemotePath,
+                                        const Operation operation,
+                                        const QString &path = {},
+                                        const QString &folderUserId = {},
+                                        const QSslCertificate &certificate = QSslCertificate{},
+                                        QObject *parent = nullptr);


indentation is wrong

src/libsync/updatee2eefolderusersmetadatajob.h

…c migration from 1.0 to 2.0(only for flat folders). Improved secure filedrop. Signed-off-by: alex-z <[email protected]>

nextcloud-desktop-bot · 2024-01-29T15:27:02Z

AppImage file: nextcloud-PR-6350-af612525c411fa57988af88bb28d58281f3a2214-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

sonarcloud · 2024-01-29T15:38:53Z

Quality Gate failed

Failed conditions

1 Security Hotspot
29.2% Coverage on New Code (required ≥ 80%)
297 New Code Smells (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

allexzander requested a review from mgallien January 15, 2024 09:20

allexzander force-pushed the feature/e2ee-v2-foldersharing branch 2 times, most recently from 16ec906 to 03c3931 Compare January 15, 2024 12:47

mgallien requested changes Jan 15, 2024

View reviewed changes

mgallien requested changes Jan 16, 2024

View reviewed changes

src/libsync/CMakeLists.txt Outdated Show resolved Hide resolved

allexzander force-pushed the feature/e2ee-v2-foldersharing branch from c388248 to 8661140 Compare January 17, 2024 10:08

mgallien reviewed Jan 17, 2024

View reviewed changes

src/libsync/discoveryphase.h Outdated Show resolved Hide resolved

mgallien requested changes Jan 17, 2024

View reviewed changes

allexzander force-pushed the feature/e2ee-v2-foldersharing branch from 98da408 to 77d778b Compare January 19, 2024 10:22

allexzander requested a review from mgallien January 19, 2024 11:58

allexzander force-pushed the feature/e2ee-v2-foldersharing branch 3 times, most recently from 031195e to cc10753 Compare January 24, 2024 09:22

mgallien requested changes Jan 26, 2024

View reviewed changes

allexzander requested a review from mgallien January 29, 2024 01:51

allexzander force-pushed the feature/e2ee-v2-foldersharing branch from db282e1 to db15833 Compare January 29, 2024 02:27

mgallien requested changes Jan 29, 2024

View reviewed changes

End-to-End Encryption V2. Implemented sharing between users. Automati…

af61252

…c migration from 1.0 to 2.0(only for flat folders). Improved secure filedrop. Signed-off-by: alex-z <[email protected]>

allexzander force-pushed the feature/e2ee-v2-foldersharing branch from a80a664 to af61252 Compare January 29, 2024 15:13

mgallien approved these changes Jan 29, 2024

View reviewed changes

mgallien merged commit 8cf4dee into master Jan 29, 2024
6 of 10 checks passed

mgallien deleted the feature/e2ee-v2-foldersharing branch January 29, 2024 15:36

mgallien added this to the 3.12.0 milestone Apr 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/e2ee v2 foldersharing #6350

Feature/e2ee v2 foldersharing #6350

allexzander commented Jan 15, 2024

mgallien left a comment

mgallien commented Jan 15, 2024

allexzander commented Jan 15, 2024

mgallien commented Jan 16, 2024

mgallien left a comment

allexzander commented Jan 16, 2024

mgallien commented Jan 16, 2024

allexzander commented Jan 16, 2024

mgallien commented Jan 16, 2024 •

edited

Loading

mgallien Jan 17, 2024

allexzander Jan 19, 2024

allexzander commented Jan 19, 2024

mgallien Jan 26, 2024

allexzander Jan 26, 2024

mgallien Jan 29, 2024

mgallien Jan 26, 2024

allexzander Jan 26, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

allexzander Jan 29, 2024 •

edited

Loading

allexzander commented Jan 29, 2024

allexzander commented Jan 29, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

mgallien Jan 29, 2024

nextcloud-desktop-bot commented Jan 29, 2024

sonarcloud bot commented Jan 29, 2024

Feature/e2ee v2 foldersharing #6350

Feature/e2ee v2 foldersharing #6350

Conversation

allexzander commented Jan 15, 2024

mgallien left a comment

Choose a reason for hiding this comment

mgallien commented Jan 15, 2024

allexzander commented Jan 15, 2024

mgallien commented Jan 16, 2024

mgallien left a comment

Choose a reason for hiding this comment

allexzander commented Jan 16, 2024

mgallien commented Jan 16, 2024

allexzander commented Jan 16, 2024

mgallien commented Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allexzander commented Jan 19, 2024

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

allexzander Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

allexzander commented Jan 29, 2024

allexzander commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Jan 29, 2024

sonarcloud bot commented Jan 29, 2024

Quality Gate failed

mgallien commented Jan 16, 2024 •

edited

Loading

allexzander Jan 29, 2024 •

edited

Loading