Change to re-create masterkeys #28145
Conversation
apps/encryption/js/settings-admin.js
Outdated
| @@ -80,6 +82,20 @@ $(document).ready(function () { | |||
| } | |||
| }); | |||
|
|
|||
| $("#newMasterKey").click(function () { | |||
| console.log("I am clicked"); | |||
| } else { | ||
| debug_print_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS); |
|
@tomneedham Thanks for pointing it out. Removing them :) |
4136fd9 to
a022e0b
Compare
There was a problem hiding this comment.
If this is running within a single ajax request then this is a no go. Decrypting and reencrypting all files within a PHP request is very likely to run into PHP timeouts.
Also, if I understand correctly, this would decrypt all files on disk first then reencrypt. It would be better (but also much more complex to implement here) to decrypt with old key and reencrypt on the fly with new key, to avoid having the files exposed for a short period of time.
| } | ||
|
|
||
| public function createNewMasterKey() { | ||
| \OC::$server->getLogger()->warning(__METHOD__." so time to create new key!!!", ['app' => __CLASS__]); |
There was a problem hiding this comment.
use a better message or remove ?
| if (empty($this->failed)) { | ||
| //Now recreate new encryption | ||
| //Delete the encryption app | ||
| \OC::$server->getAppConfig()->deleteApp('encryption'); |
There was a problem hiding this comment.
this looks a bit hacky. Can we just delete the relevant keys instead of destroying everything and reenabling ?
| //Delete the files_encryption dir | ||
| $this->rootView->deleteAll('files_encryption'); | ||
| \OC::$server->getConfig()->deleteAppValue('files_encryption','installed_version'); | ||
| \OC::$server->getConfig()->deleteAppValues('encryption'); |
There was a problem hiding this comment.
what's the difference between deleteApp above and deleteAppValues ? Doesn't this do the same ?
| \OC::$server->getConfig()->deleteAppValues('encryption'); | ||
|
|
||
| //Re-enable the encryption app | ||
| \OC_App::enable('encryption'); |
There was a problem hiding this comment.
use app manager if possible
| \OC::$server->getAppConfig()->deleteApp('encryption'); | ||
| //Delete the files_encryption dir | ||
| $this->rootView->deleteAll('files_encryption'); | ||
| \OC::$server->getConfig()->deleteAppValue('files_encryption','installed_version'); |
There was a problem hiding this comment.
receive IConfig injected in the constructor and use it instead of getConfig().
| do { | ||
| $users = $backend->getUsers('', $limit, $offset); | ||
| foreach ($users as $user) { | ||
| echo "\n Entering user $user\n"; |
apps/encryption/js/settings-admin.js
Outdated
|
|
||
| $("#reEncryptFS").click(function () { | ||
| $.post( | ||
| OC.generateUrl('/apps/encryption/ajax/reencryptFiles') |
There was a problem hiding this comment.
this will very likely run into PHP timeouts... would recommend only providing OCC command for this
| @@ -99,6 +99,18 @@ public function decryptAll(InputInterface $input, OutputInterface $output, $user | |||
|
|
|||
| if (empty($this->failed)) { | |||
| $this->output->writeln('all files could be decrypted successfully!'); | |||
| //Now recreate new encryption | |||
| //Delete the encryption app | |||
| \OC::$server->getAppConfig()->deleteApp('encryption'); | |||
There was a problem hiding this comment.
dejavu ? I've seen this code before somewhere above
a022e0b to
04c958a
Compare
| @@ -672,6 +685,9 @@ private function updateEncryptedVersion(Storage $sourceStorage, $sourceInternalP | |||
| // in case of a rename we need to manipulate the source cache because | |||
| // this information will be kept for the new target | |||
| if ($isRename) { | |||
| $encryptedVersion = 1; | |||
There was a problem hiding this comment.
This change comes from #28107. The rename operation is happening at
There was a problem hiding this comment.
I will remove this change now.
| $progress = new ProgressBar($output); | ||
| $progress->start(); | ||
| while(!$this->appConfig->hasKey('encryption', 'publicShareKeyId')) { | ||
| sleep(3); |
There was a problem hiding this comment.
I don't like this. Any better suggestion would be nice. It takes some time to regenerate new master keys.
There was a problem hiding this comment.
how is that even possible ? PHP is sequential, what is it waiting for ?
AFAIK all of the openssl commands are synchronous
There was a problem hiding this comment.
It's waiting for the new master keys. The code execution happens sequentially.
There was a problem hiding this comment.
In this case sleep is useless because it will block the PHP process.
If adding it solves the problem then it means there is some background / async process running somewhere, and that's not good because it means potential race conditions in other parts of the code.
There was a problem hiding this comment.
and reading the code above I don't see any code that would spawn a separate process ? which line is starting the process that generates the new master key ?
04c958a to
cc8ec6e
Compare
|
Removed the ugly while loop which waited to create the masterkey. Instead in the keymanager I created the function which creates the masterkey and publicsharekey. And reused the same in the constructor. |
0790c1d to
258c10e
Compare
258c10e to
aa2fadd
Compare
|
@sharidas failling unit tests, please fix |
eacf990 to
b2e0389
Compare
acf7f57 to
8d5a5d2
Compare
|
Need review for this change too... This is the followup of owncloud/encryption#6 |
| @@ -201,16 +201,19 @@ public function getMetaData($path) { | |||
| */ | |||
| public function file_get_contents($path) { | |||
|
|
|||
| $encryptionModule = $this->getEncryptionModule($path); | |||
| //if ( \OC::$server->getAppConfig()->getValue('encryption', 'enabled', false) !== false) { | |||
| @@ -421,6 +425,9 @@ public function fopen($path, $mode, $sourceFileOfRename = null) { | |||
| $encryptionModule = $this->encryptionManager->getEncryptionModule($encryptionModuleId); | |||
| $shouldEncrypt = $encryptionModule->shouldEncrypt($fullPath); | |||
| $signed = true; | |||
| if (\OC::$server->getSession()->get('decryptAllCmd') === true) { | |||
There was a problem hiding this comment.
arghh, no, that's too hacky
6919f9a to
d40404a
Compare
|
|
|
@PVince81 , Modified changes to some of the core API's. Removed the session hack from the code. |
d40404a to
f3c0144
Compare
This change supports a new command to re-create masterkey Signed-off-by: Sujith H <sharidasan@owncloud.com>
f3c0144 to
616e6dd
Compare
| * @return resource|bool | ||
| * @throws GenericEncryptionException | ||
| * @throws ModuleDoesNotExistsException | ||
| */ | ||
| public function fopen($path, $mode, $sourceFileOfRename = null) { | ||
| public function fopen($path, $mode, $sourceFileOfRename = null, $getDecryptedFile = false) { |
There was a problem hiding this comment.
This feels wrong and unintuitive.
Also I don't understand: the encryption wrapper always gives access to decrypted data. fread/fwrite operations are using decrypted data. Maybe what you meant here is that you want to access the encrypted binary data directly by skipping this wrapper.
So the key here is likely to find a way to skip/disable the encryption wrapper to get access to the file. Not sure how complicated this could be.
|
Obsolete PR. The final PR: #28774 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This change will help users to recreate masterkey
for encryption.
Signed-off-by: Sujith H sharidasan@owncloud.com
Description
This change will help users to regenerate masterkeys for encrypting data
Related Issue
https://github.com/owncloud/enterprise/issues/2033
#21434
Motivation and Context
This change will help users to regenerate masterkey. In current state masterkey once enabled cannot be changed. This change addresses the issue.
How Has This Been Tested?
Using occ command created users admin and user1. Enabled encryption app and then tried to enable masterkey. Now open the UI and try to login to both admin and user1. Verify the welcome.txt file is encrypted using cat command. As admin user try to upload/create new file, let's say hello.txt. Then update the file multiple times. And then run the command ./occ encryption:recreate-master-key from the command line. Relogin as admin user to view the files. Re-run the encryption:recreate-master-key command multiple times. And then relogin as admin user. The admin user was able to view the files.
cat the welcome.txt, hello.txt ( or any other files uploaded or created) of both admin and user1. The files are encrypted.
Screenshots (if appropriate):
Types of changes
Checklist: