Integrate HSM changes to encryption #90

sharidas · 2019-01-21T17:06:55Z

Integrate HSM changes to encryption

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

codecov · 2019-01-22T05:54:37Z

Codecov Report

Merging #90 into master will increase coverage by 1.53%.
The diff coverage is 85.38%.

@@             Coverage Diff              @@
##             master      #90      +/-   ##
============================================
+ Coverage     62.75%   64.28%   +1.53%     
- Complexity      568      594      +26     
============================================
  Files            30       33       +3     
  Lines          2081     2201     +120     
============================================
+ Hits           1306     1415     +109     
- Misses          775      786      +11

Impacted Files	Coverage Δ	Complexity Δ
lib/Crypto/Crypt.php	`54.58% <ø> (+0.83%)`	`67 <0> (ø)`	⬇️
lib/AppInfo/Application.php	`0% <0%> (ø)`	`25 <0> (+4)`	⬆️
lib/Users/Setup.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
lib/Command/HSMDaemon.php	`100% <100%> (ø)`	`7 <7> (?)`
lib/JWT.php	`100% <100%> (ø)`	`5 <5> (?)`
lib/Recovery.php	`79.62% <100%> (ø)`	`37 <0> (ø)`	⬇️
lib/Hooks/UserHooks.php	`72.82% <100%> (ø)`	`36 <0> (ø)`	⬇️
lib/KeyManager.php	`73.06% <50%> (ø)`	`83 <0> (ø)`	⬇️
lib/Crypto/CryptHSM.php	`92.85% <92.85%> (ø)`	`10 <10> (?)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6702a06...5fea266. Read the comment docs.

codecov · 2019-01-22T05:54:37Z

Codecov Report

Merging #90 into master will increase coverage by 1.51%.
The diff coverage is 85.49%.

@@             Coverage Diff              @@
##             master      #90      +/-   ##
============================================
+ Coverage     62.75%   64.26%   +1.51%     
- Complexity      572      598      +26     
============================================
  Files            30       33       +3     
  Lines          2089     2211     +122     
============================================
+ Hits           1311     1421     +110     
- Misses          778      790      +12

Impacted Files	Coverage Δ	Complexity Δ
lib/Crypto/Crypt.php	`54.58% <ø> (+0.83%)`	`67 <0> (ø)`	⬇️
lib/AppInfo/Application.php	`0% <0%> (ø)`	`25 <0> (+4)`	⬆️
lib/Users/Setup.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
lib/Command/HSMDaemon.php	`100% <100%> (ø)`	`7 <7> (?)`
lib/JWT.php	`100% <100%> (ø)`	`5 <5> (?)`
lib/Recovery.php	`78.44% <100%> (ø)`	`41 <0> (ø)`	⬇️
lib/Hooks/UserHooks.php	`72.82% <100%> (ø)`	`36 <0> (ø)`	⬇️
lib/KeyManager.php	`73.06% <50%> (ø)`	`83 <0> (ø)`	⬇️
lib/Crypto/CryptHSM.php	`92.85% <92.85%> (ø)`	`10 <10> (?)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f17d4b2...994b361. Read the comment docs.

phil-davis · 2019-01-22T06:27:03Z

@sharidas PR #92 fixed an issue related to changes to core CI yesterday. That has been merged and CI passes now in this app.

You will need to rebase this PR to get CI to pass.

sharidas · 2019-01-22T06:28:44Z

@phil-davis Rebased the PR. Thanks for the helping hand.

sharidas · 2019-01-22T10:18:07Z

Integrated the HSM patch with the encryption app
Unit test added. The tests would just improve coverage.
Command encryption:hsmdaemon is added as part of the patch.
Have not tested encryption:hsmdaemon.

PVince81

Looks fine to me apart from some minor things

@butonic quick look ?

lib/AppInfo/Application.php

lib/Command/HSMDaemon.php

PVince81 · 2019-02-04T13:47:26Z

@sharidas can you test it ? #90 (comment)

would also be good to have a docs ticket that explain how to set this up with the mock HSM for testing

sharidas · 2019-02-05T14:10:14Z

Updating my tests here... regarding #90 (comment)

Regarding encryption:hsmdaemon below are the options available in the command:

--export-masterkey
--import-masterkey=IMPORT-MASTERKEY
--decrypt=DECRYPT

The --import-masterkey option is missing in the code. May be its a TODO.

export-masterkey

export-masterkey prints the base64_encode of the file data/files_encryption/OC_DEFAULT_MODULE/master_*.privateKey

decrypt

Quite not able to get this option.

butonic · 2019-02-11T11:34:25Z

lib/AppInfo/Application.php

-					$server->getUserSession(),
-					$server->getConfig(),
-					$server->getL10N($c->getAppName()));
+				$hsmUrl = $this->config->getAppValue('encryption', 'hsm.url');


just do document ... this causes the hsm.url app config to be two things:

the url for the hsm endpoint

the toggle for using an hsm or the internal crypto implementation

these are two different things and I have a bad feeling about it. Maybe an additional property crypto.engine with internal and hsm makes more sense.

a second key would be better, yes. @sharidas please add

Updated the PR, by setting https://github.com/owncloud/encryption/pull/90/files#diff-b8a11151e44bcffc222c5fddc9ab92c9R133

Have also verified that it works with and without hsm.

butonic · 2019-02-11T11:48:55Z

@PVince81 a detailed setup is in the hsmdaemon Readme.md and Installation.md

I would merge and deal with the todos if we get opportunity for it.

PVince81 · 2019-02-11T14:46:41Z

@sharidas please check the test failures

sharidas · 2019-02-12T07:34:57Z

Most likely this failure https://drone.owncloud.com/owncloud/encryption/479/576 has nothing to do with my change. Because the failure happens in scenario https://github.com/owncloud/core/blob/master/tests/acceptance/features/webUISharingExternal/federationSharing.feature#L293 which has a tag @skipOnEncryption. Let me rebase and try my luck again.

sharidas · 2019-02-15T08:15:57Z

I have updated the PR based on the requests made:

PVince81 · 2019-02-15T12:32:51Z

restarted cancelled build

PVince81 · 2019-02-15T13:45:34Z

can't restart the build https://drone.owncloud.com/owncloud/encryption/494/1297 :-(

maybe rebase ?

sharidas · 2019-02-15T13:47:02Z

can't restart the build https://drone.owncloud.com/owncloud/encryption/494/1297 :-(

maybe rebase ?

Rebasing it.

Integrate HSM changes to encryption. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2019-02-18T11:21:55Z

Backport to stable10: owncloud/core#34527

sharidas added the 2 - Developing label Jan 21, 2019

sharidas added this to the development milestone Jan 21, 2019

sharidas self-assigned this Jan 21, 2019

sharidas force-pushed the hsm-integration branch from f496b35 to 5fea266 Compare January 22, 2019 05:54

sharidas force-pushed the hsm-integration branch from 5fea266 to a5e451b Compare January 22, 2019 06:28

sharidas changed the title ~~[WIP] Integrate HSM changes to encryption~~ Integrate HSM changes to encryption Jan 22, 2019

sharidas requested a review from PVince81 January 22, 2019 15:41

sharidas added 3 - To Review and removed 2 - Developing labels Jan 22, 2019

PVince81 reviewed Jan 24, 2019

View reviewed changes

sharidas force-pushed the hsm-integration branch 2 times, most recently from b9b482d to 29c98ec Compare January 25, 2019 09:17

sharidas force-pushed the hsm-integration branch from 29c98ec to c4fe345 Compare February 6, 2019 06:28

sharidas mentioned this pull request Feb 7, 2019

How to enable HSM with encryption owncloud/docs#586

Closed

12 tasks

sharidas force-pushed the hsm-integration branch from c4fe345 to d2eda5d Compare February 7, 2019 11:40

PVince81 assigned butonic Feb 8, 2019

butonic reviewed Feb 11, 2019

View reviewed changes

butonic approved these changes Feb 11, 2019

View reviewed changes

sharidas force-pushed the hsm-integration branch from d2eda5d to 74b6ed3 Compare February 12, 2019 07:36

sharidas mentioned this pull request Feb 15, 2019

Add an additional command to hsmdaemon to create random secret key and setup encryption #100

Open

This was referenced Feb 15, 2019

Hsmdaemon: Add encrypt masterkey command as an option #101

Open

Hsmdaemon: add decrypt option #102

Open

sharidas force-pushed the hsm-integration branch from 74b6ed3 to da1ed90 Compare February 15, 2019 07:57

sharidas force-pushed the hsm-integration branch from da1ed90 to d5fb771 Compare February 15, 2019 13:47

Integrate HSM changes to encryption

994b361

Integrate HSM changes to encryption. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the hsm-integration branch from 4a50f84 to 994b361 Compare February 18, 2019 06:05

PVince81 merged commit c1f92f4 into master Feb 18, 2019

delete-merged-branch bot deleted the hsm-integration branch February 18, 2019 09:17

sharidas added backport-request and removed 3 - To Review labels Feb 18, 2019

sharidas mentioned this pull request Feb 18, 2019

[stable10] Backport of Integrate HSM changes to encryption owncloud/core#34527

Merged

11 tasks

sharidas added 4 - To release and removed backport-request labels Feb 18, 2019

individual-it restored the hsm-integration branch February 25, 2019 11:29

individual-it deleted the hsm-integration branch February 25, 2019 11:29

patrickjahns removed this from the development milestone Aug 13, 2019

mmattel mentioned this pull request Jun 24, 2021

[QA] missing implementation of an existing occ command option #293

Open

Integrate HSM changes to encryption #90

Integrate HSM changes to encryption #90

Uh oh!

Conversation

sharidas commented Jan 21, 2019

Uh oh!

codecov bot commented Jan 22, 2019

Codecov Report

Uh oh!

codecov bot commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phil-davis commented Jan 22, 2019

Uh oh!

sharidas commented Jan 22, 2019

Uh oh!

sharidas commented Jan 22, 2019

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PVince81 commented Feb 4, 2019

Uh oh!

sharidas commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

export-masterkey

decrypt

Uh oh!

butonic Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

PVince81 Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

sharidas Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

butonic commented Feb 11, 2019

Uh oh!

PVince81 commented Feb 11, 2019

Uh oh!

sharidas commented Feb 12, 2019

Uh oh!

sharidas commented Feb 15, 2019

Uh oh!

PVince81 commented Feb 15, 2019

Uh oh!

PVince81 commented Feb 15, 2019

Uh oh!

sharidas commented Feb 15, 2019

Uh oh!

sharidas commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jan 22, 2019 •

edited

Loading

sharidas commented Feb 5, 2019 •

edited

Loading