[stable9] Setupfs before access a users keys #26820

Merged
merged 7 commits into from Jan 20, 2017

Projects

None yet

4 participants

@DeepDiver1975
Member
DeepDiver1975 commented Dec 13, 2016 edited

alternative approach for #26818

@PVince81 @felixboehm

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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.

…he keys

@DeepDiver1975 DeepDiver1975 added this to the 9.0.7 milestone Dec 13, 2016
@PVince81
Collaborator

original issue confirmed with your hack and also LDAP + homeDirectory on stable9

@PVince81
Collaborator

This PR works for newly created shared files. Old files don't have the correct keys and can't be repaired without recreating or resharing (there is no other way).

I think setupFS and tearDown here is risky, because it removes all mount points and subsequent code might expect these to still exist. Let's try with initMountPoints instead.

@PVince81 PVince81 referenced this pull request Dec 13, 2016
Closed

Respect home folder setting in encryption #26818

3 of 9 tasks complete
lib/private/encryption/keys/storage.php
}
$path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid);
$key = $this->getKey($path);
if (!empty($uid) && $uid !== $currentUser) {
- \OC_Util::tearDownFS();
- \OC_Util::setupFS($currentUser);
+ \OC\Files\Filesystem::initMountPoints($uid);
@DeepDiver1975
DeepDiver1975 Dec 13, 2016 Member

$currentUser

@PVince81
PVince81 Dec 13, 2016 Collaborator

Hmm, really? The FS of the current user is usually already setup.
What we need is setup the FS for the target user $uid. Strangely this fix works for me.

@PVince81
PVince81 Dec 13, 2016 Collaborator

To put it into perspective: let's say "user1" is logged in, so usually setupFS("user1") was already called early on. But "user1" shares a file with "user2" (or creates a file in a shared folder), so we also need to mount the home folder of "user2" to have proper access to the keys and files.

I suspect that the mounting already happens later in the sharing code, but if this enc code runs earlier than the sharing code then we need to do it too.

@DeepDiver1975
DeepDiver1975 Dec 13, 2016 Member

wrong turn in my brain
I guess we don't need the second block - this was used for teardowns

@PVince81
PVince81 Dec 13, 2016 Collaborator

I'll also replace empty($uid), remember that empty("0") evaluates to true...

@PVince81 PVince81 referenced this pull request in owncloud/QA Dec 13, 2016
Open

LDAP + homeDirectory + encryption #337

@PVince81
Collaborator

@DeepDiver1975 I redid a test run with folder shares, file shares and also creating a file inside an already shared folder and this fix as per my modification does work. This is with LDAP and an alternative home folder.

@PVince81
Collaborator

Also retested with the hash folder hack, my fix makes it work too.

@PVince81 PVince81 modified the milestone: 9.0.8, 9.0.7 Dec 13, 2016
@PVince81
Collaborator

Fixed and retested with the md5 hack variant, still works.

Squash ?

@DeepDiver1975
Member

Yes. Squash, merge at will and port it all around :party:

@DeepDiver1975
Member

@PVince81 squashed - merge now or later? Your call.
Will prepare ports ...

@PVince81
Collaborator

Rebase to resurrect Jenkins then squash+merge now and keep the "backport-request" label

@DeepDiver1975
Member

πŸ‘»

Test\Encryption\Keys\StorageTest::testGetUserKey
OC\User\NoUserException: Backends provided no user object for user1

/var/lib/jenkins/workspace/owncloud-core_core_PR-26820-5ZSS3Y2GKGCIYIEECAW4OPVKEEWW4OIQOCEPDCFQAP5744QLCREA/lib/private/files/filesystem.php:391
/var/lib/jenkins/workspace/owncloud-core_core_PR-26820-5ZSS3Y2GKGCIYIEECAW4OPVKEEWW4OIQOCEPDCFQAP5744QLCREA/lib/private/encryption/keys/storage.php:75
/var/lib/jenkins/workspace/owncloud-core_core_PR-26820-5ZSS3Y2GKGCIYIEECAW4OPVKEEWW4OIQOCEPDCFQAP5744QLCREA/tests/lib/encryption/keys/storage.php:236
lib/private/encryption/keys/storage.php
@@ -70,6 +70,10 @@ public function __construct(View $view, Util $util) {
* @inheritdoc
*/
public function getUserKey($uid, $keyId, $encryptionModuleId) {
+ $currentUser = \OC_User::getUser();
+ if (!is_null($uid) && $uid !== '' && $uid !== $currentUser) {
+ \OC\Files\Filesystem::initMountPoints($uid);
@DeepDiver1975
DeepDiver1975 Dec 14, 2016 Member

@PVince81 I guess we should apply this patch to all other methods which call constructUserKeyPath with a user ....

@PVince81
PVince81 Dec 15, 2016 Collaborator

Indeed, makes sense

@PVince81
PVince81 Dec 20, 2016 Collaborator

I looked at the other code path within that class and it seems it would indeed be needed there.
It is not clear whether initMountPoints was already called from outside there, it's not a guarantee.
I'll add something in all the methods.

@PVince81
Collaborator
PVince81 commented Dec 16, 2016 edited
  • add it to other code paths as well
@PVince81
Collaborator

@DeepDiver1975 I've added the init mount points in the other code paths, please check it out: 4b602f6

A quick manual test with LDAP + homeDirectory shows that it still works.

Now I don't feel comfortable merging this without having automated tests... this would require to finish/solve #26586 and also introduce a new user backend that md5's the user home to simulate this situation. Maybe that backend could be part of the "testing" app ? Or do we make it an official feature configurable in config.php ?

@PVince81
Collaborator

For now I made this PR with the md5 hack to see if the current test coverage would catch the issue: #26844

@PVince81
Collaborator

Let's first get the automated tests sorted out, I want proper coverage for this: #26844

@felixboehm
Contributor

@PVince81 please provide this patch for 9.1

@PVince81
Collaborator
PVince81 commented Jan 9, 2017

@felixboehm here you go: #26824 (updated with the latest commit).
I retested it with the md5 hack and the fix works.

@PVince81
Collaborator

Looks like we still have some unit test failures

@PVince81
Collaborator

stable9.1: #26824
master: #26917

Automated tests will be added through this PR #26844 (comment).

@PVince81
Collaborator

Jenkins passed.

Since most of the code is now modified by me, need review from someone else please @jvillafanez @DeepDiver1975 @VicDeo @butonic

@jvillafanez
Contributor

Same comments as #26917

@PVince81
Collaborator

Cherry-picked the test from master and replace createMock with getMock.

@jvillafanez please re-review. Thanks

@PVince81
Collaborator

forgot to git add β˜•οΈ

NOW it should be ok

@jvillafanez
Contributor

πŸ‘

@PVince81
Collaborator

Let's wait before merging. Need to investigate this (not-so-critical-but-still) regression #26935

@PVince81
Collaborator

Yeah, syntax error in PHP 5.4.
Will adjust when rebasing later.

@PVince81
Collaborator
PVince81 commented Jan 13, 2017 edited
  • TODO: fix unit test syntax error
  • TODO: cherry-pick the regression fix: #26938
  • TODO: cherry-pick the other relevant fix about alternative home dir: #26937
@PVince81
Collaborator

Need rereview, I cherry-picked backports of master to fix a regression and incomplete cases.

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
a2aa8be
@DeepDiver1975 @PVince81 DeepDiver1975 Fix unit test execution 831224d
@PVince81 PVince81 Init mount points for user in more places in Keys\Storage eeef520
@PVince81 PVince81 Added test that check if user home is mounted after resolving key 5b15b11
@PVince81 PVince81 Don't mount user home with alternate keys root
When encryption keys are stored outside the user's homes, there is no
need to mount said homes.
0838619
@PVince81 PVince81 Ignore exception when deleting keys of deleted user
Whenever a user was deleted for encryption where the keys are stored in
the home, we can ignore user existence exceptions because it means the
keys are already gone.
2ed1bc7
@PVince81
Collaborator
PVince81 commented Jan 19, 2017 edited

Rebased and removed the "preDelete" commit and cherry-picked the better fix from #26968

After all these additions, let's retest this:

  • TEST: sharing with alt home works
  • TEST: delete user works with no warnings
  • TEST: delete user works with no warnings when using alternative key storage
@PVince81
Collaborator
  • backport the integration tests from #26844
@PVince81 PVince81 Fix encryption key storage test for older PHP
967f0bd
@PVince81
Collaborator

Integratin tests backport pending to be merged: #26985

@jvillafanez
Contributor

Changes look good πŸ‘

@PVince81 PVince81 merged commit 75c96fe into stable9 Jan 20, 2017

3 checks passed

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-stable9 branch Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment