extend fix-key-location to handle cases from broken cross-storage moves #36068
Conversation
icewind1991
requested review from
PVince81,
a team,
ArtificialOwl and
blizzz
and removed request for
a team
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Psalm found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
5ed04ea
to
0e6110e
Compare
| $systemKeyPath = $this->getSystemKeyPath($node); | ||
|
|
||
| if ($this->rootView->file_exists($systemKeyPath)) { | ||
| // already has a key, reject new key |
There was a problem hiding this comment.
alternatively we could check if the mtime of the new key is higher than the existing one and then overwrite.
and don't overwrite is mtime is lower
a higher mtime would mean that the file has been overwritten by another user despite being not decryptable (not sure if possible though)
There was a problem hiding this comment.
As is, this code path shouldn't be reached if a system key already exists, the check is mostly there as an extra safety rail.
| $this->rootView->copy($key, $systemKeyPath); | ||
|
|
||
| try { | ||
| if (!$storage->instanceOfStorage(Encryption::class)) { |
There was a problem hiding this comment.
does it mean this command works even with encryption app disabled ? maybe add a comment here to answer
There was a problem hiding this comment.
The command itself is part of the encryption app, so no
|
might be useful to add more debug logging / verbose output for each step in this complex algorithm, to be able to trace back repair problems and/or cases we haven't found yet then we can ask people to run with -vvvv also wondering if we should add a ctrl+c trap that only allow shutdown between two files, to avoid having a mess if an admin abort while we test keys combinations (where we put it in a place then delete again if it doesn't work). |
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
0e6110e
to
bb3262c
Compare
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
bb3262c
to
db06b6c
Compare
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
db06b6c
to
e890d1d
Compare
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
e890d1d
to
fc6df38
Compare
Signed-off-by: Robin Appelman <[email protected]>
icewind1991
force-pushed
the
gf-encryption-fix
branch
from
fc6df38
to
e4f8522
Compare
Signed-off-by: Robin Appelman <[email protected]>
Fixes files not being decrypted when moved cross-storage due to (#35894) (ab)using the fact that the encrypted flag wasn't cleared for them (#35961) to find these cases.
If a working key can be found for the files they are decrypted in place, leaving the encrypted file as a backup to reduce chances of errors leading to further data loss.
This currently only works if the file hasn't been renamed after being moved because the file name is being used to try and find possible encryption keys. A possible further step would be to add an option to check every available key, covering more cases at the cost of performance.
A special case was also added for when encryption is enabled for the storage after encrypted files have been moved. In those cases the found key is copied to the correct location.
The "fix-key-location" name of the command isn't fully applicable anymore so it might make sense to give it a new name.
To test:
occ encryption:fix-key-location <user>Alternatively add step 3.5
occ config:app:set groupfolders enable_encryption --value trueto enable groupfolder encryptionBackport note: this currently reuses the normal encryption logic to determine which mountpoints are "system" mountpoints, previous nc versions didn't handle groupfolders for that properly. So some extra logic for that will probsbly need to be added for backports.