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

Change default encryption behaviour #26847

Closed
JKawohl opened this issue Dec 20, 2016 · 22 comments · Fixed by #27512
Closed

Change default encryption behaviour #26847

JKawohl opened this issue Dec 20, 2016 · 22 comments · Fixed by #27512

Comments

@JKawohl
Copy link
Contributor

JKawohl commented Dec 20, 2016

Current default behaviour: The encryption is per user

Often enough admins do not read the documentation or estimate the risk of this action.
RTFM is imho not an valid excuse here, because the action to restore data is beyond reconfiguring or applying easy changes.

I suggest we use a default encryption with master-key. And make it harder to break your stuff by only allowing the current behaviour with an occ command.

encryption:enable --no-master-key

False understanding of encryption creates hazardous and painful restore sessions, and imho i think setting the default behaviour to master-key encryption would improve ux tremendously.

@felixboehm as discussed
@pmaier1

@PVince81
Copy link
Contributor

There is also a middle ground where we keep the current encryption but force the admin to create recovery keys. Also recovery keys are enabled for every user by default. Basically make it opt-out instead of opt-in.

That said, I'd personally also prefer to have the master key variant enabled by default instead.

@felixboehm
Copy link
Contributor

Yes, let's do opt-out for recovery-key and master key by default.
This is exactly what I hear people ask for.

@PVince81
Copy link
Contributor

This will modify the whole flow of enabling encryption on the GUI and CLI side.
Currently there is no master key option on the GUI so we might need to add it there to make it clear that it's the default mode.

force the admin to create recovery keys

Also need to clear out when and how to do this, UX-wise, in a way that integrates both in the web UI approach when enabling encryption and the CLI approach when enabling encryption.
Forcing a user or admin to do something and having the system in a "locked" state when not done is a bit tricky. For example when encryption keys are missing every file returns 403 access denied, need to find out if that would work here as well.

Maybe we can split this in two separate tasks:

  1. Make master key the default and adjust CLI + web flows, estimate 1-2 md
  2. Make recovery key opt-out and adjust CLI + web flows, estimate 2-3 md

Note that we don't have any automated tests neither for master key nor recovery key: owncloud/QA#340 owncloud/QA#341

I have included design discussions but not QA in the estimates.

@felixboehm
Copy link
Contributor

We can also just randomly generate a master key, if it is not given, and print it for the admin.

@PVince81
Copy link
Contributor

PVince81 commented Jan 9, 2017

@felixboehm you probably mean generate a recovery key password ? The master key is already automatically generated.

@PVince81
Copy link
Contributor

and before recommending master key, need to make sure it actually works correctly in all scenarios: owncloud/QA#340

@PVince81
Copy link
Contributor

@hodyroff
Copy link

This is an interesting one, but one we need to solve for 10.0.0, latest 10.0.1 one way or another.
10.0.0 Alpha does not have the encryption app by default, but the GUI still has the enable servier side encryption option. So it seems to be part of core even.
We have two capable choices it seems:

  1. Bring Masterkey into GUI and give users the choice to enable either. With a clear default recommendation for Masterkey.
  2. Take both out of the GUI which makes it impossible to start encryption without
    In any case we don't want to ship the encryption module with the default, but the GUI is there, so thats somewhat ugly right now ...
    Checked the warnings, they are pretty red and a lot of text already.
    The naming "server side encryption" seems somewhat misleading.
    No migration issues I can think of, as long as we detect previous encryption module usage properly - hope we can test with Beta?

@PVince81
Copy link
Contributor

10.0.0 Alpha does not have the encryption app by default

Neither does 9.1 or 9.0.
You need to enable it from the apps page.

Remember that in the past we added encryption modularity to allow for community to develop their own encryption schemes. This modularity comes with two parts:

  • core part which is the encryption module plugin system. That one needs to be enabled in the admin page "Enable server-side encryption". I think this should stay as it is generic.
  • encryption module plugin: we ship the "default encryption module" as a separate app "encryption" that needs to be manually enable. This is the one that we don't want to be default any more.

Unfortunately the default module is also the one that provides master key even though its implementation is different.

If there was enough time my suggestion for a clean approach would be to split the default module into two:

  • master key encryption app (which we make the recommended default)
  • non-master-key encryption app (formerly known as "default encryption module") which is the one with per-user keypairs and is too secure to support some group sharing scenarios, etc...

@PVince81
Copy link
Contributor

But since we don't have enough time and will likely keep the both master and non-master key in the "default encryption module" app, then we might need to change the app's GUI (not the generic core encryption GUI) to allow enabling master key as suggested by @hodyroff.

Currently the problem is that as soon as this app is enabled, then it already starts using the default mode if people access/create files. Ideally we should then add another step "no encryption mode selected yet" in which no files can be accessed and created, until the admin has picked either "master key" or "non-master key".

@PVince81
Copy link
Contributor

Raised #27438 for separating master-key into a separate app.

@PVince81 PVince81 modified the milestones: 10.0.1, 10.0 Mar 21, 2017
@PVince81
Copy link
Contributor

PVince81 commented Mar 21, 2017

TODOs:

  • add additional step after enabling "default encryption module app":
    • settings page for app shows dropdown with choice between "master key" (new defaults) and "user-specific keys aka non-master-key" (need a better name!).
    • Button to confirm encryption mode (with warning that this is not reversible)
    • As long as no choice was made, no other settings are displayed (usually there's something about recovery keys or a message that keys aren't initialized)
  • when no encryption mode was selected, all filesystem calls fail with 403 Forbidden (similar to when encryption keys were not enabled, maybe the same API can be used to return false to trigger this behavior)
  • add endpoint for settings page choices with ability to enable master key
  • add OCC command to enable non-master key for consistency, because some scripts or admins need a way to also enable the old mode there since it's not enabled by default any more

@pmaier1
Copy link
Contributor

pmaier1 commented Mar 21, 2017

  • master key encryption app (which we make the recommended default)
  • non-master-key encryption app (formerly known as "default encryption module") which is the one with per-user keypairs and is too secure to support some group sharing scenarios, etc...

Actually I would be very much in favor of getting this done in a clean way as @PVince81 suggested and then have encryption modularized:

  • Core encryption module
    • User/PW encryption plugin
    • Master-key encryption plugin (default)
    • HSM plugin(s)
  • Cipher modules (?!)

Can this still be done at a later point without generating additional efforts or do we have to decide now?

@PVince81
Copy link
Contributor

If we go with the quick route (adding an extra state as I described above) the extra effort would be to remove that state again from the app. But I think removing should be easier than adding it.

@pmaier1
Copy link
Contributor

pmaier1 commented Mar 21, 2017

Ok then we should probably walk the quick route and get stuff sorted afterwards.
Do we need to discuss this again or are things clear or do you still need input/concept at some point?

@PVince81
Copy link
Contributor

I think things were clear from @hodyroff's comments.

@sharidas will take care of this. Let us know if you have questions.

@sharidas
Copy link
Contributor

I have query regarding the comment. So lets say I made 3 options as drop down as http://imgur.com/a/ysF9b. The options listed as "Master Key", "Custom Key" and "None". So when user selects "Master Key" or "Custom Key" and clicks the button "Confirm Selection" a warning label is raised. Now inside the warning do we need to have a confirmation asking: "Do you still want to proceed" or so? By default I have made "None" as default and the option underneath like "Enable recovery key" is hidden for that option.

@PVince81
Copy link
Contributor

  • Rename "type of key" with "encryption type" maybe.
  • Rename "None" to "Please select an encryption type" because "None" is not a valid option.
  • Rename "Custom key" to "User-specific keys"
  • As long as nothing is confirmed, anything below this dropdown must stay hidden.
  • After confirmation I'd suggest a full page reload, just in case. Some other code parts that run on page load might depend on the encryption type.
  • Not sure if an additional confirmation is needed. IF we do, then use OC.dialogs.confirm() to display an ugly popup.

@PVince81
Copy link
Contributor

PVince81 commented May 8, 2017

PR was merged #27512, will be in 10.0.1 release

@PVince81
Copy link
Contributor

PVince81 commented May 8, 2017

Now the "Master key" choice is the first one in the dropdown. Should we add (recommended) behind it ?

But before recommending explicitly I suggest we first address the remaining issues with master key: #27427

@hodyroff
Copy link

hodyroff commented May 8, 2017

Thanks! Candidate for 10.0.2 then. IMHO.

@lock
Copy link

lock bot commented Jul 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants