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

limitations of encryption #70

Merged
merged 7 commits into from
Sep 20, 2018
Merged

limitations of encryption #70

merged 7 commits into from
Sep 20, 2018

Conversation

voroyam
Copy link
Contributor

@voroyam voroyam commented Sep 7, 2018

known limitations of user key encryption
owncloud-archive/documentation#4179

known limitations of user key encryption
* elastic search will not work
* user getting access to external storage already containing existing encrypted files cannot get access to said files for similar reasons like the group case above
* When having data shared with a group and group membership changes after the share is established, subsequently added users will not be able to open the shared data unless the owner will share it again

Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this paragraph as last part of Before Enabling Encryption and reword the headline:
Here are the limitations of this type of encryption -->
Limitations of User-Key Based Encryption

Please also see the header size which imho does not look good, limitations should be a bit smaller than the header before
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I adjusted the header size.

On the first point though, I must disagree.

If we put this info at the top, far away from the commands that enable the user key encryption I fear that users will just skip that section, and scroll down to the commands.

When we, on the other hand, put the warning directly above the commands, the chance of the users to read this is higher imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is, that there is no mentioning of limitations in Before Enabling Encryption which is the must read part before you go for what ever type of encryption.
But we can bring our points together :-)
Move it up and refrence to it from below like like Before you enable, please see the limitations described above.

@settermjd can we ask you for a review of our thoughts ?

Copy link
Contributor

@settermjd settermjd Sep 12, 2018

Choose a reason for hiding this comment

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

After having a look at the existing documentation and its structure, I'd keep the order as it is, as it seems to be correct because warnings for each type of encryption are provided before (yet alongside) the commands to enable it. However, I'd make several changes:

  • Add a new, third-level, header: "Master Key Encryption", after the existing header "Encryption Types".
  • Reflow all the existing headers down to, and including "Disabling Encryption" to be in sync with that header level.
  • Add a new, third-level, header: "User-Key Encryption", before "Enabling User-Key Based Encryption From the Command-line".
  • Reword "Here are the limitations of this type of encryption:" as "Encryption Limitations".
  • Make "To enable User-Key based encryption:" a fourth-level header.
  • Reflow all the existing headers down to, and including "Disabling Encryption" to be in sync with that header level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Add a new, third-level, header: "Master Key Encryption", after the existing header "Encryption Types".

right after "Encryption Types" comes:
"ownCloud provides two encryption types"

Putting a "Master Key Encryption Header" there makes no sense to me. Could you specify?

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent is to make it clear where Master Key encryption starts, as there are two encryption types.

* User added to group cannot decrypt files on existing shares
* OnlyOffice will not work
* Impersonate will not work
* oauth2 does will not work
Copy link
Contributor

Choose a reason for hiding this comment

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

s/oauth2/OAuth2/g

@@ -229,6 +230,16 @@ You may only disable encryption by using the `occ Encryption Commands`_. Make su

== Enabling User-Key Based Encryption From the Command-line

=== Here are the limitations of this type of encryption:

* User added to group cannot decrypt files on existing shares
Copy link
Contributor

Choose a reason for hiding this comment

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

s/User added to group/Users added to groups/g

* OnlyOffice will not work
* Impersonate will not work
* oauth2 does will not work
* elastic search will not work
Copy link
Contributor

Choose a reason for hiding this comment

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

s/elastic search/Elasticsearch/g

* Impersonate will not work
* oauth2 does will not work
* elastic search will not work
* user getting access to external storage already containing existing encrypted files cannot get access to said files for similar reasons like the group case above
Copy link
Contributor

@settermjd settermjd Sep 12, 2018

Choose a reason for hiding this comment

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

I'd rewrite this as:

Users getting access to an external storage which already contains existing encrypted files cannot get access to said files for reasons such as the group case above.

Copy link
Contributor

@settermjd settermjd left a comment

Choose a reason for hiding this comment

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

Please see the changes requested. A pain, to be sure, but I believe they'll move the content in the right direction.

@settermjd settermjd added enhancement New feature or request work in progress Still in development. Not to be merged. labels Sep 12, 2018
@voroyam
Copy link
Contributor Author

voroyam commented Sep 17, 2018

@settermjd do you need something from me to finish this?

@settermjd
Copy link
Contributor

@voroyam, doesn't look like it.

@settermjd settermjd merged commit 0e8f868 into master Sep 20, 2018
@settermjd settermjd deleted the encryption-limitations branch September 20, 2018 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request work in progress Still in development. Not to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants