New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

interfaces: add cifs-mount interface #5340

Merged
merged 10 commits into from Aug 22, 2018

Conversation

Projects
None yet
6 participants
@datenhahn
Contributor

datenhahn commented Jun 18, 2018

https://forum.snapcraft.io/t/interface-mount-umount-cifs-share-permission/5689

  • an interface which allows mounting and unmounting of cifs shares

  • currently not all permissions needed for mount/umount are included,
    it requires snaps to request the mount-observe interface as well

cifs-mount-control interface (discussed in snapcraft.io forum post 5689)
https://forum.snapcraft.io/t/interface-mount-umount-cifs-share-permission/5689

- an interface which allows mounting and unmounting of cifs shares
- currently not all permissions needed for mount/umount are included,
  it requires snaps to request the mount-observe interface as well

  - created development environment description and some basic configs/tools to
  test and reproduce my usecase here : https://github.com/datenhahn/devtools-cifs-mount-control
@zyga

Some initial feedback. I think this one needs to wait till next week for a security review.

umount fstype=cifs /home/*/snap/@{SNAP_NAME}/common/{,**/},
umount fstype=cifs /var/snap/@{SNAP_NAME}/common/{,**/},
/run/mount/utab rw,

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

Not sure if we want this since the filesystem is not really visibly mounted from the outside.

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

Without the /run/mount/utab permission the mount (or umount?) command did fail. It showed up in the apparmor log that's why I added it. But I am happy for other suggestions. I am not too deep into apparmor / snap container confinements. So any other suggestion is welcome.

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

Well the problem is that /run/mount/utab is shared with the host. So on the outside it will seem from that file alone that some CIFS is mounted but when you go and look (again on the outside of snap application's point of view) they are not.

Does it fail outright if you drop this rule?

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

If I remember correctly, yes. But I will retry this later and record the logs and post them here, just to be sure.

Just in case this is relevant somehow: When I executed "mount" outside of the snap the shares were not visible in the output, when I entered the snaps name space with "snap run" I was able to see them in the list.

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

That's true (because the mount namespace shields us) but the userspace libmount library does use that file for extra bookkeeping. In some cases it can clash with what is really mounted outside so I think this needs some more investigation.

capability sys_admin,
# Allow mounts to our snap-specific writable directories
mount fstype=cifs ** -> /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

You can probably use @{HOMEDIRS} in place of explicit home instead. Though it doesn't matter much in snaps.

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

done

capability sys_admin,
# Allow mounts to our snap-specific writable directories
mount fstype=cifs ** -> /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

Could you please sort this section please?

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

done

mount fstype=cifs ** -> /home/*/snap/@{SNAP_NAME}/common/{,**/},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**/},
umount fstype=cifs /home/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

Same

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

done

c.Assert(si.BaseDeclarationSlots, testutil.Contains, "cifs-mount-control")
}
func (s *CifsMountControlInterfaceSuite) TestAutoConnect(c *C) {

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

You can drop this test I think.

This comment has been minimized.

@datenhahn

datenhahn Jun 18, 2018

Contributor

done

@zyga zyga requested a review from jdstrand Jun 18, 2018

@zyga

This comment has been minimized.

Contributor

zyga commented Jun 18, 2018

Replied to your question @datenhahn

@zyga

One more comment. In addition please see #5340 (comment) above

capability sys_admin,
# Allow mounts to our snap-specific writable directories
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**/},

This comment has been minimized.

@zyga

zyga Jun 18, 2018

Contributor

I think **/ is equivalent to **.

@zyga zyga added the ⛔ Blocked label Jun 18, 2018

@zyga

This comment has been minimized.

Contributor

zyga commented Jun 18, 2018

I've marked this as blocked for now. We need to have some solution for the libmount state handling issue. This is happening on the forum: https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jun 18, 2018

I also did some checks, and it seems the functionality itself is fine also without the permission, BUT it is reported as a security violation.

= AppArmor =
Time: Jun 18 13:27:33
Log: apparmor="DENIED" operation="open" profile="snap.smb-copier.smb-copier" name="/run/mount/utab" pid=4586 comm="umount" requested_mask="wrc" denied_mask="wrc" fsuid=0 ouid=0
File: /run/mount/utab (write)
Suggestions:
* adjust program to use $SNAP_DATA
* adjust program to use run/shm/snap.$SNAP_NAME.*
@zyga

This comment has been minimized.

Contributor

zyga commented Jun 18, 2018

Can you add deny /run/mount/utab rw, to the rules and push this change. This will keep the denial but silence the log message. Please reference this thread as a comment. This can unblock the PR in my eyes.

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jun 18, 2018

I added the line and tested again. The log message vanished and functionality is still there, so from my side this workaround would be ok.

@zyga

This comment has been minimized.

Contributor

zyga commented Jun 18, 2018

Thank you for the modification. I think from my point of view the only remaining immediate thing is #5340 (comment). I suspect you can just drop the trailing slash after two consecutive star characters.

This will now wait for a security review from @jdstrand and on a design/naming review from @niemeyer

@zyga zyga removed the ⛔ Blocked label Jun 18, 2018

@zyga zyga requested a review from niemeyer Jun 18, 2018

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jun 18, 2018

@zyga done, I removed the slashes

@zyga

This comment has been minimized.

Contributor

zyga commented Jun 19, 2018

@datenhahn you need to tweak integration tests, please look at this error message:

Missing high-level test for interface 'cifs-mount-control'. Please add to:
* tests/lib/snaps/test-snapd-policy-app-consumer/meta/snap.yaml
* tests/lib/snaps/test-snapd-policy-app-provider-core/meta/snap.yaml (if needed)
* tests/lib/snaps/test-snapd-policy-app-provider-classic/meta/snap.yaml (if needed)
@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jun 20, 2018

@zyga I added it to the first file. I don't really know if it is needed in the other ones, but as it is similar to fuse-support and that one is not there, so I guess the cifs-mount-control also doesn't have to go in the "(if needed)" files.

What commands do I have to run, to verify that I am not missing anything else?

@codecov-io

This comment has been minimized.

codecov-io commented Jun 20, 2018

Codecov Report

Merging #5340 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5340      +/-   ##
==========================================
- Coverage   78.96%   78.96%   -0.01%     
==========================================
  Files         503      523      +20     
  Lines       37785    39957    +2172     
==========================================
+ Hits        29836    31551    +1715     
- Misses       5536     5840     +304     
- Partials     2413     2566     +153
Impacted Files Coverage Δ
interfaces/builtin/cifs_mount.go 100% <100%> (ø)
cmd/snap/cmd_wait.go 66.12% <0%> (-13.47%) ⬇️
overlord/ifacestate/implicit.go 89.47% <0%> (-10.53%) ⬇️
cmd/snap/cmd_advise.go 56.25% <0%> (-9.93%) ⬇️
overlord/configstate/configcore/refresh.go 54.71% <0%> (-8.33%) ⬇️
interfaces/udev/udev.go 92.3% <0%> (-7.7%) ⬇️
store/details.go 95% <0%> (-5%) ⬇️
overlord/assertstate/assertmgr.go 73.33% <0%> (-4.03%) ⬇️
interfaces/udev/spec.go 90.58% <0%> (-3.94%) ⬇️
overlord/cmdstate/cmdmgr.go 84% <0%> (-3.88%) ⬇️
... and 142 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 446917c...68f4d1e. Read the comment docs.

@zyga

This comment has been minimized.

Contributor

zyga commented Jun 20, 2018

@datenhahn usually we run run-checks.sh but since the branch is green I don't think you need anything else. Let's just wait for the security review from Jamie and for design review from Gustavo.

@mvo5 mvo5 added this to the 2.34 milestone Jun 28, 2018

@niemeyer

This doesn't seem to fit the usual rules for the "-control" suffix. There's no well-defined resource being managed there. It's just allowing use of system tools to manipulate custom mounts. Perhaps "cifs-mount" would be appropriate as a pattern, anticipating further "*-mount" needs.

This needs a review from @jdstrand as well.

package builtin
const cifsMountControlSummary = `allows to mount and unmount CIFS shares`

This comment has been minimized.

@niemeyer

niemeyer Jun 29, 2018

Contributor
`allows mounting and unmounting CIFS filesystems`

?

This comment has been minimized.

@datenhahn

datenhahn Jul 2, 2018

Contributor

changed

`
const cifsMountControlConnectedPlugAppArmor = `
# Description: Allow to mount and unmount CIFS filesystems.

This comment has been minimized.

@niemeyer

niemeyer Jun 29, 2018

Contributor

"Allow mounting and unmounting", maybe

This comment has been minimized.

@datenhahn

datenhahn Jul 2, 2018

Contributor

changed

# Don't log violations for this file, see discussion here:
# - https://github.com/snapcore/snapd/pull/5340#issuecomment-398071797
# - https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987
deny /run/mount/utab rw,

This comment has been minimized.

@niemeyer

niemeyer Jun 29, 2018

Contributor

The described need for "mount-observe" seems a bit curious. Why is that necessary? Is it because the tools require it, or is there another limitation that prevent the mount syscall from working at all?

This comment has been minimized.

@zyga

zyga Jun 29, 2018

Contributor

The use of libmount requires the effective permissions granted by mount-observe

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jul 2, 2018

@niemeyer @zyga @jdstrand

Regarding the naming of the interface and the dependency to mount-observe I suggest you internally discuss this and then tell me in which way I should change the Pull-Request.

@mvo5 mvo5 modified the milestones: 2.34, 2.35 Jul 5, 2018

@zyga

zyga approved these changes Jul 13, 2018

This now looks good to me.

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Jul 30, 2018

@zyga is there a timeline when this change will be merged in to mainline and when approximately it would be in an ubuntu-core release? Our customer got their own brandstore and would like to drop the "--devmode" for install.

@mvo5 mvo5 added this to the 2.35 milestone Aug 8, 2018

@jdstrand

jdstrand requested changes Aug 15, 2018 edited

I apologize on the delay on this. I had a lot of reservations about the PR since I don't care for interfaces that allow mount operations in general and I wanted to give it some thought. Unfortunately, it then fell off my radar for a while.

Please see my comments inline for some changes and open questions. Now that I've worked through my reservations, I suspect we can get this into shape.

# Allow mounts to our snap-specific writable directories
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

Due to https://launchpad.net/bugs/1612393, @{HOMEDIRS} can't be used here. That said, I'd very much prefer if we didn't allow cifs mounts into the user's home. The operation would require root privileges and it is a tricky proposition for root to mount into the user's home.

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

That said, if there are important use cases for this functionality, please list them here.

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor

I removed the user home dir specific rules

mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**},
umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

These can be removed if the above are.

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor

I removed the user home dir specific rules

umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},
umount fstype=cifs /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs /var/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

Unfortunately, due to https://bugs.launchpad.net/apparmor/+bug/1613403, specifying fstype=cifs doesn't actually limit the umount. Please adjust to be:

# NOTE: due to LP: #1613403, fstype is not mediated and as such, these rules
# allow, for example, unmounting bind mounts from the content interface
umount fstype=cifs /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
umount fstype=cifs /var/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor

Changed as requested

This comment has been minimized.

@niemeyer

niemeyer Aug 21, 2018

Contributor

Isn't this a more serious problem, @jdstrand? If the snap can unmount, it can override mounts and get access to the underlying filesystem. Not sure if there's already a security issue created by this, but it's an axis we don't generally account for when reviewing logic.

This comment has been minimized.

@niemeyer

niemeyer Aug 21, 2018

Contributor

Sorry, nevermind... I see the unmounts are restricted to $SNAP_DATA, so it won't really be a problem.

# Don't log violations for this file, see discussion here:
# - https://github.com/snapcore/snapd/pull/5340#issuecomment-398071797
# - https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987
deny /run/mount/utab rw,

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

I don't care for the explicit denial here since deny rules take precedent over allow rules. Ie, this means if another interface came along that allowed the access, this denial would still deny it. We either need to not worry about the logged denial or extend the comment for this. I prefer the former as a reminder to do something about https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987/3.

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor

removed the explicit denial and updated the comment

mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 15, 2018

Contributor

Using '**' means that the snap could try to mount anything from the host into SNAP_DATA or SNAP_COMMON, which would bypass intended access restrictions. That said, the fstype should cause things to fail, but a crafted mount command shipped in the snap might be used to tickle kernel bugs. What do denials look like when you try to mount cifs filesystems and you don't have these rules?

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor
= AppArmor =
Time: Jul 15 05:52:53
Log: apparmor="DENIED" operation="mount" info="failed mntpnt match" error=-13 profile="snap.smb-copier.smb-copier" name="/var/snap/smb-copier/common/shares/" pid=14381 comm="mount.cifs" fstype="cifs" srcname="//192.168.123.1/sambashare"

smb-copier is a simple daemon stripped down to only trigger mount umount with which I am testing the interface:

https://github.com/datenhahn/devtools-cifs-mount-control

This comment has been minimized.

@datenhahn

datenhahn Aug 16, 2018

Contributor

@jdstrand what would you suggest otherwise? I would like to allow all kind of cifs options and arbitrary cifs pathes. Would it help to let the mount pathes start with // ? How would I allow all mount options and pathes starting with // ?

This comment has been minimized.

@jdstrand

jdstrand Aug 17, 2018

Contributor

It would look better if we could do:

mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/common/{,**},

Can you try it locally and see if it works? I suspect however that it will not and that apparmor_parser will minimize that to '/**' internally and it won't match. If it doesn't work, simply use:

// srcname is of form '//<ip>/dir1/dirN' and is therefore not predictable so we'll
// rely on the fstype.
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/common/{,**},

datenhahn added some commits Aug 16, 2018

cifs-mount-control interface (discussed in snapcraft.io forum post 5689)
  https://forum.snapcraft.io/t/interface-mount-umount-cifs-share-permission/5689

- remove rules allowing mount into user homes
- remove explicit deny of /run/mount/utab (this would interfere with
  allows from other interfaces)
- add warning comment to unmount
@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Aug 16, 2018

@jdstrand I addressed most issues, do you have a suggestion regarding the ** in the mount rules? @niemeyer had a comment regarding the naming of the interface, did you discuss this and is cifs-mount-control ok or should it be changed to something else? (he suggested cifs-mount.

# decide to allow access (deny rules have precedence).
#
# - https://github.com/snapcore/snapd/pull/5340#issuecomment-398071797
# - https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987`

This comment has been minimized.

@jdstrand

jdstrand Aug 17, 2018

Contributor

You deleted the trailing '`' so this isn't syntactically correct. I suggest adding this:

#deny /run/mount/utab w,
`

(ie, add a comment and the trailing ''). If you run ./run-checks before committing, it will catch this sort of thing.

mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> @{HOMEDIRS}/*/snap/@{SNAP_NAME}/common/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 17, 2018

Contributor

It would look better if we could do:

mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs //*/** -> /var/snap/@{SNAP_NAME}/common/{,**},

Can you try it locally and see if it works? I suspect however that it will not and that apparmor_parser will minimize that to '/**' internally and it won't match. If it doesn't work, simply use:

// srcname is of form '//<ip>/dir1/dirN' and is therefore not predictable so we'll
// rely on the fstype.
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs -> /var/snap/@{SNAP_NAME}/common/{,**},
@jdstrand

Few small things.

@jdstrand

This is close.

spec := &apparmor.Specification{}
c.Assert(spec.AddConnectedPlug(s.iface, s.plug, s.slot), IsNil)
c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, `/run/mount/utab`)

This comment has been minimized.

@jdstrand

jdstrand Aug 17, 2018

Contributor

This will need to be adjusted since you removed it (please adjust to be #deny /run/mount/utab,\\n since I recommended a commented out rule.

@jdstrand

This comment has been minimized.

Contributor

jdstrand commented Aug 17, 2018

I agree with @niemeyer, please rename to 'cifs-mount'.

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Aug 17, 2018

@jdstrand @niemeyer

I renamed the interface to cifs-mount. I wasn't able to run ./run-checks it fails when parsing some shellscripts in the tests directories. But at least the format-check and vet phase ran through. I was able to run the cifs_mount_test.go locally (just that file in Goland IDE).

I also changed the mount command to only allow network shares ( //** ).

@jdstrand

You might find these commands helpful for testing just your changes:

$ go build -v github.com/snapcore/snapd/... && go test -v github.com/snapcore/snapd/interfaces/builtin
$ go build -v github.com/snapcore/snapd/... && go test -v github.com/snapcore/snapd/interfaces/policy

Those will run the unit tests for your added interface and the base declaration.

mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs ** -> /var/snap/@{SNAP_NAME}/common/{,**},
mount fstype=cifs //** -> /var/snap/@{SNAP_NAME}/@{SNAP_REVISION}/{,**},
mount fstype=cifs //** -> /var/snap/@{SNAP_NAME}/common/{,**},

This comment has been minimized.

@jdstrand

jdstrand Aug 17, 2018

Contributor

Can you comment that this actually works for you?

This comment has been minimized.

@datenhahn

datenhahn Aug 20, 2018

Contributor

yes, I tested it with my minimal test snap and everything seemed to work fine:

https://github.com/datenhahn/devtools-cifs-mount-control

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Aug 20, 2018

I ran the two commandlines for running tests and both passed

@mvo5 mvo5 modified the milestones: 2.35, 2.36 Aug 20, 2018

@jdstrand

Thanks for all the changes! LGTM as is.

I do have a few teensy consistency nitpicks that would be nice to fix before committing.

// -*- Mode: Go; indent-tabs-mode: t -*-
/*
* Copyright (C) 2016-2018 Canonical Ltd

This comment has been minimized.

@jdstrand

jdstrand Aug 20, 2018

Contributor

Please adjust to '2018'

This comment has been minimized.

@datenhahn

datenhahn Aug 21, 2018

Contributor

changed

const cifsMountConnectedPlugSecComp = `
# Description: Allow mount and umount syscall access.

This comment has been minimized.

@jdstrand

jdstrand Aug 20, 2018

Contributor

Can you remove this newline?

This comment has been minimized.

@datenhahn

datenhahn Aug 21, 2018

Contributor

removed

# /run/mount/utab but fails. The resulting apparmor warning can be ignored. The log warning
# was not removed via an explicit deny to not interfere with other interfaces which might
# decide to allow access (deny rules have precedence).
#

This comment has been minimized.

@jdstrand

jdstrand Aug 20, 2018

Contributor

Can you remove this blank comment line?

This comment has been minimized.

@datenhahn

datenhahn Aug 21, 2018

Contributor

removed

#
# - https://github.com/snapcore/snapd/pull/5340#issuecomment-398071797
# - https://forum.snapcraft.io/t/namespace-awareness-of-run-mount-utab-and-libmount/5987
#

This comment has been minimized.

@jdstrand

jdstrand Aug 20, 2018

Contributor

Can you remove this blank comment line?

This comment has been minimized.

@datenhahn

datenhahn Aug 21, 2018

Contributor

removed

c.Assert(spec.SecurityTags(), DeepEquals, []string{"snap.consumer.app"})
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "mount\n")
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "umount\n")
c.Assert(spec.SnippetForTag("snap.consumer.app"), testutil.Contains, "umount2\n")

This comment has been minimized.

@jdstrand

jdstrand Aug 20, 2018

Contributor

Just one of these is enough.

This comment has been minimized.

@datenhahn

datenhahn Aug 21, 2018

Contributor

Not sure I understood your comment correctly. I changed it and only kept the umount2 assert (it is most specific for the cifs mount util). If one assertion is true all settings were applied successfully anyway, that was your point, right?

@jdstrand jdstrand changed the title from cifs-mount-control interface (discussed in snapcraft.io forum post 5689) to cifs-mount interface (discussed in snapcraft.io forum post 5689) Aug 20, 2018

@niemeyer niemeyer changed the title from cifs-mount interface (discussed in snapcraft.io forum post 5689) to interfaces: add cifs-mount interface Aug 21, 2018

@niemeyer

LGTM, and this looks ready to go in.

One last question: have you signed the CLA already, @datenhahn?

If so, please just let me know (privately if you want) the email you signed with and we'll merge it. If not, it's a simple process and non-draconian agreement you can easily sign online:

http://www.ubuntu.com/legal/contributors

Thanks for your help.

@datenhahn

This comment has been minimized.

Contributor

datenhahn commented Aug 22, 2018

@niemeyer I signed it just now and put you as "Canonical Contact" , I am not 100% sure if I chose the correct launchpad-ID (I don't have one, I sighed in with my Ubuntu-One account and I put that ID)

@niemeyer

This comment has been minimized.

Contributor

niemeyer commented Aug 22, 2018

@datenhahn I see it, thanks! Change going in...

@niemeyer niemeyer merged commit 7cce3f5 into snapcore:master Aug 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment