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.

Sign up for GitHub

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

Jump to bottom

Change to re-create masterkeys #12

Merged
merged 1 commit into from
Oct 16, 2017
Merged

Change to re-create masterkeys #12

merged 1 commit into from
Oct 16, 2017

Conversation

sharidas
Copy link
Contributor

This change brings a new command to re-create
masterkey

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

@sharidas sharidas self-assigned this Aug 23, 2017
@sharidas sharidas added this to the development milestone Aug 23, 2017
@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch 2 times, most recently from c7c0efa to 73870af Compare August 24, 2017 09:26
@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch from 73870af to cd1a215 Compare August 30, 2017 14:48
@PVince81
Copy link
Contributor

moving to planned. Let's focus on fixing the core issues first.

@PVince81 PVince81 modified the milestones: planned, development Aug 31, 2017
This was referenced Aug 31, 2017
Closed
Closed
@sharidas sharidas mentioned this pull request Sep 18, 2017
Closed
PVince81
PVince81 reviewed Sep 18, 2017
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

please refresh my memory: how much code from decrypt-all and encrypt-all has been reused/copied ?

If a lot, is there a way to internally invoke the decrypt-all and encrypt-all command ? before and after regenerating the master key ?

lib/Command/RecreateMasterKey.php Outdated
$yes = $input->getOption('yes');
if ($this->util->isMasterKeyEnabled()) {
$question = new ConfirmationQuestion(
'Warning: Inorder to re-create master key, the entire ownCloud filesystem will be decrypted and then encrypted using new master key.'
Copy link
Contributor

Choose a reason for hiding this comment

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

"In order"

Copy link
Contributor

Choose a reason for hiding this comment

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

not the entire filesystem but: "all the user's home storages will be decrypted".

Will external storages be affected ? I'm not sure

Copy link
Contributor Author

@sharidas sharidas Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Verified with external storages:

  • SFTP
  • ownCloud ( without encryption when added and later running the recreate master key script )
  • ownCloud ( with maseter key encryption added and later ran the recreate master key script )
  • ownlCloud ( with user key encryption added and later ran the recreate master key script )

lib/Command/RecreateMasterKey.php Outdated
$output->writeln("Decryption started\n");
$progress = new ProgressBar($output);
$progress->start();
$progress->setMessage("Decryption progress...");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Decryption in progress..."

lib/Command/RecreateMasterKey.php Outdated
$this->IAppManager->disableApp('encryption');

//Delete the files_encryption dir
$this->rootView->deleteAll('files_encryption');
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct ? Remember that encryption keys can be stored in a different location with an occ storage (key storage root)

lib/Command/RecreateMasterKey.php Outdated
$this->appConfig->setValue('encryption', 'enabled', 'yes');
$this->appConfig->setValue('encryption', 'useMasterKey', '1');

$this->keyManager->validateShareKey();
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if this fails ? will it throw an exception, should we catch it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would try to create keys if it doesn't exist. If key exist nothing will be done. Same is for validateMasterKey

lib/Command/RecreateMasterKey.php Outdated
$target = $path . '.decrypted.' . $this->getTimestamp();

try {
\OC\Files\Storage\Wrapper\Encryption::setDisableWriteEncryption(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

are we also using this in the occ decrypt:all command ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't this technique of calling setDisableWriteEncryption at \OC\Encryption\DecryptAll::decryptAll()

@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch 3 times, most recently from 765ab35 to 9ca5039 Compare September 22, 2017 12:40
@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch 2 times, most recently from 6d61606 to 9dce123 Compare October 9, 2017 06:36
PVince81
PVince81 reviewed Oct 9, 2017
View reviewed changes
tests/KeyManagerTest.php Outdated
@@ -73,9 +73,12 @@ public function setUp() {
->disableOriginalConstructor()
->getMock();
$this->configMock = $this->createMock('OCP\IConfig');
/*$this->configMock->expects($this->any())
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the commented out code.

PVince81
PVince81 reviewed Oct 9, 2017
View reviewed changes
lib/Command/RecreateMasterKey.php Outdated
protected $encUtil;

/** @var IAppManager */
protected $IAppManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

$appManager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with $appManager

PVince81
PVince81 reviewed Oct 9, 2017
View reviewed changes
lib/Command/RecreateMasterKey.php Outdated
$this->IAppManager->disableApp('encryption');

//Delete the files_encryption dir
$filesEncryptionDir = $this->encUtil->getKeyStorageRoot();
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming this works with alternative key storages ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I have tested with alternate key storages

PVince81
PVince81 reviewed Oct 9, 2017
View reviewed changes
lib/KeyManager.php Outdated

$this->masterKeyId = $this->config->getAppValue('encryption',
'masterKeyId');
if (empty($this->masterKeyId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't use empty() for strings, because in PHP empty("0") is true...

Copy link
Contributor

Choose a reason for hiding this comment

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

same above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of using empty() , tried to use !== ''

Copy link
Contributor

Choose a reason for hiding this comment

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

can it be null ? in your other PRs you tested with both is_null and !== ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the check for is_null and !== ''

@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch from 9dce123 to a34a983 Compare October 9, 2017 14:41
@sharidas
Copy link
Contributor Author

sharidas commented Oct 9, 2017

This PR should go along with owncloud/core#29072

@sharidas
Change to re-create masterkeys
4c962f7 
This change brings a new command to re-create
masterkey

A small modification to the decryptall, to
standard output to console.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the withencryptionwrapper-recreate-master branch from a34a983 to 4c962f7 Compare October 9, 2017 16:49
PVince81
PVince81 approved these changes Oct 16, 2017
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit fcf697b into master Oct 16, 2017
@PVince81 PVince81 deleted the withencryptionwrapper-recreate-master branch October 16, 2017 10:04
@PVince81
Copy link
Contributor

@sharidas please backport to stable10

@sharidas sharidas mentioned this pull request Oct 16, 2017
Merged
9 tasks
@sharidas
Copy link
Contributor Author

Backport available here: owncloud/core#29260

@J0WI J0WI mentioned this pull request Jul 6, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

Assignees

@sharidas sharidas

Labels
3 - To Review
Projects
None yet
Milestone
backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@sharidas @PVince81