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

Harden SSE key generation #22018

Merged
merged 3 commits into from
Aug 19, 2020
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Jul 27, 2020

There might be cases where multiple requests trigger the key generation at the same time and the instance ends up with a non-fitting public/private key pair. Therefore the whole key generation should be locked.

Other than that this makes sure that user key generation return values are properly validated.

Somehow related to #8349 but I did not see any reason why the key might be overwritten during operation or upgrade from the existing code, so it is rather related than a fix for this. There is also not much we can do if the keys don't match. We could probably add a setupcheck to warn the admin if the modulus of both keys doesn't match, but this of course just helps to understand the issue.

@juliushaertl
Copy link
Member Author

1) OCA\Encryption\Tests\Users\SetupTest::testSetupUser with data set #1 (false, true)
Failed asserting that false is identical to true.

@juliushaertl juliushaertl force-pushed the bugfix/noid/harden-key-generation branch from 1cab40a to 0bc7ba5 Compare July 28, 2020 10:47
@juliushaertl juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 29, 2020
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.

See inline comments

There might be cases where multiple requests trigger the key generation
at the same time and the instance ends up with a non-fitting
public/private key pair. Therefore the whole key generation should be
locked. Other than that this makes sure that user key generation return
values are properly validated.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@faily-bot
Copy link

faily-bot bot commented Aug 14, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 31784: failure

mariadb10.4-php7.3

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 was 1 failure:

1) Test\Files\ObjectStore\ObjectStoreStorageTest::testCopyOverwrite with data set #6 ('/sòurcē.txt', '/tärgét.txt')
Expected /tärgét.txt to be a copy of /drone/src/tests/data/lorem.txt
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.\n
-Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.\n
-Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.\n
-Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.'
+''

/drone/src/tests/lib/Files/Storage/Storage.php:222
/drone/src/tests/lib/Files/Storage/Storage.php:263

mysql8.0-php7.2

Could not fetch logs

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()
    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()

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.

Looks good beside the typo in the error message. 👍

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

All good indeed besides the typo

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke merged commit da58446 into master Aug 19, 2020
@MorrisJobke MorrisJobke deleted the bugfix/noid/harden-key-generation branch August 19, 2020 18:42
@rullzer
Copy link
Member

rullzer commented Oct 28, 2020

/backport to stable19

@rullzer
Copy link
Member

rullzer commented Oct 28, 2020

/backport to stable18

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