Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: add support for /home on NFS #3958
Conversation
zyga
added some commits
Sep 20, 2017
jdstrand
commented on interfaces/apparmor/backend.go in 754d855
Sep 21, 2017
|
What about reexec? |
jdstrand
commented on interfaces/apparmor/backend.go in 754d855
Sep 21, 2017
|
There is also 'nfs'. I wonder if we should include 'cifs' and 'smbfs'.... |
zyga
added some commits
Sep 22, 2017
All checks have passed
All checks have passed
|
@jdstrand I've updated this based on your earlier review on IRC. This includes changes to:
In addition to this I added full unit test coverage and I'll re-create the spread test I lost with my snapd checkout last week. |
codecov-io
commented
Sep 25, 2017
•
Edited 1 time
-
codecov-io
Sep 25, 2017
-
Sep 25, 2017
Codecov Report
@@ Coverage Diff @@
## master #3958 +/- ##
=========================================
+ Coverage 75.8% 75.8% +<.01%
=========================================
Files 433 433
Lines 37174 37287 +113
=========================================
+ Hits 28180 28266 +86
- Misses 7022 7046 +24
- Partials 1972 1975 +3
Continue to review full report at Codecov.
|
zyga
added this to the 2.29 milestone
Sep 25, 2017
|
Ohhh, it's green on first push :-) Must be my lucky day |
zyga
added some commits
Sep 28, 2017
All checks have failed
All checks have passed
| + policy := make(map[string]*osutil.FileState) | ||
| + | ||
| + // Create the local policy directory if it is not there. | ||
| + if err := os.MkdirAll(dir, 0755); err != nil { |
stolowski
Oct 3, 2017
•
Edited 1 time
-
stolowski
Oct 3, 2017
Contributor
-
Oct 3, 2017
I'd use dirs.SnapConfineAppArmorDir directly, no need for an intermediate temp variable in a simple case like this.
| + // Check if NFS is mounted at or under $HOME. Because NFS is not | ||
| + // transparent to apparmor we must alter our profile to counter that and | ||
| + // allow snap-confine to work. | ||
| + if nfs, err := isHomeUsingNFS(); err != nil { |
stolowski
Oct 3, 2017
Contributor
I wonder if an inability to determine if we are using NFS here should be fatal (it's fatal in this code if I'm not mistaken), unless we've full faith in our fstab/mountinfo parsing (which we probably need to have given that it's used in some other places afair). Anyway, just mentioning the concern since NFS is not a typical use case - perhaps a warning would be enough. For your consideration.
zyga
Oct 3, 2017
Contributor
Interesting question. While we don't expect any errors I agree we should stop the world about it. It is just a workaround for a missing kernel feature (NFS transparency in LSMs). I'll make this non fatal.
| + return false, fmt.Errorf("cannot parse /proc/self/mountinfo, %s", err) | ||
| + } | ||
| + for _, entry := range mountinfo { | ||
| + if (entry.FsType == "nfs4" || entry.FsType == "nfs") && (strings.HasPrefix(entry.MountDir, "/home/") || entry.MountDir == "/home") { |
stolowski
Oct 3, 2017
Contributor
I know we've a deeper and historical problem with hardcoded /home, but I'm not super happy about adding another hardcoded case here. Actually, if we ever fix the problem with hardcoded /home in dirs.go, this fix will break completely and this makes me a sad panda...
Would it be possible to make it more generic and future-proof (e.g. check if any of the filesystems that the snap may potentially be interested in is NFS)? Don't we get the same problem if - say - /usr or /var is NFS-mounted?
zyga
Oct 3, 2017
Contributor
This was added by a request from @jdstrand - the idea is that we want to slowly open this up to more use cases. For now we just want to support /home on NFS. We plan to expand this to CIFS as well but not in arbitrary places yet.
Note that /home is already explicitly described in apparmor profiles and there's no easy way around it today.
jdstrand
Oct 5, 2017
Contributor
To check for home or not is a difficult question. If we consider that we are lessening the security of the system if NFS is detected, then I think pursuing only doing it when needed is worthwhile. For example, if there is an NFS mount in /srv for something, snaps don't have access to that anyway, so adding network rules to everything snappy for something no snaps will access is wrong. Determining exactly when to apply NFS rules might take some effort, but I think if we are thoughtful and open to iterating, we can do it without too much complexity.
Note that /home is not actually hard-coded in AppArmor policy as included in snappy for app snaps, it is defined via an apparmor tunable which the user can adjust and the apparmor policy for app snaps will be ok.
We do hardcode /home in snap-confine code and apparmor policy though. Until this is resolved, there is no requirement to adjust this PR. If you wanted to make this PR dynamic today, you could do the equivalent of getent passwd, looking at every uid >= 1000, collecting the dirname's of the found homes and then iterate through those to check if on NFS. Of course, this wouldn't catch someone who has a symlink from /home/foo to /var/exports/homes/foo, so could readlink() first. Alternatively, perhaps in the future the core snap can gain 'snap set' config for adding additional locations for HOME (which would update the apparmor tunables). If so, this code could be updated to check /home and any additional directories that were set with snap set.
| @@ -0,0 +1,96 @@ | ||
| +summary: Test that snaps still work when /home is a NFS mount |
zyga
Oct 3, 2017
Contributor
As things go, this test obviously caught a bug in the initial code that worked fine for /home sub-directories but not the full thing :)
zyga
added some commits
Oct 3, 2017
All checks have passed
zyga
requested review from
jdstrand and
chipaca
Oct 5, 2017
stolowski
approved these changes
Oct 5, 2017
Looks good to me. Thanks for making the changes and for extensive tests!
| + // Check if NFS is mounted at or under $HOME. Because NFS is not | ||
| + // transparent to apparmor we must alter our profile to counter that and | ||
| + // allow snap-confine to work. | ||
| + if nfs, err := isHomeUsingNFS(); err != nil { |
stolowski
Oct 3, 2017
Contributor
I wonder if an inability to determine if we are using NFS here should be fatal (it's fatal in this code if I'm not mistaken), unless we've full faith in our fstab/mountinfo parsing (which we probably need to have given that it's used in some other places afair). Anyway, just mentioning the concern since NFS is not a typical use case - perhaps a warning would be enough. For your consideration.
zyga
Oct 3, 2017
Contributor
Interesting question. While we don't expect any errors I agree we should stop the world about it. It is just a workaround for a missing kernel feature (NFS transparency in LSMs). I'll make this non fatal.
| + return false, fmt.Errorf("cannot parse /proc/self/mountinfo, %s", err) | ||
| + } | ||
| + for _, entry := range mountinfo { | ||
| + if (entry.FsType == "nfs4" || entry.FsType == "nfs") && (strings.HasPrefix(entry.MountDir, "/home/") || entry.MountDir == "/home") { |
stolowski
Oct 3, 2017
Contributor
I know we've a deeper and historical problem with hardcoded /home, but I'm not super happy about adding another hardcoded case here. Actually, if we ever fix the problem with hardcoded /home in dirs.go, this fix will break completely and this makes me a sad panda...
Would it be possible to make it more generic and future-proof (e.g. check if any of the filesystems that the snap may potentially be interested in is NFS)? Don't we get the same problem if - say - /usr or /var is NFS-mounted?
zyga
Oct 3, 2017
Contributor
This was added by a request from @jdstrand - the idea is that we want to slowly open this up to more use cases. For now we just want to support /home on NFS. We plan to expand this to CIFS as well but not in arbitrary places yet.
Note that /home is already explicitly described in apparmor profiles and there's no easy way around it today.
jdstrand
Oct 5, 2017
Contributor
To check for home or not is a difficult question. If we consider that we are lessening the security of the system if NFS is detected, then I think pursuing only doing it when needed is worthwhile. For example, if there is an NFS mount in /srv for something, snaps don't have access to that anyway, so adding network rules to everything snappy for something no snaps will access is wrong. Determining exactly when to apply NFS rules might take some effort, but I think if we are thoughtful and open to iterating, we can do it without too much complexity.
Note that /home is not actually hard-coded in AppArmor policy as included in snappy for app snaps, it is defined via an apparmor tunable which the user can adjust and the apparmor policy for app snaps will be ok.
We do hardcode /home in snap-confine code and apparmor policy though. Until this is resolved, there is no requirement to adjust this PR. If you wanted to make this PR dynamic today, you could do the equivalent of getent passwd, looking at every uid >= 1000, collecting the dirname's of the found homes and then iterate through those to check if on NFS. Of course, this wouldn't catch someone who has a symlink from /home/foo to /var/exports/homes/foo, so could readlink() first. Alternatively, perhaps in the future the core snap can gain 'snap set' config for adding additional locations for HOME (which would update the apparmor tunables). If so, this code could be updated to check /home and any additional directories that were set with snap set.
chipaca
approved these changes
Oct 5, 2017
Thank you for this.
Could you add a test using proto=udp?
|
@chipaca oh, great idea. I'll do that. |
jdstrand
requested changes
Oct 5, 2017
This is looking really good. A few questions, comments, nit-picks inline.
| +// Initialize prepares customized apparmor policy for snap-confine. | ||
| +func (b *Backend) Initialize() error { | ||
| + // TODO: also generate policy for snap-confine from core. | ||
| + return setupSnapConfineGeneratedPolicy() |
jdstrand
Oct 5, 2017
Contributor
Based on a comment further down below, I think this comment can be removed?
zyga
Oct 5, 2017
Contributor
I was about to say "but I really want to do that eventually" and then I realized it needs to be the way it is for now. I'll change this comment to explain why.
| + | ||
| + // If snapd is executing from the core snap the it means it has | ||
| + // re-executed. In that case we are no longer using the copy of | ||
| + // snap-confined from the host distribution but our own copy. We don't have |
| + // setupSnapConfineReexec below. | ||
| + exe, err := os.Readlink(procSelfExe) | ||
| + if err != nil { | ||
| + return fmt.Errorf("cannot read /proc/self/exe, %s", err) |
jdstrand
Oct 5, 2017
Contributor
You are using procSelfExe in Readlink() but hardcoding /proc/self/exe in Errorf.
| + // load any of the files placed in /var/lib/snapd/apparmor/snap-confine.d/. | ||
| + // We are not using apparmor.LoadProfile() because it uses other cache. | ||
| + if output, err := exec.Command("apparmor_parser", "--replace", | ||
| + // TODO: the name varies across distros, fix that :/ |
| + if output, err := exec.Command("apparmor_parser", "--replace", | ||
| + // TODO: the name varies across distros, fix that :/ | ||
| + "--write-cache", filepath.Join(dirs.SystemApparmorDir, "usr.lib.snapd.snap-confine.real"), | ||
| + "--cache-loc", dirs.SystemApparmorCacheDir).CombinedOutput(); err != nil { |
jdstrand
Oct 5, 2017
Contributor
While not as important here since we shouldn't be reloading this policy all the time, I think you want '"-O", "no-expr-simplify"'. See LoadProfile() in interfaces/apparmor/apparmor.go for details.
| + return false, fmt.Errorf("cannot parse /proc/self/mountinfo, %s", err) | ||
| + } | ||
| + for _, entry := range mountinfo { | ||
| + if (entry.FsType == "nfs4" || entry.FsType == "nfs") && (strings.HasPrefix(entry.MountDir, "/home/") || entry.MountDir == "/home") { |
stolowski
Oct 3, 2017
Contributor
I know we've a deeper and historical problem with hardcoded /home, but I'm not super happy about adding another hardcoded case here. Actually, if we ever fix the problem with hardcoded /home in dirs.go, this fix will break completely and this makes me a sad panda...
Would it be possible to make it more generic and future-proof (e.g. check if any of the filesystems that the snap may potentially be interested in is NFS)? Don't we get the same problem if - say - /usr or /var is NFS-mounted?
zyga
Oct 3, 2017
Contributor
This was added by a request from @jdstrand - the idea is that we want to slowly open this up to more use cases. For now we just want to support /home on NFS. We plan to expand this to CIFS as well but not in arbitrary places yet.
Note that /home is already explicitly described in apparmor profiles and there's no easy way around it today.
jdstrand
Oct 5, 2017
Contributor
To check for home or not is a difficult question. If we consider that we are lessening the security of the system if NFS is detected, then I think pursuing only doing it when needed is worthwhile. For example, if there is an NFS mount in /srv for something, snaps don't have access to that anyway, so adding network rules to everything snappy for something no snaps will access is wrong. Determining exactly when to apply NFS rules might take some effort, but I think if we are thoughtful and open to iterating, we can do it without too much complexity.
Note that /home is not actually hard-coded in AppArmor policy as included in snappy for app snaps, it is defined via an apparmor tunable which the user can adjust and the apparmor policy for app snaps will be ok.
We do hardcode /home in snap-confine code and apparmor policy though. Until this is resolved, there is no requirement to adjust this PR. If you wanted to make this PR dynamic today, you could do the equivalent of getent passwd, looking at every uid >= 1000, collecting the dirname's of the found homes and then iterate through those to check if on NFS. Of course, this wouldn't catch someone who has a symlink from /home/foo to /var/exports/homes/foo, so could readlink() first. Alternatively, perhaps in the future the core snap can gain 'snap set' config for adding additional locations for HOME (which would update the apparmor tunables). If so, this code could be updated to check /home and any additional directories that were set with snap set.
| + } | ||
| + fstab, err := mount.LoadProfile(etcFstab) | ||
| + if err != nil { | ||
| + return false, fmt.Errorf("cannot parse /etc/fstab, %s", err) |
jdstrand
Oct 5, 2017
Contributor
You are specifying etcFstab to mount.LoadProfile() but hardcoding /etc/fstab in Errorf
| @@ -254,6 +361,9 @@ func addContent(securityTag string, snapInfo *snap.Info, opts interfaces.Confine | ||
| // the super-broad template we are starting with. | ||
| } else { | ||
| tagSnippets = snippetForTag | ||
| + if nfs, _ := isHomeUsingNFS(); nfs { |
jdstrand
Oct 5, 2017
Contributor
Can you add a comment here like you did above:
// Check if NFS is mounted at or under $HOME. Because NFS is not
// transparent to apparmor we must alter the profile to counter that and
// allow access to SNAP_USER_* files.
| + { | ||
| + fstab: "localhost:/srv/nfs /home/zyga/nfs nfs defaults 0 0", | ||
| + nfs: true, | ||
| + }, |
jdstrand
Oct 5, 2017
Contributor
fstab entry has 'nfs', but comment is NFSv4 here and the test below. I like these tests, we probably want tests for nfs4 and (at least spot checks for) nfs.
zyga
Oct 5, 2017
Contributor
This is how it works actually. You get NFSv4 by default but you can still specify NFSv3 by using nfsvers=3. I'll expand tests to include NFSv3 as well.
| + // Make it appear as if NFS was mounted under /home. | ||
| + restore := apparmor.MockMountInfo("680 27 0:59 / /home/zyga/nfs rw,relatime shared:478 - nfs4 localhost:/srv/nfs rw,vers=4.2,rsize=524288,wsize=524288,namlen=255,hard,proto=tcp,port=0,timeo=600,retrans=2,sec=sys,clientaddr=127.0.0.1,local_lock=none,addr=127.0.0.1") | ||
| + defer restore() | ||
| + restore = apparmor.MockEtcFstab("") |
jdstrand
Oct 5, 2017
Contributor
I noticed that we have a lot of tests using MockMountInfo("...") when using MockEtcFstab(""), but not the other way around or when both are set (outside of TestIsHomeUsingNFS()). Was this intentional?
zyga
Oct 5, 2017
Contributor
I wrote mountinfo support code first and only then added the trigger code that looked for fstab. Since other tests are checking that they have equivalent power I should perhaps just say "mock stuff so that NFS workaround is enabled" without going into the details of how that is done.
| + // We created the policy file. | ||
| + files, err := ioutil.ReadDir(dirs.SnapConfineAppArmorDir) | ||
| + c.Assert(err, IsNil) | ||
| + c.Assert(files, HasLen, 1) |
jdstrand
Oct 5, 2017
Contributor
If we can't read /proc/self/exe, why would we create the policy file? We want to create the directory but I see no reason why we would fallback to creating the file, even if we don't reload, since something outside of snapd might reload the policy and then have the NFS policy included.
zyga
Oct 5, 2017
Contributor
Because the policy is equally applicable to the re-exec case. We generate the local policy file but don't reload the distro profile. When Setup is called it will choose (or not) to reload the core profile. If it chooses to reload it at that time the generated include file will be present and we will benefit from it.
In all cases I think that if we cannot examine /proc/self/exe we will have other trouble elsewhere so I think this is good as is.
jdstrand
Oct 5, 2017
•
Edited 1 time
-
jdstrand
Oct 5, 2017
Contributor
-
Oct 5, 2017
I understand with the way you wrote setupSnapConfineGeneratedPolicyImpl() why you wrote the test this way, but my point is to fail closed rather than open (and reorder the read on procSelfExe so we don't generate the policy). The idea is if there is a crazy kernel bug on reading /proc/self/exe, we fail early and loud rather than try to plod along.
I also understand your point that being able to read /proc/self/exe or not doesn't really have anything to do with isHomeUsingNFS(), which is why you wrote out the policy. I'll leave the decision up to you (ie, not a blocker).
| + | ||
| + // Setup generated policy for snap-confine. | ||
| + err = apparmor.SetupSnapConfineGeneratedPolicy() | ||
| + c.Assert(err, ErrorMatches, "cannot reload snap-confine apparmor profile: testing") |
jdstrand
Oct 5, 2017
•
Edited 1 time
-
jdstrand
Oct 5, 2017
Contributor
-
Oct 5, 2017
If the policy load fails, it seems to make sense to clean up the generated profile?
zyga
Oct 5, 2017
Contributor
Hmmm, I think the re-exec-from-core case may still want that but if we cannot load it ourselves then yeah, lets remove it. I'll make the adjustment.
| +// to operate when NFS is used. This is an imperfect solution as this grants | ||
| +// some network access to all the snaps on the system. | ||
| +var nfsSnippet = ` | ||
| + # Workaround for systems using NFS, for details see: |
| + Snapd now contains a feature where NFS-mounted /home (or any sub-directory) | ||
| + initializes a workaround mode where all snaps gain minimal amount of network | ||
| + permissions sufficient for NFS to operate. | ||
| +systems: [ubuntu-16.04-64] # TODO: expand this list |
zyga
Oct 5, 2017
Contributor
No, it's a lot of hairy stuff already. I'll address this with separate PRs.
| +execute: | | ||
| + run_export_nfs() { | ||
| + # NOTE: This is so silly because spellchecker complains about the real | ||
| + # command name so w have to go and obfuscate it /o\ |
zyga
added some commits
Oct 5, 2017
Some checks were not successful
Some checks were not successful
|
@jdstrand I've addressed or commented on all the items you have highlighted except for the |
Some checks were not successful
|
@jdstrand I believe everything is changed or explained (in one case) |
|
Approved (though see my last comment (not a blocker)). |
All checks have passed
|
@jdstrand after thinking about it I'll rework the code slightly. |
zyga
added some commits
Oct 16, 2017
Some checks were not successful
All checks have passed
niemeyer
requested changes
Oct 19, 2017
Nice, thanks for working on this.
Some simple, and probably last, notes.
| @@ -176,9 +176,9 @@ func SetRootDir(rootdir string) { | ||
| SnapDataDir = filepath.Join(rootdir, "/var/snap") | ||
| SnapDataHomeGlob = filepath.Join(rootdir, "/home/*/snap/") | ||
| SnapAppArmorDir = filepath.Join(rootdir, snappyDir, "apparmor", "profiles") | ||
| + SnapConfineAppArmorDir = filepath.Join(rootdir, snappyDir, "apparmor", "snap-confine.d") |
niemeyer
Oct 19, 2017
Contributor
Can we please fix the name for this directory to "snap-confine" while we have time (no .d)? That convention doesn't make much sense here.
| + return setupSnapConfineGeneratedPolicy() | ||
| +} | ||
| + | ||
| +// setupSnapConfineGeneratedPolicyImpl inspects the system and sets up local |
niemeyer
Oct 19, 2017
Contributor
That's three levels of indirection, and looking through the code it doesn't look like we need any of it. Can we please have the logic below under the real Initialize, and use the real initialize from tests?
| +func setupSnapConfineGeneratedPolicyImpl() error { | ||
| + // Create the local policy directory if it is not there. | ||
| + if err := os.MkdirAll(dirs.SnapConfineAppArmorDir, 0755); err != nil { | ||
| + return fmt.Errorf("cannot create snap-confine policy directory, %s", err) |
niemeyer
Oct 19, 2017
Contributor
s/,/:/. Every error and log message in this PR seems to use this convention, and this is not our usual convention. Can you please review all error messages and make sure they follow suit?
As a more general note, let's please be careful to preserve consistency in the code base. I've seen that sort of minor discrepancy often, and if it goes unnoticed the code gradually becomes messy. We can change conventions, but we cannot change them in isolation and without notice.
zyga
Oct 20, 2017
Contributor
I was wondering why I got this wrong. I must have got the impression that the cannot quux: %s is the wrong way and cannot quux, %s is the right way of formatting errors somehow (as I tried to be consistent, just going the wrong way).
I've reviewed and corrected all the error messages around this code.
| + } | ||
| + | ||
| + // Check the /proc/self/exe symlink, this is needed below but we want to | ||
| + // fail early if this fails for whatever reason. |
niemeyer
Oct 19, 2017
Contributor
Thanks for making that clear. That sort of comment helps much when someone else goes to refactor the logic.
| +} | ||
| + | ||
| +var ( | ||
| + procSelfMountInfo = mount.ProcSelfMountInfo |
| + procSelfMountInfo = mount.ProcSelfMountInfo | ||
| +) | ||
| + | ||
| +// isHomeUsingNFS returns true if /home contains or might contain NFS mounts. |
niemeyer
Oct 19, 2017
Contributor
The "might contain" is a truism and would be nice to clarify, in the sense that /etc/fstab might contain NFS entries at all times. Clearly there's something more specific implied there since it mentions both "contains" and "might contain", though.
zyga
Oct 20, 2017
•
Edited 1 time
-
zyga
Oct 20, 2017
Contributor
-
Oct 20, 2017
Re-worded to
// isHomeUsingNFS returns true if NFS mounts are defined or mounted under /home.
| @@ -121,7 +260,7 @@ func setupSnapConfineReexec(snapInfo *snap.Info) error { | ||
| // create for policy extensions for snap-confine. This is required for the | ||
| // profiles to compile but distribution package may not yet contain this | ||
| // directory. | ||
| - if err := os.MkdirAll(dirs.SnapAppArmorConfineDir, 0755); err != nil { | ||
| + if err := os.MkdirAll(dirs.SnapConfineAppArmorDir, 0755); err != nil { |
| + fstab: "localhost:/srv/nfs /home nfs4 defaults 0 0", | ||
| + nfs: true, | ||
| + }, | ||
| + { |
niemeyer
Oct 19, 2017
Contributor
We tend to unify those lines elsewhere, as in }, {. The extra spacing doesn't buy us much. The comments can go inside the brackets instead of outside, which even improves the readability as it's closer to the data.
| + old := procSelfMountInfo | ||
| + f, err := ioutil.TempFile("", "mountinfo") | ||
| + if err != nil { | ||
| + panic(fmt.Errorf("cannot open temporary file %s", err)) |
| + panic(fmt.Errorf("cannot open temporary file %s", err)) | ||
| + } | ||
| + if err := ioutil.WriteFile(f.Name(), []byte(text), 0644); err != nil { | ||
| + panic(fmt.Errorf("cannot write mock mountinfo file %s", err)) |
| + | ||
| +// MockEtcFstab mocks content of /etc/fstab read by isHomeUsingNFS | ||
| +func MockEtcFstab(text string) (restore func()) { | ||
| + old := procSelfMountInfo |
| + old := procSelfMountInfo | ||
| + f, err := ioutil.TempFile("", "fstab") | ||
| + if err != nil { | ||
| + panic(fmt.Errorf("cannot open temporary file %s", err)) |
| + panic(fmt.Errorf("cannot open temporary file %s", err)) | ||
| + } | ||
| + if err := ioutil.WriteFile(f.Name(), []byte(text), 0644); err != nil { | ||
| + panic(fmt.Errorf("cannot write mock fstab file %s", err)) |
| + } | ||
| + etcFstab = f.Name() | ||
| + return func() { | ||
| + os.Remove(etcFstab) |
niemeyer
Oct 19, 2017
Contributor
We need a check here that panics if this is /etc/fstab. We don't want to kill the system by mistake.
| + | ||
| +// nfsSnippet contains extra permissions necessary for snaps and snap-confine | ||
| +// to operate when NFS is used. This is an imperfect solution as this grants | ||
| +// some network access to all the snaps on the system. |
niemeyer
Oct 19, 2017
Contributor
I'm honestly quite surprised that the application needs network access to access the filesytem. I won't question that as we'd not have gotten that far if that wasn't true, but I'd like to hear more about how this ended up like this. It seems to make no sense to me at least.
zyga
Oct 19, 2017
•
Edited 1 time
-
zyga
Oct 19, 2017
Contributor
-
Oct 19, 2017
I'll find the relevant apparmor bug and link it here. EDIT. After some discussion with jj there's a new bug report: https://bugs.launchpad.net/apparmor/+bug/1724903
| @@ -74,13 +74,19 @@ func (m *InterfaceManager) addInterfaces(extra []interfaces.Interface) error { | ||
| return nil | ||
| } | ||
| -func (m *InterfaceManager) addBackends(extra []interfaces.SecurityBackend) error { | ||
| +func (m *InterfaceManager) addAndInitBackends(extra []interfaces.SecurityBackend) error { |
niemeyer
Oct 19, 2017
Contributor
It's actually init and then add, not the opposite, but instead of fixing that let's revert and preserve the original function name here as it's symmetrical with other functions around it. It will do whatever it needs to do for adding the backends. We don't need to call out every operation in its name.
| + | ||
| + # Install NFS server and a simple shell snap. | ||
| + distro_install_package nfs-kernel-server | ||
| + distro_install_package bsdgames |
niemeyer
Oct 19, 2017
Contributor
The package installations should happen at the global location next to every other package installation, and it doesn't have to be purged at restore time. Please coordinate with Sergio so that these packages also go into the image maintenance spread runs so that once we finally manage to cook packages into the images this will be there too.
| +execute: | | ||
| + run_export_nfs() { | ||
| + # NOTE: This is so silly because spellchecker complains about the real | ||
| + # command name so we have to go and obfuscate it /o\ |
niemeyer
Oct 19, 2017
Contributor
Ouch. If we end up with more of those, we may as well drop spellchecker.
zyga
Oct 20, 2017
Contributor
It would be good to be able to teach the spellchecker about new words. I'll see if I can do that instead of this hack.
| + ensure_normal_perms | ||
| + | ||
| + # Back up the /etc/fstab file and define a NFS mount mount there. | ||
| + cp /etc/fstab fstab.bak |
| + # Unmount NFS mount over /home if one exists. | ||
| + umount /home || true | ||
| + | ||
| + # Restore the fstab backup file if one exits. |
| + | ||
| + # Restore the fstab backup file if one exits. | ||
| + if [ -e fstab.bak ]; then | ||
| + mv fstab.bak /etc/fstab |
zyga
added some commits
Oct 20, 2017
|
With the exception of unified location of package installation this is all done now. I've also renamed the generated file to |
zyga
added some commits
Oct 20, 2017
zyga commentedSep 25, 2017
This branch adds support for systems running on hosts with NFS-shared /home (or
fragment of /home).
The main problem is that NFS is not transparent to apparmor. When a process
attempts to access NFS-mounted files or directories it will be subject to
network mediation. In the future the kernel may understand what is going on
better and no longer require any workarounds but for now those are necessary.
With this branch, on startup only, snapd will interrogate mounted filesystems
and if NFS is found it will insert additional apparmor snippet into every
apparmor profile. This includes snap application profiles as well as the
profile of snap-confine itself.
The additional profile rules allow "network inet" and "network inet6" access.
For now we are unable to detect NFS filesystems mounted "in flight" at runtime
but we do analyze /etc/fstab and if statically declared NFS filesystem is found
there we assume it may be mounted in the future, even if it is not necessarily
mounted at the given time.
Fixes: https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1662552
Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com