skills: add security layer #468

Merged
merged 14 commits into from Feb 16, 2016

Conversation

Projects
None yet
3 participants
Contributor

zyga commented Feb 9, 2016

This branch adds support for generating files for all the security subsystems that snappy is currently using today. Those are: apparmor, seccomp, udev and dbus. Since StateEngine is not yet operational, the top-level API for the whole interface is skills.Repository.SecurityFilesForSnap() that produces information about all the files (and their contents) required to make effective skill-based security work.

There are several TODOs, mostly about using real (longish) preambles to various files.

zyga added some commits Feb 9, 2016

skills: use CamelCase for security names
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add constant for udev security
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: make TestType security snippets configurable
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add securityHelper interface
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add SecuritySnippetsForSnap()
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add SecurityFilesForSnap()
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add appArmor security helper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add secComp security helper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add uDev security helper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: add dBus security helper
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills/repo.go
@@ -407,3 +415,98 @@ func (c bySlotSnapAndName) Less(i, j int) bool {
func LoadBuiltInTypes(repo *Repository) error {
return nil
}
+
+// SecuritySnippetsForSnap collects all of the snippets of a given security
+// system that affect a given snap. The return value is indexed by app name
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

2 spaces after .

skills/repo_test.go
+ }
+ repo := s.emptyRepo
+ err := repo.AddType(t)
+ c.Assert(err, IsNil)
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

Maybe c.Assert(repo.AddType(t), IsNil) would make this test read more concisely

skills/repo.go
+ return snippets, nil
+}
+
+// SecurityFilesForSnap computes files that constitute all of the security permissions.
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

Is "computes files" the right way thing to say here? This method returns the paths and contents of the security files?

skills/repo_test.go
+// Tests for Repository.SecuritySnippetsForSnap()
+
+func (s *RepositorySuite) TestSlotSnippetsForSnapSuccess(c *C) {
+ var testSecurity SecuritySystem = "security"
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

Could use const here instead of var

skills/security.go
+// identifier can be either the full path of the executable or an abstract
+// identifier not related to the executable name.
+//
+// File containing apparmor profile has to be parsed, compiled and loaded into
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

A file containing an apparmor profile may make more sense

skills/security.go
+// File containing apparmor profile has to be parsed, compiled and loaded into
+// the running kernel using apparmor_parser. After this is done the actual file
+// is irrelevant and can be removed. To improve performance certain command
+// line options to apparmor_parser can be used to cache compiled profile across
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

Need a plural here? cache compiled profiles

skills/security.go
+// secComp is a security subsystem that writes additional seccomp rules.
+//
+// Rules use a simple line-oriented record structure. Each line specifies a
+// system call that is allowed. Lines starting with "deny" specify system
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

2 spaces after .

skills/security.go
+// Rules use a simple line-oriented record structure. Each line specifies a
+// system call that is allowed. Lines starting with "deny" specify system
+// calls that are explicitly not allowed. Lines starting with '#' are treated
+// as comments are ignored.
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

s/are/and are/

skills/security.go
+//
+// NOTE: This subsystem interacts with ubuntu-core-launcher. The launcher reads
+// a single profile from a specific path, parses it and loads a seccomp profile
+// (using Berkley packet filter as low level mechanism).
@stevenwilkin

stevenwilkin Feb 9, 2016

Member

s/as/as a/

zyga added some commits Feb 9, 2016

skill: correct comment typos
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: use more concise test style
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
skills: use const instead of var
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented Feb 9, 2016

Thanks @stevenwilkin -- I've corrected everything that you pointed out.

Member

chipaca commented Feb 16, 2016

LGTM. Landing.

chipaca added a commit that referenced this pull request Feb 16, 2016

@chipaca chipaca merged commit 33f6682 into snapcore:master Feb 16, 2016

3 checks passed

Integration tests Success 60 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 69.516%
Details

@zyga zyga deleted the zyga:skills-security branch Mar 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment