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

Respect home folder setting in encryption #26818

Closed
wants to merge 1 commit into from

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Dec 13, 2016

Description

In case the user backend defines the user home folder to be in a different place then data/$uid encryption will fail to find the keys.

How Has This Been Tested?

  1. use ldap with special home folder rule
  2. enable encryption
  3. share an encrypted file with another user
  4. 💥
Could not encrypt file for _8060: Public Key missing for user: _8060

In case you cannot setup an ldap use this patch:

Index: lib/private/User/Database.php
I===================================================================
--- lib/private/User/Database.php	(revision fa7a82fe0526645e00fed51d5f9ce869b2b67660)
+++ lib/private/User/Database.php	(revision )
@@ -280,6 +280,7 @@
 	 */
 	public function getHome($uid) {
 		if ($this->userExists($uid)) {
+			$uid = md5($uid);
 			return \OC::$server->getConfig()->getSystemValue("datadirectory", \OC::$SERVERROOT . "/data") . '/' . $uid;
 		}

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@mention-bot
Copy link

@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81 to be a potential reviewer.

@PVince81
Copy link
Contributor

Weird, normally when using views the user's home folder is supposed to be already mounted there.
I'll have a closer look... Could also be that initMountPoints was missing so the mount is not there.

@DeepDiver1975
Copy link
Member Author

Weird, normally when using views the user's home folder is supposed to be already mounted there.

As far as I can tell the user's folder is properly mounted but the encryption code tries to access the keys with the uid - ignoring the home folder setting.
There are many more locations in the code where this fails .....

Copy link
Contributor

@felixboehm felixboehm left a comment

Choose a reason for hiding this comment

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

👍 confirmed to work so far

@@ -170,8 +170,8 @@ protected function constructUserKeyPath($encryptionModuleId, $keyId, $uid) {
if ($uid === null) {
$path = $this->root_dir . '/' . $this->encryption_base_dir . '/' . $encryptionModuleId . '/' . $keyId;
} else {
$path = $this->root_dir . '/' . $uid . $this->encryption_base_dir . '/'
. $encryptionModuleId . '/' . $uid . '.' . $keyId;
$home = $this->util->getUserHomeFolder($uid);
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 expect $home to be "root_dir . $home"

@PVince81
Copy link
Contributor

Let's close in favor of the alternative: #26820

Apps should not have to care about getHome, they work on the existing OC virtual filesystem where the user's home must be mounted. However the subtle thing is that even if we forgot to mount the user's home, if the user id matches the user id on disk, it magically works because the folder has the same name... That's how such issues sneak in.

@PVince81
Copy link
Contributor

👎

@DeepDiver1975 DeepDiver1975 deleted the respect-home-dir-in-encryption branch December 14, 2016 08:21
@lock
Copy link

lock bot commented Aug 4, 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 Aug 4, 2019
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.

4 participants