Code review comment for lp://qastaging/~jdstrand/snap-confine/ubuntu-core-launcher.pts

Revision history for this message
Seth Arnold (seth-arnold) wrote :

This looks good with a few caveats:

- AppArmor "ought to" support something like "options=newinstance" so that
  we can reject any mounts that may violate our expectations. "Ought to"
  since it looks like the parser and kernel were written with this in mind
  but I can't figure out how to make it work.

- We really shouldn't be afraid to modify our startup routines to do the
  right thing. We can either adjust now and deal with the fallout or we
  can continue to live on the assumption that no one will ever screw up,
  ever. Setting -o newinstance on the very first mount sounds like the
  best way to reduce our long-term technical debt.

- I'm not sure the gid=5 is correct if a gid map is in place
  (/proc/pid/guid_map). I can't find documentation on this one way or
  another. I thought I'd call this out in case things don't look correct
  elsewhere (say, gids are wrong in containers) then this may be recalled.

Thanks

review: Approve

« Back to merge proposal