Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

stage1/kvm/lkvm: chown files and dirs on creation #3485

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

grahamwhaley
Copy link

Honor the uid/gid values when creating files and directories

Fixes #2576

As a side effect, it also seems to solve the remaining problems with
'apt-get update' in lkvm, so also:

Fixes #1917

Even though a uid/gid pair are passed over the protocol, they were
not being used for chmod'ing files or dirs being created. This
surfaced inside /tmp with the S_ISVTX 'sticky' bit set, where
all files ended up being owned by root.
Fix it by chmod'ing after creation.

Fixes rkt#2576

As a side effect, it also seems to solve the remaining problems with
'apt-get update' in lkvm, so also:

Fixes rkt#1917
@grahamwhaley
Copy link
Author

@lucab PTAL

@lucab
Copy link
Member

lucab commented Dec 19, 2016

LGTM. @grahamwhaley it looks like kvmtool recently received some 9p fixes. Any chance we can upstream this sticky-fix and perhaps re-sync with latest master?

@lucab
Copy link
Member

lucab commented Dec 19, 2016

(I restarted the jenkins build that got stuck, but should be just a flake)

@grahamwhaley
Copy link
Author

@lucab Yep, i saw lkvm had a few fixes (security as well iirc), and have on my list to bump the tag we currently clone from.
Yes, I'm looking at upstreaming the lkvm fixes.
And, I was going to ask about the Jenkins fails, as I think that happened on a a couple of PRs, and neither @jodh-intel nor myself could view the details afaict. Is that an internal thing, or do we need to go register for access etc.

@lucab
Copy link
Member

lucab commented Dec 19, 2016

@grahamwhaley I'll check how to provide you access to Jenkins in case of flakes. Just for the build logs, however, the public anonymous vhost should be enough: https://jenkins-rkt-public.prod.coreos.systems/job/rkt-github-ci/.

(The first URL in the list points there, but unfortunately it is not possible to fine-tweak Jenkins for each build-status entry)

@grahamwhaley
Copy link
Author

Ah. OK, some more history has come to light...
It looks like lkvm used to do the chown, and it was in fact removed from the code:
https://git.kernel.org/cgit/linux/kernel/git/will/kvmtool.git/commit/?id=ce780416361baa6f8b41843b4fec58f52d505b08
Now, the theory there has merit (if you are not root on the host then the chown will fail, and if you are root then maybe you should have concerns about id mappings between the host and the guest).

It looks like I won't be submitting this patch in this form at least upstream. I'll have a think about this...

@grahamwhaley
Copy link
Author

And, for more completeness, this is handled through QEMU using the local fsdev security mount options. Presently we mount with the 'none' security mode (https://github.com/coreos/rkt/blob/master/stage1/init/kvm/hypervisor/hvqemu/qemu_driver.go#L55), which allows the chown's in QEMU to execute, and complete. lkvm doesn't have any equivalent type of security configuration option.

@s-urbaniak
Copy link
Contributor

build is green, and the PR is reviewed. Tentatively setting to current milestone, /cc @lucab

@s-urbaniak s-urbaniak added this to the v1.22.0 milestone Jan 5, 2017
@lucab
Copy link
Member

lucab commented Jan 5, 2017

For rkt usecase, I think it is fine to ship with this patch. We always end up in the uid=0 case, and changing permissions on stage2-rootfs shouldn't be an issue (path is unreachable for normal users).
@grahamwhaley are you ok with shipping rkt-lkvm with this?

@lucab
Copy link
Member

lucab commented Jan 5, 2017

(Bad timing on our side, I'm deferring this to next release to have some room for discussion/followup).

@grahamwhaley
Copy link
Author

Having discussed with @lucab , I believe we are all happy with this functionality being re-introduced for rkt/kvm/lkvm, and we are pretty sure these patches will not go back upstream to lkvm. Given the slow movement of the lkvm codebase, the risk of carrying these patches in rkt causing us major rebase issues in the future is very low. Thus, I believe the agreement is we will merge this PR.

@lucab
Copy link
Member

lucab commented Jan 12, 2017

@grahamwhaley yes, thanks for summing this up. LGTM.

@lucab lucab merged commit 1e646cd into rkt:master Jan 12, 2017
@jjlakis
Copy link

jjlakis commented Jan 16, 2017

Big up @grahamwhaley

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