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

fix: Autodetect legacy filekey instead of trusting the header for legacy header #45669

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jun 4, 2024

Summary

When resharing, the filekey gets migrated to new encryption, but the header is untouched so it fails to open.
With this change we just autodetect which kind of filekey to use when opening unless we know for sure we do not need to use the old way.

Also refactored the decryptAll related code to use the same getFileKey method as the rest.
Should be tested.

Checklist

…acy header

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc requested a review from artonge June 4, 2024 15:24
@come-nc come-nc self-assigned this Jun 4, 2024
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

I don't see the part where you autodetect the fileKey

apps/encryption/lib/KeyManager.php Show resolved Hide resolved
apps/encryption/lib/Crypto/Encryption.php Show resolved Hide resolved
@m-vz
Copy link

m-vz commented Jun 5, 2024

We just tried this patch in a test environment and it seems to give access to the files that were broken before. (At least the ones in a moved folder, we haven't tested the files broken by resharing)

@come-nc
Copy link
Contributor Author

come-nc commented Jun 6, 2024

I don't see the part where you autodetect the fileKey

Passing null to getFileKey for $useLegacyFileKey means auto-detection, while before we were passing true.

@artonge
Copy link
Contributor

artonge commented Jun 6, 2024

  • Test decrypt all

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

All good after testing decrypt all

@artonge
Copy link
Contributor

artonge commented Jun 6, 2024

Pushed commit to attempt to fix tests

@artonge artonge force-pushed the fix/fix-encryption-legacy-reshare branch from 5e297ff to 01e3921 Compare June 6, 2024 13:34
Signed-off-by: Louis Chemineau <louis@chmn.me>
@artonge artonge force-pushed the fix/fix-encryption-legacy-reshare branch from 01e3921 to f244261 Compare June 6, 2024 16:09
@ChristophWurst
Copy link
Member

/backport to stable29

@ChristophWurst
Copy link
Member

/backport to stable28

@ChristophWurst
Copy link
Member

/backport to stable27

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good but didn't test

@artonge artonge merged commit bab9125 into master Jun 11, 2024
155 checks passed
@artonge artonge deleted the fix/fix-encryption-legacy-reshare branch June 11, 2024 07:54
@AndyScherzinger AndyScherzinger added this to the Nextcloud 30 milestone Jun 11, 2024
@artonge
Copy link
Contributor

artonge commented Jun 11, 2024

/backport to stable27

@artonge
Copy link
Contributor

artonge commented Jun 11, 2024

/backport to stable28

@artonge
Copy link
Contributor

artonge commented Jun 11, 2024

/backport to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Multikey Encryption Breaks When Upgrading from NC26 to NC27
5 participants