Skip to content
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

snap: ensure security polices are re-created #3442

Closed
wants to merge 9 commits into from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Jun 7, 2017

Based on #3438
For easy review: mvo5@1dfa2ee

Wait in `snap run` for snapd to finish activating

Our snapd uses the systemd notify protocol and will only finish
initialization when all security profiles have been re-generated.

This allows us to check in `snap run` if snapd is still activating
and if so to wait a little bit. This will wait for a maximum of
5s after that the old profiles will be used. Note that systemd
itself will give up after ~100 seconds so the worst case is that
snapd fails to start on boot. In this case all snaps will start
with a delay of 5 seconds in the first 100 seconds. After that
things will be "normal" (except that the old security profiles
will be used).

mvo5 added 3 commits June 7, 2017 18:07
Our snapd uses the systemd notify protocol and will only finish
initialization when all security profiles have been re-generated.

This allows us to check in `snap run` if snapd is still activating
and if so to wait a little bit. This will wait for a maximum of
5s after that the old profiles will be used. Note that systemd
itself will give up after ~100 seconds so the worst case is that
snapd fails to start on boot. In this case all snaps will start
with a delay of 5 seconds in the first 100 seconds. After that
things will be "normal" (except that the old security profiles
will be used).
@mvo5 mvo5 force-pushed the snapd-notify-snap-confine branch from ca17563 to 64a6c16 Compare June 8, 2017 08:12
@codecov-io
Copy link

Codecov Report

Merging #3442 into master will decrease coverage by 0.01%.
The diff coverage is 46.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3442      +/-   ##
==========================================
- Coverage   77.23%   77.22%   -0.02%     
==========================================
  Files         373      373              
  Lines       25645    25658      +13     
==========================================
+ Hits        19808    19815       +7     
- Misses       4087     4090       +3     
- Partials     1750     1753       +3
Impacted Files Coverage Δ
cmd/snap/cmd_run.go 63.31% <46.15%> (-1.2%) ⬇️
interfaces/sorting.go 96.66% <0%> (+3.33%) ⬆️

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 d538109...0a04269. Read the comment docs.

// permanent failed state if it cannot be activated and at this
// point the code will continue (with potentially stale profiles
// but at least services will run).
for i := 0; i < 500; i++ {
Copy link

@jdstrand jdstrand Jun 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that 5 seconds is not very long on armhf. Touch profiles could take as long as 1 second to load each, so a system with many profiles on armhf could fallback to old profiles pretty regularly, depending on when the systemd unit started.

// XXX2: the backported systemd on 14.04 does not support
// the dbus interface of systemd, only the private
// socket which is only available as root, ignore
// this error.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to not being supported on 14.04, it occurred to me that performing a DBus call in snap run is overhead we could avoid with a stamp file. I'm not sure if this fits all the properties you are looking for, but if instead of this DBus call snap run checks for the existence of a file (eg /run/snapd/something-appropriate) and waits on that, this would be a cheap check that would also work on 14.04. This stamp file would be created after profile generation finished and it could be removed on 'stop' so that 'snap run' during a core snap refresh would cause snap run to wait.

@mvo5
Copy link
Contributor Author

mvo5 commented Jun 9, 2017

Closing for now, this needs some more work after the latest forum discussions

@mvo5 mvo5 closed this Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants