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

feat: drop setup of user based encryption #389

Merged
merged 5 commits into from
Mar 29, 2023

Conversation

DeepDiver1975
Copy link
Member

No description provided.

@DeepDiver1975 DeepDiver1975 force-pushed the feat/disallow-setup-user-based-enc branch from 97d2af7 to ea3b905 Compare March 28, 2023 08:33
@DeepDiver1975 DeepDiver1975 force-pushed the feat/disallow-setup-user-based-enc branch from ea3b905 to 819e5e4 Compare March 28, 2023 10:03
@DeepDiver1975 DeepDiver1975 force-pushed the feat/disallow-setup-user-based-enc branch from 819e5e4 to fd6e912 Compare March 28, 2023 10:17
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiMasterKeys-latest-mysql8.0-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/encryption/2968/16

@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline cliEncryption-latest-postgres9.4-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/encryption/2968/13

@mrow4a
Copy link
Contributor

mrow4a commented Mar 28, 2023

@DeepDiver1975 I did similar investigation in #388 , as you already created PR might be worth to crosscheck with my PR, because I see few parts are different in your way.

One remark: I think we cannot have none of the modules enabled. At least one needs to be auto-enabled. Otherwise it would require too much hacking. But I think in your PR you enable master-key encryption by default right ?

@mrow4a mrow4a self-requested a review March 28, 2023 12:42
@DeepDiver1975
Copy link
Member Author

Yes. As soon as encryption is enabled and the encryption app is enabled as well master key is enabled as well.
Unless user key was already there before.

@mrow4a
Copy link
Contributor

mrow4a commented Mar 28, 2023

Also I think it would be worth to add this block to inform about depreciation

<?php if (\OC::$server->getAppConfig()->getValue("encryption", "userSpecificKey", "") !== ""): ?>
		<p name="userSpecificKeyWarning" id="userSpecificKeyWarning">
		<span class="msg warning"><?php print_unescaped($l->t("User Specific Key encryption module is deprecated. See the <a target=\"_blank\" rel=\"noreferrer\" href=\"%s\">server release notes ↗</a> for more information.", ["https://doc.owncloud.com/docs/next/server_release_notes.html#deprecation-note-for-user-key-storage-encryption"]));  ?></span>
		</p>
		<br/>
<?php endif; ?>

@DeepDiver1975
Copy link
Member Author

Feel free to push it to this branch

@CLAassistant
Copy link

CLAassistant commented Mar 28, 2023

CLA assistant check
All committers have signed the CLA.

@mrow4a
Copy link
Contributor

mrow4a commented Mar 28, 2023

@DeepDiver1975 added a commit that cleans a bit the backwards compatible flow and adds the decpreciation note for user-key
Screenshot 2023-03-28 at 15 37 11

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/encryption/2971/6/5

There was 1 failure:

1) OCA\Encryption\Tests\Hooks\UserHooksTest::testLogin
Expectation failed for method name is "getAppValue" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

@phil-davis
Copy link
Contributor

Code looks good.

For support and regression testing, do we still want some way to create an oC10 install with user-key encryption?

For example, support is investigating some issue with a production installation somewhere and wants to try and reproduce the problem. How can the support team startup a test oC10, and enable encryption with user-key?

And as long as we still support user-key encryption, then we want to avoid accidentally introducing regressions. So it would be good t have some way that a test environment (preferably automated) can enable user-key encryption and then run the test suite (and/or some manual tests).

Maybe it is as easy as:

\OC::$server->getAppConfig()->getValue("encryption", "userSpecificKey", "")

setting that app config value to 1, and "it will just happen"?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

25.0% 25.0% Coverage
0.0% 0.0% Duplication

@mrow4a
Copy link
Contributor

mrow4a commented Mar 29, 2023

@phil-davis I think user-key was deprecated in 10.7, which was very long ago. I think it is fair to stop testing it by now already.
ref. https://doc.owncloud.com/docs/next/server_release_notes.html#deprecation-note-for-user-key-storage-encryption

@DeepDiver1975
Copy link
Member Author

10.7 was release 2 years ago - long enough .... let it die ....

@mrow4a
Copy link
Contributor

mrow4a commented Apr 17, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants