Skip to content

Add a flag to ingore signature check#106

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

Add a flag to ingore signature check#106
sharidas wants to merge 1 commit into
masterfrom
newbypass-signature

Conversation

@sharidas

@sharidas sharidas commented Mar 8, 2019

Copy link
Copy Markdown
Contributor

Add a flag to ignore the signature check for
encrypted files.

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

@sharidas

sharidas commented Mar 8, 2019

Copy link
Copy Markdown
Contributor Author

@sharidas sharidas force-pushed the newbypass-signature branch from 6cdfa35 to fd82f2b Compare March 11, 2019 06:09
@codecov

codecov Bot commented Mar 11, 2019

Copy link
Copy Markdown

Codecov Report

Merging #106 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
- Coverage     64.28%   64.14%   -0.15%     
- Complexity      598      599       +1     
============================================
  Files            33       33              
  Lines          2212     2217       +5     
============================================
  Hits           1422     1422              
- Misses          790      795       +5
Impacted Files Coverage Δ Complexity Δ
appinfo/app.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/Crypto/Crypt.php 54.13% <0%> (-0.46%) 68 <0> (+1)

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 4fbb0d9...aaa53e7. Read the comment docs.

@codecov

codecov Bot commented Mar 11, 2019

Copy link
Copy Markdown

Codecov Report

Merging #106 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
- Coverage     64.28%   64.14%   -0.15%     
- Complexity      598      599       +1     
============================================
  Files            33       33              
  Lines          2212     2217       +5     
============================================
  Hits           1422     1422              
- Misses          790      795       +5
Impacted Files Coverage Δ Complexity Δ
appinfo/app.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/Crypto/Crypt.php 54.13% <0%> (-0.46%) 68 <0> (+1)

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 0ce6b6d...fd82f2b. Read the comment docs.

@sharidas sharidas force-pushed the newbypass-signature branch from fd82f2b to dc36b8c Compare March 14, 2019 06:13

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👎

This should be an internal flag to be used only by transfer ownership temporarily.
If we expose this in the DB, there is a risk that someone will decide to disable this on purpose and compromise encryption security.

Also the occ transfer ownership command would likely overwrite whatever an admin (if they discovered this and disabled it) with its own values.

not

Read data from the event to decide if signature check should
be disabled or not.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the newbypass-signature branch from dc36b8c to aaa53e7 Compare March 15, 2019 07:38
@sharidas

Copy link
Copy Markdown
Contributor Author

Updated this PR, and below is how this PR works:
Reads data from the custom symfony event and then decides if signature check has to be done or not.
Reason for using custom symfony event to read the data:

  • The file reading/writing is done in encryption wrapper. So the exception thrown in Crypt is caught at wrapper, which needs a mechanism to tell the transfer-ownership command to know which file has signature mismatch. This is done via event dispatched from wrapper. Bascially the emitter of the event is the wrapper. And there are 2 listeners for this event:
  1. transfer ownership command
  2. The encryption apps Crypt file

The transfer ownership command would read the events data to figure out if the file copy needs to be done second time ( or say retry ), and then the second listener to the event in the Crypt would read and updates the variable ( if required ) and the decision to enable/disable signature check happens.

Comment thread lib/Crypto/Crypt.php
* @throws DecryptionFailedException
*/
public function symmetricDecryptFileContent($keyFileContents, $passPhrase, $cipher = self::DEFAULT_CIPHER, $version = 0, $position = 0) {
$this->eventDispatcher->addListener('files.aftersignaturemismatch', function (GenericEvent $event) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's just use a static attribute... we already have this kind of dirty practice in the decrypt all command.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm now thinking this is not possible because of the encryption module abstraction.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

idea would be if there was a way to inject generic flags or configs into the encryption module

in your first attempt you used the oc_appconfig table for that, which was a bit too global.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The whole problem is here with the code flow

transfer commnad <----> Encryption Wrapper <-----> Crypt

The transfer needs to output the files which are problematic and then come back for a retry in the encryption wrapper to copy the file again and crypt should know that this time no signature check.

Even I was not fully satisfied with the event logic.

Will see what can be done here..

@sharidas

sharidas commented Apr 4, 2019

Copy link
Copy Markdown
Contributor Author

Closing this PR on behalf of #115

@sharidas sharidas closed this Apr 4, 2019
@sharidas sharidas deleted the newbypass-signature branch April 4, 2019 05:59
@patrickjahns patrickjahns removed this from the development milestone Aug 13, 2019
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