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.

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

New SSE key format #21529

Merged
merged 1 commit into from
Aug 20, 2020
Merged

New SSE key format #21529

merged 1 commit into from
Aug 20, 2020

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Jun 22, 2020

  • Encrypt the keys with the instance secret
  • Store them as json (so we can add other things if needed)

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added the 2. developing Work in progress label Jun 22, 2020
@rullzer rullzer added this to the Nextcloud 20 milestone Jun 22, 2020
@rullzer rullzer requested a review from nickvergessen June 22, 2020 13:37
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense 👍

@rullzer
Copy link
Member Author

rullzer commented Aug 11, 2020

So I way pointed to another corner case here.
But I'll look into that tomorrow.

@rullzer rullzer force-pushed the enh/encryption/improve_key_format branch from ade6ae5 to aa38724 Compare August 12, 2020 10:42
$progress->start();


foreach ($this->userManager->getBackends() as $backend) {
Copy link
Member

Choose a reason for hiding this comment

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

May use callForSeenUsers of the IUserManager instead of the for each loop

Copy link
Member

Choose a reason for hiding this comment

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

Kind a agree, but on the other hand we must migrate all users encryption keys, not only the once that ever logged in.
Not sure if this is possible to differ, but save is better than sorry (looking at you ldap and external user backends).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep good point


$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
throw new ServerNotAvailableException('Invalid encryption key');
Copy link
Member

Choose a reason for hiding this comment

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

Write the json_decode error?

if (!$fallback) {
$dataArray = json_decode($clearData, true);
if ($dataArray === null) {
throw new ServerNotAvailableException('Invalid encryption key');
Copy link
Member

Choose a reason for hiding this comment

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

Log the JSON error? Or give it to the exception.

@MorrisJobke
Copy link
Member

@rullzer This needs some work

@rullzer rullzer force-pushed the enh/encryption/improve_key_format branch 3 times, most recently from bfb721d to 0abdc26 Compare August 20, 2020 07:49
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

🐘 works

Comment on lines 29 to 39
use OC\Encryption\Keys\Storage;
use OC\Encryption\Util;
use OC\Files\Filesystem;
use OC\Files\View;
use OCP\IConfig;
use OCP\IUserManager;
use OCP\Security\ICrypto;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\ProgressBar;
use Symfony\Component\Console\Helper\QuestionHelper;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
Copy link
Member

Choose a reason for hiding this comment

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

something is unused it seems

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@rullzer rullzer force-pushed the enh/encryption/improve_key_format branch from cecf32c to 6c3dda4 Compare August 20, 2020 11:13
@nickvergessen
Copy link
Member

There was 1 failure:

  1. Test\Encryption\Keys\StorageTest::testSetFileOld

* Encrypt the keys with the instance secret
* Store them as json (so we can add other things if needed)

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the enh/encryption/improve_key_format branch from 6c3dda4 to 5340ab3 Compare August 20, 2020 13:42
@rullzer
Copy link
Member Author

rullzer commented Aug 20, 2020

There was 1 failure:

1. Test\Encryption\Keys\StorageTest::testSetFileOld

yeah wrong order thing. fix pushed

@faily-bot
Copy link

faily-bot bot commented Aug 20, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 32091: failure

mysql8.0-php7.2

Show full log
There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

--

There were 2 failures:

1) OCA\FederatedFileSharing\Tests\FederatedShareProviderTest::testDeleteUser with data set #3 ('a', 'b', 'c', 'd', false)
Failed asserting that actual size 0 matches expected size 1.

/drone/src/apps/federatedfilesharing/tests/FederatedShareProviderTest.php:717

2) OCA\Files_Versions\Tests\VersioningTest::testRestoreMovedShare
File content has not changed
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'version 2'
+'version 1'

/drone/src/apps/files_versions/tests/VersioningTest.php:729

mysql5.6-php7.2

Show full log
There was 1 error:

1) OCA\FederatedFileSharing\Tests\FederatedShareProviderTest::testGetSharedByWithNode
OCP\Share\Exceptions\ShareNotFound: There was an error retrieving the share. Maybe the link is wrong, it was unshared, or it was deleted.

/drone/src/apps/federatedfilesharing/lib/FederatedShareProvider.php:863
/drone/src/apps/federatedfilesharing/lib/FederatedShareProvider.php:242
/drone/src/apps/federatedfilesharing/tests/FederatedShareProviderTest.php:579

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

acceptance-app-files

  • tests/acceptance/features/app-files.feature:262
Show full log
  Scenario: unmarking a file as favorite causes the file list to be sorted again                          # /drone/src/tests/acceptance/features/app-files.feature:262
    Given I am logged in                                                                                  # LoginPageContext::iAmLoggedIn()
    And I create a new folder named "A name alphabetically lower than welcome.txt"                        # FileListContext::iCreateANewFolderNamed()
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    And I close the details view                                                                          # FilesAppContext::iCloseTheDetailsView()
    And I see that the details view is closed                                                             # FilesAppContext::iSeeThatTheDetailsViewIsClosed()
      │ Details view in Files app visibility could not be got
      │ Exception message: Element not found with xpath, (//html//*[@id="app-sidebar" or contains(@class, 'app-sidebar')])[1]
      │ 
      │ Unable to locate element: {"method":"xpath","selector":"(//html//*[@id=\"app-sidebar\" or contains(@class, 'app-sidebar')])[1]"}
      │ For documentation on this error, please visit: http://seleniumhq.org/exceptions/no_such_element.html
      │ Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
      │ System info: host: '36ae1a036df0', ip: '172.27.0.2', os.name: 'Linux', os.arch: 'amd64', os.version: '4.15.0-76-generic', java.version: '1.8.0_91'
      │ Driver info: driver.version: unknown
      │ Trying again
      │ 
    And I mark "welcome.txt" as favorite                                                                  # FileListContext::iMarkAsFavorite()
    And I see that "welcome.txt" is marked as favorite                                                    # FileListContext::iSeeThatIsMarkedAsFavorite()
    And I see that "welcome.txt" precedes "A name alphabetically lower than welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()
    When I unmark "welcome.txt" as favorite                                                               # FileListContext::iUnmarkAsFavorite()
    Then I see that "welcome.txt" is not marked as favorite                                               # FileListContext::iSeeThatIsNotMarkedAsFavorite()
      Not favorited state icon for file welcome.txt in file list could not be found after 100 seconds (NoSuchElementException)
    And I see that "A name alphabetically lower than welcome.txt" precedes "welcome.txt" in the file list # FileListContext::iSeeThatPrecedesInTheFileList()

@MorrisJobke MorrisJobke merged commit 65b5e65 into master Aug 20, 2020
@MorrisJobke MorrisJobke deleted the enh/encryption/improve_key_format branch August 20, 2020 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants