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-{confine,seccomp}: make @unrestricted fully unrestricted #4054

Merged
merged 8 commits into from
Oct 27, 2017

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Oct 19, 2017

For @unrestricted we did load a "allow-all" bpf program so far.
However in LP: 1724697 the LXD team asks for fully unrestricted
mode (i.e. no filter at all) because they need to put their own
filter in place and they don't stack apparently.

The only downside/risk with this branch is that if snapd and
snap-confine get out of sync things will get messy as an old
snap-confine will try to load an invalid bpf program
(the program that reads "@unrestricted").

For @unrestricted we did load a "allow-all" bpf program so far.
However in LP: 1724697 the LXD team asks for fully unrestricted
mode (i.e. no filter at all) because they need to put their own
filter in place and they don't stack apparently.

The only downside/risk with this branch is that if snapd and
snap-confine get out of sync things will get messy as an old
snap-confine will try to load an invalid bpf program
(the program that reads "@unrestricted").
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks good

@codecov-io
Copy link

codecov-io commented Oct 19, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4054      +/-   ##
==========================================
- Coverage   75.75%   75.75%   -0.01%     
==========================================
  Files         435      435              
  Lines       37442    37441       -1     
==========================================
- Hits        28366    28365       -1     
  Misses       7094     7094              
  Partials     1982     1982
Impacted Files Coverage Δ
cmd/snap-seccomp/main.go 54.21% <100%> (-0.19%) ⬇️

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 0190c29...ed22d4f. Read the comment docs.

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

The memcmp() is technically fine but it means that @unrestricted<GARBAGE> is allowed. I also don't like text files that have a missing newline, which we do in this PR. I'd prefer we be a little more tidy. Eg, test for "@unrestricted\n" and test the size of the file. Another option is simply to use an empty file.

@zyga
Copy link
Contributor

zyga commented Oct 19, 2017

@jdstrand I'd like to avoid an empty file as any glitch that truncates the seccomp profile could turn it into accidental unconfined profile. I think the string is a good and explicit expression of that.

@stgraber
Copy link
Contributor

approach looks good to me

@mvo5 mvo5 added this to the 2.29 milestone Oct 20, 2017
Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@mvo5 mvo5 merged commit 1c243d0 into canonical:master Oct 27, 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.

5 participants