Setupfs before access a users keys #26917

Merged
merged 5 commits into from Jan 13, 2017

Projects

None yet

5 participants

@PVince81
Collaborator

Forward port of #26824 to master.

I've retested with the md5 home hack and it works fine.

Automated test will appear when #26844 gets merged.
We should first merge this encryption fix before we can merge the tests PR to avoid failures.

Please review @DeepDiver1975 @jvillafanez @SergioBertolinSG

PVince81 and others added some commits Dec 13, 2016
@PVince81 PVince81 The file system of a user has to be properly setup before accessing t…
…he keys
7327fa9
@DeepDiver1975 @PVince81 DeepDiver1975 Fix unit test execution 730d6ae
@PVince81 PVince81 Init mount points for user in more places in Keys\Storage 8b669b8
@PVince81 PVince81 Fix encryption key storage tests to properly create required users
1e1c11f
@PVince81 PVince81 added this to the 10.0 milestone Jan 11, 2017
@mention-bot

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

@PVince81 PVince81 referenced this pull request Jan 11, 2017
Merged

[stable9] Setupfs before access a users keys #26820

3 of 9 tasks complete
@PVince81 PVince81 requested review from butonic and DeepDiver1975 Jan 11, 2017
@PVince81
Collaborator

Repasting the steps here because it's a long way to find them:

Steps

  1. Setup LDAP with special home folder rule, where home folder name is different than the user id
  2. Enable encryption
  3. Login as a LDAP user zombie2 (to initialize encryption keys)
  4. Login as a LDAP user zombie1
  5. Create a folder "test"
  6. Upload a file "bacon.txt" into "test"
  7. Share "test" with "zombie2"
  8. Login as "zombie2"
  9. Open/download file "test/bacon.txt"

Before this fix: public key exception
After this fix: file can be read

lib/private/Encryption/Keys/Storage.php
@@ -64,6 +67,11 @@ public function __construct(View $view, Util $util) {
$this->encryption_base_dir = '/files_encryption';
$this->keys_base_dir = $this->encryption_base_dir .'/keys';
$this->root_dir = $this->util->getKeyStorageRoot();
+
+ $session = \OC::$server->getUserSession();
@jvillafanez
jvillafanez Jan 12, 2017 Contributor

I guess it would be a pain to inject this.

@PVince81
PVince81 Jan 12, 2017 Collaborator

Good catch, apparently not. This class is only in one place in server.php, so I'll add it.

+ *
+ * @param string $uid user id
+ */
+ protected function setupUserMounts($uid) {
@jvillafanez
jvillafanez Jan 12, 2017 Contributor

Maybe this function should return something to check the result of the initMountPoint (if any) or to check if the function is ignoring the request because the uid matches the current user or the mount point is already mounted, specially if there are plans to unittest this function.

Since it isn't public we might skip testing this.

@@ -55,6 +67,8 @@ public function setUp() {
->disableOriginalConstructor()
->getMock();
+ $this->createUser('user1', '123456');
@jvillafanez
jvillafanez Jan 12, 2017 Contributor

I think this will create a user each time a test runs, which seems wrong.

Either you create the users in the setUpBeforeClass method (and maybe delete them in the tearDownAfterClass method) or you delete them in the tearDown method so the next tests recreate them without any problem

@PVince81
PVince81 Jan 12, 2017 Collaborator

Good catch, I forgot to re-delete the user. Will do.

@PVince81
PVince81 Jan 12, 2017 Collaborator

I checked the code. Apparently createUser comes from UserTrait which itself uses the Dummy user backend which is reset after every test. So no need for additional changes here.

@SergioBertolinSG
Member

Works using detailed steps from #26917 (comment)
👍

Used displayName as special home folder rule.

@PVince81
Collaborator

Setting back to "Developing". I'll address @jvillafanez's relevant comments.

@PVince81 PVince81 self-assigned this Jan 12, 2017
@PVince81 PVince81 Added test that check if user home is mounted after resolving key
34461cf
@PVince81
Collaborator

@jvillafanez adjusted, please recheck.

I only indirectly tested the setupUserMounts, I had to do a big detour to find out whether the mounts are setup without triggering the setup through the check. (MountManager->getAll does that)

@jvillafanez
Contributor

👍

@PVince81
Collaborator
PVince81 commented Jan 13, 2017 edited
  • TODO: backport the additional unit test to the other branches (yay, snakeport!)
@PVince81 PVince81 merged commit a4883ae into master Jan 13, 2017

4 checks passed

Scrutinizer 1 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@PVince81 PVince81 deleted the setupfs-before-access-a-users-keys branch Jan 13, 2017
@PVince81
Collaborator

First regression already... #26935

@PVince81 PVince81 referenced this pull request Jan 13, 2017
Merged

Don't mount user home with alternate keys root #26937

5 of 9 tasks complete
@PVince81
Collaborator

First missing piece to properly deal with "alternative keys storage root" in which case we don't need to mount anything, see #26937

@PVince81 PVince81 referenced this pull request Jan 13, 2017
Merged

Use preDelete hook to delete encryption keys #26938

5 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment