Downstream encryption:fix-encrypted-version for repairing "bad signature" errors

[PVince81]

For fixing "Bad signature" errors.

Imported from https://github.com/owncloud/encryption/blob/c8d67f5/lib/Command/FixEncryptedVersion.php (master from that time).
No changes were made in the initial import.

Tests

To test, upload a file and then manually change its "encrypted" value to either value - 1 or value + 1 then run occ encryption:fix-encrypted-version

User-key encryption

• recovering all files for user not supported
• recovering by path for user not supported

Master-key encryption

• recovering all files for user
• recovering by path for user

Other

• does it repair when "encryption_skip_signature_check" is set ? 🚫 see below
• group folders ??? => not supported

[juliusknorr]

group folders

Groupfolders currently don't support encryption nextcloud/groupfolders@4521451

[PVince81]

• BUG: does not fix the files when signature check is enabled, because the verification thinks the files are ok
  • solution 1: detect the option and ask the admin to disable it, or disable it temporarily.
    • pros: easy to implement, non-intrusive
    • cons: if admin disables it it might make the instance unusable for users for which bad signature errors existed until the repair happens => but maybe here we should advice to only run this command in single-user mode ??
  • solution 2: disable the option while the command runs
    • cons: dangerous as it will affect concurrent requests, like in solution 1
    • cons: messes up with config.php, might not work on clustered setups
  • solution 3: adjust the "skip signature" check code to also allow for another flag to be passed it to disable skipping only within the occ command run
    • pros: safest approach
    • cons: intrusive as it requires more modifications (could potentially make backporting more difficult)

Thoughts ?

[PVince81]

rebased as I forgot to pull before I made this branch

[PVince81]

I've now also downstreamed the matching unit tests file.

[PVince81]

for #27638 (comment) I went with approach 1: display an error if the skip flag is set. That should do for now.

[PVince81]

I'm trying to fix the unit tests, there seem to be a discrepancy of behavior, but could be due to test setup

[PVince81]

okay, I'm guessing that NC doesn't encrypt / doesn't increase the encryption version when a file is empty, so the test calls to touch() don't increase the version

[PVince81]

yeah, I suspected that:

OCA\Encryption\Tests\Command\FixEncryptedVersionTest::testEncryptedVersionLessThanOriginalValue
--


OCA\Encryption\Exceptions\MultiKeyDecryptException:
 multikeydecrypt with share key failed:error:0E06D06C:configuration file
 routines:NCONF_get_string:no value

the test setup is dirty and causing side effects

• redo test setup for encryption to avoid side effects with other tests (use more mocks?)
• php:cs fix

[PVince81]

the tests of the app run fine for me locally, so I suspect that there might be another test that runs before that doesn't clean up properly and is causing side effects. good luck finding which one 😢

[PVince81]

I've done some manual bisecting by deleting tests and running the whole DB suite locally: when deleting "apps/dav/tests" the encryption tests pass. So that folder likely contains a test that is causing side effects.
Digging deeper... 🕳️

[PVince81]

getting closer, it's in the "apps/dav/tests/unit/Connector/Sabre/RequestTest" folder. If I delete that folder the encryption test will pass.

aha, I see some encryption-related tests in there, so probably the setup there isn't cleaning up fully

yup, it's EncryptionMasterKeyUploadTest.php

[juliusknorr]

Tested and works and code looks good.

[juliusknorr]

Since the user key todos are still in the description, I'd say user key handling is out of scope for now since we'd need to provide the password of the user somehow. If we might need a recover option for that, we could probably maybe wrap the fixing logic into a small app to allow users to trigger it within their session as a kind of self service.

[PVince81]

Since the user key todos are still in the description, I'd say user key handling is out of scope for now since we'd need to provide the password of the user somehow. If we might need a recover option for that, we could probably maybe wrap the fixing logic into a small app to allow users to trigger it within their session as a kind of self service.

Indeed. IIRC user key decryption is not supported by the command anyway as it would require either the user's password or a recovery key to be configured for that user.

[juliusknorr]

I didn't see any check in that regard so i guess it would still try to find the matching encryptedVersion which would just not be successful then, but also haven't tested that.

[PVince81]

I didn't see any check in that regard so i guess it would still try to find the matching encryptedVersion which would just not be successful then, but also haven't tested that.

if this is a concern, it should be possible to add a check for master key for this command and abort in case none is found

all this is assuming that the "default encryption module" is being used

[PVince81]

@juliushaertl I've added an extra check d3eeecb

please re-review

[PVince81]

failing test unrelated

[PVince81]

/backport to stable22

[PVince81]

While this is an addition / feature, I wonder if we should backport this further down as it will be useful for fixing broken setups and would be safer than letting people patch their instances there.

[juliusknorr]

While this is an addition / feature, I wonder if we should backport this further down as it will be useful for fixing broken setups and would be safer than letting people patch their instances there.

I'd vote for that 👍 Any concers @skjnldsv ?

[PVince81]

force-merge needed due to intermitted acceptance test failure

[skjnldsv]

/backport to stable21

[skjnldsv]

/backport to stable20

[PVince81]

PR for the docs here: nextcloud/documentation#6884