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

Feature/e2ee v2 foldersharing #6350

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

allexzander
Copy link
Contributor

No description provided.

@allexzander allexzander requested a review from mgallien January 15, 2024 09:20
@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch 2 times, most recently from 16ec906 to 03c3931 Compare January 15, 2024 12:47
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator

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

@allexzander
Copy link
Contributor Author

@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
Copy link
Collaborator

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

Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

early feedback
when testing against v28, I got this
image

src/libsync/CMakeLists.txt Outdated Show resolved Hide resolved
@allexzander
Copy link
Contributor Author

early feedback when testing against v28, I got this image

@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
Copy link
Collaborator

early feedback when testing against v28, I got this image

@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
Copy link
Contributor Author

early feedback when testing against v28, I got this image

@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
Copy link
Collaborator

mgallien commented Jan 16, 2024

early feedback when testing against v28, I got this image

@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

@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch from c388248 to 8661140 Compare January 17, 2024 10:08
QSharedPointer<FolderMetadata> _e2EeFolderMetadata;
QSharedPointer<FolderMetadata> _topLevelE2eeFolderMetadata;

QSet<QString> _listRootE2eeFolders;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 Outdated Show resolved Hide resolved
src/libsync/owncloudpropagator.h Outdated Show resolved Hide resolved
src/libsync/owncloudpropagator.h Outdated Show resolved Hide resolved
src/libsync/foldermetadata.h Show resolved Hide resolved
src/libsync/propagatedownload.h Outdated Show resolved Hide resolved
src/libsync/syncfileitem.h Outdated Show resolved Hide resolved
src/libsync/syncfileitem.h Show resolved Hide resolved
src/libsync/propagateremotemkdir.cpp Outdated Show resolved Hide resolved
src/libsync/vfs/cfapi/hydrationjob.h Outdated Show resolved Hide resolved
src/common/syncjournaldb.h Show resolved Hide resolved
@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch from 98da408 to 77d778b Compare January 19, 2024 10:22
@allexzander
Copy link
Contributor Author

early feedback when testing against v28, I got this image

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

@allexzander allexzander requested a review from mgallien January 19, 2024 11:58
@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch 3 times, most recently from 031195e to cc10753 Compare January 24, 2024 09:22
src/common/checksums.h Outdated Show resolved Hide resolved
src/common/syncjournaldb.h Show resolved Hide resolved
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question to clarify the meaning of folder in this context

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgallien folderId as returned from the PROPFIND LsColJob

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the id of _folder class attribute from line 239 ?

src/libsync/basepropagateremotedeleteencrypted.h Outdated Show resolved Hide resolved
src/libsync/foldermetadata.h Outdated Show resolved Hide resolved
src/libsync/foldermetadata.h Outdated Show resolved Hide resolved
src/libsync/foldermetadata.h Outdated Show resolved Hide resolved
src/libsync/syncfileitem.h Outdated Show resolved Hide resolved
src/libsync/syncfileitem.h Show resolved Hide resolved
@allexzander allexzander requested a review from mgallien January 29, 2024 01:51
@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch from db282e1 to db15833 Compare January 29, 2024 02:27
};

Q_ENUM_NS(ItemEncryptionStatus)

OCSYNC_EXPORT Q_NAMESPACE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplicate from line 53
please remove

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the id of _folder class attribute from line 239 ?

src/libsync/encryptedfoldermetadatahandler.h Outdated Show resolved Hide resolved
src/libsync/encryptedfoldermetadatahandler.h Outdated Show resolved Hide resolved
QByteArray cipherText;
QByteArray nonce;
QByteArray authenticationTag;
FileDropEntryUser currentUser;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. file file1 and user a
  2. file file1 and user b
  3. file file2 and user a
  4. file file1 and user b
  5. ...

Copy link
Contributor Author

@allexzander allexzander Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 Outdated Show resolved Hide resolved
src/libsync/rootencryptedfolderinfo.h Show resolved Hide resolved
src/libsync/updatee2eefolderusersmetadatajob.h Outdated Show resolved Hide resolved
@allexzander
Copy link
Contributor Author

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

@allexzander
Copy link
Contributor Author

@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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 53 to 57
explicit EncryptedFolderMetadataHandler(const AccountPtr &account,
const QString &folderPath,
SyncJournalDb *const journalDb,
const QString &pathForTopLevelFolder,
QObject *parent = nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is wrong

Comment on lines 45 to 52
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation is wrong

…c migration from 1.0 to 2.0(only for flat folders). Improved secure filedrop.

Signed-off-by: alex-z <[email protected]>
@allexzander allexzander force-pushed the feature/e2ee-v2-foldersharing branch from a80a664 to af61252 Compare January 29, 2024 15:13
@nextcloud-desktop-bot
Copy link

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.

@mgallien mgallien merged commit 8cf4dee into master Jan 29, 2024
6 of 10 checks passed
@mgallien mgallien deleted the feature/e2ee-v2-foldersharing branch January 29, 2024 15:36
Copy link

Quality Gate Failed 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

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

@mgallien mgallien added this to the 3.12.0 milestone Apr 24, 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 this pull request may close these issues.

3 participants