Restore creation of $SNAP_USER_DATA #97

Merged
merged 6 commits into from Aug 11, 2016

Conversation

Projects
None yet
3 participants
Collaborator

zyga commented Aug 11, 2016

This patch restores creation of SNAP_USER_DATA directory. This was a regression of functionality that occurred because the new code path in snapd that does the same thing is not active yet (it depends on snap-exec).

The fix comes with spread tests that should reliably work even when snap-exec is in use.

Fixes: https://bugs.launchpad.net/snap-confine/+bug/1612120

zyga added some commits Aug 10, 2016

Fix regression test to always remove test snap
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Unconditionally install snapd-hacker-toolbelt
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Fix broken test due to subtle yaml syntax issue
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Add spread test for user directory creation
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
+
+ if (user_data == NULL)
+ return;
+ // Only support absolute paths.
@mvo5

mvo5 Aug 11, 2016

Contributor

I was initially wondering if we need extra validation here, but I think we don't because privs have already dropped when mkdir is called.

@mvo5

mvo5 Aug 11, 2016

Contributor

We might check for ".." in the path though and disallow that.

@zyga

zyga Aug 11, 2016

Collaborator

We might though snap-confine is already confined with apparmor so (at least where apparmor is used) this is not critical. I will land this and follow up with a small patch to check for ... Thanks for spotting this.

Contributor

mvo5 commented Aug 11, 2016

Looks good, thanks for the tests!

Contributor

mvo5 commented Aug 11, 2016

Sorry, but this appears to cause a regression with the apparmor rules on ecryptfs.

Collaborator

zyga commented Aug 11, 2016

I will try to come up with a one-time fix by testing locally with encrypted home directory but we should investigate how to support that in spread.

Allow snap-confine to access ecryptfs of any user
This patch fixes a subtle issue when a snap is ran on encrypted home
filesystem with sudo. For more details please consult the bug report
below.

Fixes: https://bugs.launchpad.net/snap-confine/+bug/1612291
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

mvo5 commented Aug 11, 2016

👍 looks good, will test in a bit

@zyga zyga merged commit ba8ab6a into master Aug 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zyga zyga deleted the restore-user-dir-creation branch Aug 12, 2016

Contributor

jdstrand commented Sep 20, 2016

I am not a fan of this change because I think it is papering over the larger problem of using sudo with snap commands and the fact that it is creating directories with root:root permissions in the user's home directory. This feels very wrong and causes lots of pain for users when they use the command under sudo and it later fails when not running under sudo.

That said, the architects have not taken a stance on this issue yet and the current implementation dictates that it should work, so these rules are required.

+0 (as in, I won't block this commit, but I do not approve of the changes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment