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.

Sign up for GitHub

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

Jump to bottom

Add an occ command to scan files for legacy file key in use and get rid of those #38080

Merged
merged 7 commits into from
May 15, 2023
Merged

Add an occ command to scan files for legacy file key in use and get rid of those #38080

merged 7 commits into from
May 15, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented May 4, 2023 •
edited
Loading

Summary

Add an occ command to scan files for legacy key format, and migrate those to the new format.
Only works if master key is enabled.

Checklist

@come-nc come-nc added this to the Nextcloud 27 milestone May 4, 2023
@come-nc come-nc self-assigned this May 4, 2023
@come-nc come-nc requested review from icewind1991, a team, ArtificialOwl and nfebe and removed request for a team May 4, 2023 14:55
@come-nc come-nc added enhancement 3. to review Waiting for reviews labels May 4, 2023
come-nc added 2 commits May 4, 2023 17:50
@come-nc
Fix fopen mode
146284f 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Log failures to delete legacy file key
c9c49bf 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Copy link
Contributor Author

come-nc commented May 4, 2023

@icewind1991
So, this is currently broken, because opening with mode r+ does not touch the header. See https://github.com/nextcloud/server/blob/master/lib/private/Files/Stream/Encryption.php#L287-L292
Same problem in https://github.com/nextcloud/server/blob/master/apps/encryption/lib/Crypto/Encryption.php#L221-L225
Is r+ mode supported at all by server side encryption?

github-advanced-security[bot]
github-advanced-security bot found potential problems May 4, 2023
View reviewed changes
apps/encryption/lib/Crypto/Encryption.php
@@ -309,7 +309,12 @@

$publicKeys = $this->keyManager->addSystemKeys($this->accessList, $publicKeys, $this->getOwner($path));
$shareKeys = $this->crypt->multiKeyEncrypt($this->fileKey, $publicKeys);
$this->keyManager->deleteLegacyFileKey($this->path);
if (!$this->keyManager->deleteLegacyFileKey($this->path)) {
$this->logger->warning(

Check notice

Code scanning / Psalm

DeprecatedMethod

The method OCP\ILogger::warning has been marked as deprecated
This was referenced May 9, 2023
Closed
Merged
@come-nc come-nc force-pushed the enh/add-occ-command-for-legacy-filekey branch from 1379731 to 0f867eb Compare May 9, 2023 10:00
@come-nc
Copy and move files to migrate them to the new key
725403c 
We have to rewrite the header, so the whole file needs to be rewritten,
 so we just use the same strategy as DecryptAll.

Signed-off-by: Côme Chilliet <[email protected]>
@come-nc come-nc force-pushed the enh/add-occ-command-for-legacy-filekey branch from 0f867eb to 725403c Compare May 9, 2023 10:03
@come-nc
Copy link
Contributor Author

come-nc commented May 9, 2023

So, I was not able to do this in a smart way, in the end the whole file has to be written again anyway since the header at the beginning needs to change. So I went for the same implementation as the DecryptAll class, I copy to a temporary name and move over the existing file.
But it works, after running the fileKeys are gone and the encrypted files can still be opened.

Ready for review.

@come-nc come-nc requested a review from artonge May 11, 2023 09:55
@come-nc come-nc force-pushed the enh/add-occ-command-for-legacy-filekey branch from 3884df7 to a92028f Compare May 11, 2023 09:56
@come-nc
Copy link
Contributor Author

come-nc commented May 11, 2023

Ok this is now working and not losing fileid anymore.
It will be slow sadly but I do not see how to avoid that.

Ready for review.

@come-nc
Copy link
Contributor Author

come-nc commented May 11, 2023

CI failures not related

@come-nc
Add fclose on opened resources
4910888 
Signed-off-by: Côme Chilliet <[email protected]>
github-advanced-security[bot]
github-advanced-security bot found potential problems May 11, 2023
View reviewed changes
apps/encryption/lib/Command/DropLegacyFileKey.php
$output->writeln('<error>Failed to migrate ' . $path . '</error>');
$output->writeln('<error>' . $e . '</error>', OutputInterface::VERBOSITY_VERBOSE);
} finally {
if (is_resource($copyResource)) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type resource for $copyResource is always resource
apps/encryption/lib/Command/DropLegacyFileKey.php
$output->writeln('<error>Failed to migrate ' . $path . '</error>');
$output->writeln('<error>' . $e . '</error>', OutputInterface::VERBOSITY_VERBOSE);
} finally {
if (is_resource($copyResource)) {

Check notice

Code scanning / Psalm

PossiblyUndefinedVariable

Possibly undefined variable $copyResource defined in try block
apps/encryption/lib/Command/DropLegacyFileKey.php
if (is_resource($copyResource)) {
fclose($copyResource);
}
if (is_resource($sourceResource)) {

Check notice

Code scanning / Psalm

RedundantConditionGivenDocblockType

Docblock-defined type resource for $sourceResource is always resource
apps/encryption/lib/Command/DropLegacyFileKey.php
if (is_resource($copyResource)) {
fclose($copyResource);
}
if (is_resource($sourceResource)) {

Check notice

Code scanning / Psalm

PossiblyUndefinedVariable

Possibly undefined variable $sourceResource defined in try block
@come-nc come-nc merged commit 7fd453e into master May 15, 2023
@come-nc come-nc deleted the enh/add-occ-command-for-legacy-filekey branch May 15, 2023 08:20
icewind1991
icewind1991 reviewed May 15, 2023
View reviewed changes
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

👍

@AkechiShiro
Copy link

Is there any plans to update the documentation on server side encryption or write some documentation on the migration process using this new occ command ?

If a follow up PR on the documentation of the migration process exists, could it be please linked here ?
I could also open such a PR but I'm afraid I'm not enough knowledgeable in Nextcloud and don't have at the moment a dev environment to test NC27 Beta 2.

@blizzz blizzz mentioned this pull request May 17, 2023
Merged
@joshtrichards joshtrichards mentioned this pull request Feb 13, 2024
Closed
@joshtrichards
Copy link
Member

@AkechiShiro Still needs someone to pick it up, but tracking doc addition in nextcloud/documentation#11539. Thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icewind1991 icewind1991 icewind1991 left review comments

@artonge artonge artonge approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe was automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Milestone
Nextcloud 27
Development

Successfully merging this pull request may close these issues.

Add an occ command to migrate all files to the new key format, when master key is used
5 participants
@come-nc @AkechiShiro @joshtrichards @icewind1991 @artonge