Skip to content

Disable signature check if the file copy fails in encrypted file#34718

Closed
sharidas wants to merge 1 commit into
masterfrom
newbypass-signature
Closed

Disable signature check if the file copy fails in encrypted file#34718
sharidas wants to merge 1 commit into
masterfrom
newbypass-signature

Conversation

@sharidas

@sharidas sharidas commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

Disable signature check if the file copy fials in the encrypted
file, due to signature mismatch. The code has been adjusted
so that it would try by ignoring the signature mismatch. If the
code fails again, then it would throw the exception.

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

Description

Problem

When files are transferred using transfer ownership command, if there occurs to be a signature mismatch, in any of the encrypted files, the command fails. Idea used here is to use the flag at owncloud/encryption#106.

Idea used

  • Allow signature check to happen in the transfer ownership command before the transfer happens.
  • If the signature mismatch happens for any of the file, log then and there in the oc logs.
    • Disable signature check
    • Retry the transfer of file
    • Once the transfer is successful, revert back the flag to enable signature check.
    • If the file transfer fails again, throw exception which would lead to termination of transfer command.
  • The transfer ownership command listens to an event which provides the way to enable/disable signature check. I preferred to do this in the transfer ownership command, because it would look really ugly to do this enable/disable of signature check in the Encryption wrapper code. Apart from this signal mechanism, I am not able to figure out a clean way to pass the file name which will be logged in the console, so that user would see the files affected in the console in real time.

Related Issue

  • Fixes <issue_link>

Motivation and Context

Help transfer ownership command move ahead and process the transfer of file(s) and abort only when the copy of a file is failed twice ( with and without signature check).

How Has This Been Tested?

  • Create 2 users user1 and user2 ( make sure both users are logged in )
  • Enable masterkey encryption
  • As user2 create a folder test and create 3 files inside test folder :
    • a.txt
    • b.txt
    • big.txt
  • Modify the filecache table for a.txt and big.txt as shown below
mysql> select fileid,encrypted from oc_filecache where path="files/test/big.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     64 |         1 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> update oc_filecache set encrypted=10 where fileid=64;
Query OK, 1 row affected (0.01 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql> select fileid,encrypted from oc_filecache where path="files/test/a.txt";
+--------+-----------+
| fileid | encrypted |
+--------+-----------+
|     45 |         3 |
+--------+-----------+
1 row in set (0.00 sec)

mysql> update oc_filecache set encrypted=10 where fileid=45;
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql>
  • Now run the transfer ownership command as shown below:
sujith@sujith-ownCloud  ~/test/owncloud   newbypass-signature  ./occ files:transfer-ownership user1 user2                                                           
Cannot load Xdebug - it was already loaded
Analysing files of user1 ...
    4 [============================]
Collecting all share information for files and folder of user1 ...
    0 [>---------------------------]
Transferring files to user2/files/transferred from user1 on 20190308_113635 ...
The file affected by signature mismatch: files/test/a.txt
The file affected by signature mismatch: files/test/big.txt
Restoring shares ...
    0 [>---------------------------]
 sujith@sujith-ownCloud  ~/test/owncloud   newbypass-signature 
  • User should see 2 files a.txt and big.txt will be mentioned in the console log and the oc log.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@sharidas

sharidas commented Mar 8, 2019

Copy link
Copy Markdown
Contributor Author

Kindly let me know if this idea looks ok, thanks.

@sharidas
sharidas force-pushed the newbypass-signature branch 2 times, most recently from 82de3cd to 9e46e19 Compare March 14, 2019 06:12
@codecov

codecov Bot commented Mar 14, 2019

Copy link
Copy Markdown

Codecov Report

Merging #34718 into master will decrease coverage by 0.04%.
The diff coverage is 26.31%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34718      +/-   ##
============================================
- Coverage      65.3%   65.26%   -0.05%     
- Complexity    18478    18494      +16     
============================================
  Files          1209     1210       +1     
  Lines         69964    70020      +56     
  Branches       1280     1280              
============================================
+ Hits          45689    45697       +8     
- Misses        23903    23951      +48     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.66% <26.31%> (-0.05%) 18494 <10> (+16)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 0% <0%> (ø) 52 <4> (+6) ⬆️
lib/private/Files/Storage/Wrapper/Encryption.php 63.63% <43.47%> (-1.23%) 163 <6> (+7)
lib/private/Server.php 86.56% <0%> (-0.12%) 253% <0%> (ø)
core/register_command.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
core/Command/Db/GenerateChangeScript.php 0% <0%> (ø) 3% <0%> (?)

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 a37e12c...9e46e19. Read the comment docs.

@codecov

codecov Bot commented Mar 14, 2019

Copy link
Copy Markdown

Codecov Report

Merging #34718 into master will decrease coverage by 0.01%.
The diff coverage is 27.02%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34718      +/-   ##
============================================
- Coverage      65.3%   65.28%   -0.02%     
- Complexity    18479    18491      +12     
============================================
  Files          1209     1209              
  Lines         69970    70006      +36     
  Branches       1280     1280              
============================================
+ Hits          45693    45704      +11     
- Misses        23905    23930      +25     
  Partials        372      372
Flag Coverage Δ Complexity Δ
#javascript 53.04% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.69% <27.02%> (-0.03%) 18491 <10> (+12)
Impacted Files Coverage Δ Complexity Δ
apps/files/lib/Command/TransferOwnership.php 0% <0%> (ø) 51 <4> (+5) ⬆️
lib/private/Files/Storage/Wrapper/Encryption.php 63.63% <43.47%> (-1.23%) 163 <6> (+7)
lib/private/Server.php 86.68% <0%> (+0.11%) 253% <0%> (ø) ⬇️
...eratedfilesharing/lib/Controller/OcmController.php 66.26% <0%> (+0.2%) 30% <0%> (ø) ⬇️

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 48f7a8f...afcde27. Read the comment docs.

@sharidas
sharidas force-pushed the newbypass-signature branch from 9e46e19 to 5ea5456 Compare March 15, 2019 07:26
@sharidas

Copy link
Copy Markdown
Contributor Author

Added a comment at owncloud/encryption#106 (comment)

Disable signature check if the file copy fails in the encrypted
file, when files are transferred using transfer-ownership command.
When the file is retried with disabled signature check and if
the file is again hit with an exception the transfer ownership
is stopped from execution.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas
sharidas force-pushed the newbypass-signature branch from 5ea5456 to afcde27 Compare March 15, 2019 08:11
@sharidas

sharidas commented Apr 5, 2019

Copy link
Copy Markdown
Contributor Author

Closing this PR in favour of owncloud/encryption#115

@sharidas sharidas closed this Apr 5, 2019
@sharidas
sharidas deleted the newbypass-signature branch April 5, 2019 08:00
@lock lock Bot locked as resolved and limited conversation to collaborators Apr 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants