Change to re-create masterkeys by sharidas · Pull Request #6 · owncloud/encryption

sharidas · 2017-07-24T14:14:06Z

This change brings a new command to re-create
masterkey

Signed-off-by: Sujith H sharidasan@owncloud.com

PVince81

Good progress. There are still a few changes needed.

PVince81 · 2017-07-24T14:22:44Z

+	}
+
+	protected function execute(InputInterface $input, OutputInterface $output) {
+		if ($this->util->isMasterKeyEnabled()) {


I think we should add a warning here before starting and asking whether to proceed "Y/N". Please check how warnings are displayed for the other encryption commands. Also add a "-y" option to answer yes automatically.

Considering that this will decrypt a whole instance and might take days, we better inform the admin in advance before starting.

cc @tomneedham

PVince81 · 2017-07-24T14:23:18Z

+			$progress->finish();
+			$output->writeln("\nEncryption completed successfully\n");
+		} else {
+			$output->writeln("Master key is not enabled. Kindly enable it\n");


please remove the "Kindly" bit. The instance might be in "user key" mode

PVince81 · 2017-07-24T14:23:42Z

+	}
+
+	public function encryptAllUsersFiles(ProgressBar $progress) {
+		$userNo = 1;


start at zero ?

PVince81 · 2017-07-24T14:24:14Z

+		}
+	}
+
+	public function encryptUsersFiles($uid, ProgressBar $progress) {


is there any code from the occ encrypt-all command we could reuse ?

PVince81 · 2017-07-24T14:24:44Z

+	protected $appConfig;
+
+	/** @var IConfig  */
+	protected $IConfig;


local attributes must start with a lower case and be camel case.

PVince81 · 2017-07-24T14:25:37Z

+			}
+		}
+
+		if (empty($this->failed)) {


show error in case of failure ? an admin might want to ctrl+c when seeing this in real-time

PVince81 · 2017-07-24T14:26:15Z

+	protected $appConfig;
+
+	/** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
+	protected $IConfig;


see my comment on attribute casing above

sharidas · 2017-07-28T14:30:03Z

+			$this->keyManager, $this->util, $this->config,
+			$this->mailer, $this->l, $this->questionHelper,
+			$this->secureRandom);
+		$this->encryptAll->encryptAll($input, $output);


We call encryptAll here. Reusing old code.

Since we use encrypt all. Let me also see if we can reuse decrypt all.

My mistake, its better to stick on with the decrypt implemented here. The final output of a decryptall is again encrypted. Which is not the result we want.

can you add a comment to clarify this, in case future developers wonder about the same question ?

sharidas · 2017-07-31T12:31:40Z

+
+	protected function encryptAllUsers(InputInterface $input, OutputInterface $output) {
+		/*
+		 * We are reusing the encryptAll code but not the decryptAll. The reason being


Added doc which says why we are not using decryptAll and re-using encryptAll. This should help future devs understand why we used this approach.

PVince81

👍, assuming you tested it and it still works

sharidas · 2017-07-31T14:25:46Z

Testing done as follows:

Created user admin and created files.
Tried to modify the files.
Ran the command. Relogin as admin user and verified the files were readable.
Created another user user1 and did the same procedure on both admin and user1

Testing done with external storage:

Create a file and move to the external storage.
Now try to update the file
Run the command from the location where the file was initially created.
After relogin, the file was readable. Done this exercise multiple times.

CLAassistant · 2017-07-31T17:09:17Z

All committers have signed the CLA.

This change brings a new command to re-create masterkey Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2017-08-31T09:42:31Z

Closing this PR. We have #12 to track.

PVince81 reviewed Jul 24, 2017

View reviewed changes

PVince81 assigned sharidas Jul 24, 2017

PVince81 added this to the development milestone Jul 24, 2017

sharidas force-pushed the recreate-masterkey-task branch 2 times, most recently from a69526c to cc7bbcb Compare July 28, 2017 14:29

sharidas commented Jul 28, 2017

View reviewed changes

sharidas force-pushed the recreate-masterkey-task branch 2 times, most recently from a19ae76 to 5ebdd9e Compare July 31, 2017 12:30

sharidas commented Jul 31, 2017

View reviewed changes

PVince81 approved these changes Jul 31, 2017

View reviewed changes

sharidas mentioned this pull request Jul 31, 2017

Change to re-create masterkeys owncloud/core#28145

Closed

9 tasks

sharidas force-pushed the recreate-masterkey-task branch 2 times, most recently from afe97a5 to 71f182d Compare July 31, 2017 17:08

sharidas force-pushed the recreate-masterkey-task branch from 71f182d to 775a4d0 Compare July 31, 2017 17:11

sharidas force-pushed the recreate-masterkey-task branch 13 times, most recently from 3749e30 to fd11f85 Compare August 21, 2017 10:07

Change to re-create masterkeys

2b1eb3e

This change brings a new command to re-create masterkey Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the recreate-masterkey-task branch from fd11f85 to 2b1eb3e Compare August 21, 2017 12:14

sharidas closed this Aug 31, 2017

patrickjahns removed this from the QA milestone Aug 13, 2019

Uh oh!

Conversation

sharidas commented Jul 24, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharidas commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sharidas commented Jul 31, 2017 •

edited

Loading

CLAassistant commented Jul 31, 2017 •

edited

Loading