/snapd

Allow types to hand out security snippets #326

Merged
merged 13 commits into from Jan 15, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@zyga @niemeyer @mvo5
@zyga
Contributor

zyga commented Jan 14, 2016

This patch commences the work on the security side of capabilities. Each
capability type now has a way to hand out "snippets" of security
information applicable to a given security system. The information
describes alterations of the security system needed to consume a given
capability.

The patch defines three security system names:

  • apparmor
  • seccomp
  • dbus
@zyga
Allow types to hand out security snippets 
This patch commences the work on the security side of capabilities. Each
capability type now has a way to hand out "snippets" of security
information applicable to a given security system. The information
describes alterations of the security system needed to consume a given
capability.

The patch defines three security system names:
 - apparmor
 - seccomp
 - dbus

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.07%) to 67.603%
Details
a02f779
caps/security.go
+
+package caps
+
+// NOTE: all the security constants are used by Type.SecuritySnippet()
@niemeyer

niemeyer Jan 14, 2016

Contributor

The note doesn't seem necessary, and will likely be wrong soon. The constants are used wherever they are used.

@zyga

zyga Jan 14, 2016

Contributor

Removed.

caps/security.go
+// NOTE: all the security constants are used by Type.SecuritySnippet()
+const (
+ // Identifier of the apparmor security system.
+ SecurityApparmor = "apparmor"
@niemeyer

niemeyer Jan 14, 2016

Contributor

These constants should be typed. Please define a type SecuritySystem above, and then:

SecurityApparmor SecuritySystem = "apparmor"

etc.

@niemeyer

niemeyer Jan 14, 2016

Contributor

Also, can the comments be dropped or is golint also enforcing them? They're not adding any useful information on top of the code.

@zyga

zyga Jan 14, 2016

Contributor

Golint is enforcing them but since they will likely stay package-local I can just make them private.

@niemeyer

niemeyer Jan 14, 2016

Contributor

Well, not really. It doesn't make sense to have a public API that depends on private constants.

zyga added some commits Jan 14, 2016

@zyga
Use typed security system namges 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
5d0676e
@zyga
Remove note about security system names 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
e9bf44a
caps/types.go
@@ -31,6 +31,9 @@ type Type interface {
Name() string
// Sanitize a capability (altering if necessary).
Sanitize(c *Capability) error
+ // Obtain the security snippet for the given security system.
+ // If no security snippet is needed, hand out empty string.
@niemeyer

niemeyer Jan 14, 2016

Contributor 
// SecuritySnippet returns the configuration snippet that should be used by
// the given security system to enable this capability to be consumed.
// An empty snippet is returned when the capability doesn't require anything
// from the security system to work, in addition to the default configuration.
// ErrUnknownSecurity is returned when the capability cannot deal with the
// requested security system.
@zyga

zyga Jan 14, 2016

Contributor

Copied, thanks :-)

@zyga
Make security system names private 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
748dc6b
caps/types.go
@@ -31,6 +31,9 @@ type Type interface {
Name() string
// Sanitize a capability (altering if necessary).
Sanitize(c *Capability) error
+ // Obtain the security snippet for the given security system.
+ // If no security snippet is needed, hand out empty string.
+ SecuritySnippet(c *Capability, securitySystem string) (string, error)
@niemeyer

niemeyer Jan 14, 2016

Contributor

Considering the data nature, I think the most appropriate result type here is actually a []byte.

@zyga

zyga Jan 14, 2016

Contributor

Ah, good point, thanks.

zyga added some commits Jan 14, 2016

@zyga
Fix typo 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
6b36c64
@zyga
Fix inaccurate TODO 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.07%) to 67.603%
Details
1cf3495
caps/types.go
@@ -60,6 +63,24 @@ func (t *BoolFileType) Sanitize(c *Capability) error {
return nil
}
+// SecuritySnippet for bool-file capability type.
@niemeyer

niemeyer Jan 14, 2016

Contributor

The standard convention for Go comments should be used here too.

caps/types.go
+ // Allow read,write and lock on the file designated by the path.
+ return fmt.Sprintf("%s rwl,\n", path), nil
+ case SecuritySeccomp:
+ return "", nil
@niemeyer

niemeyer Jan 14, 2016

Contributor

If Seccomp is active, will this capability work if nothing is configured? If it won't, then we shouldn't return an empty snippet, as that indicates nothing is required for it to work.

@zyga

zyga Jan 14, 2016

Contributor

From what I read so far seccomp defaults are sufficient for bool-file (open, read, write are all there). @jdstrand can you confirm this please?

@niemeyer

niemeyer Jan 14, 2016

Contributor

If we can open/read/write any file, what else is being restricted?

caps/types.go
+ case SecurityDBus:
+ return "", nil
+ default:
+ return "", fmt.Errorf("unknown security system %q", securitySystem)
@niemeyer

niemeyer Jan 14, 2016

Contributor

This should be ErrUnknownSecurity so we can differentiate the capability blowing up with a known security system for whatever reason from it not being able to handle the given security system at all.

@zyga

zyga Jan 14, 2016

Contributor

Done.

caps/types.go
+func (t *TestType) SecuritySnippet(c *Capability, securitySystem string) (string, error) {
+ switch securitySystem {
+ case SecurityApparmor:
+ fallthrough
@niemeyer

niemeyer Jan 14, 2016

Contributor

Seems wrong to fallthrough here. It's trivial to return an empty snippet, and if any of these are actually implemented the actual implementation will definitely not be common.

On another angle, what security system would be unsupported for the test type, considering it doesn't require anything at all from the system to implement the capability? Might be worth returning an empty snippet in all cases, and maybe allow a callback similar to Sanitize if that turns out to be useful?

@zyga

zyga Jan 14, 2016

Contributor

Changed, let me know what you think. I've opted for simplicity so there's no callback yet. Once we have a need for one it can be easily added.

caps/security.go
+type SecuritySystem string
+
+const (
+ securityApparmor SecuritySystem = "apparmor"
@niemeyer

niemeyer Jan 14, 2016

Contributor

A public API shouldn't depend on private constants.

@zyga

zyga Jan 14, 2016

Contributor

Fixed. I tweaked the comments around each constant below. Golint is now happy but I don't know if they provide any useful value.

@niemeyer
Contributor

niemeyer commented Jan 14, 2016

The structure looks good. Please ping again when the details above are addressed.

zyga added some commits Jan 14, 2016

@zyga
Tweak Type.SecuritySnippet() 
There are two changes, based on feedback from code review.

First the return type is now using []byte rather than string since it
will likely fit the intended problem better (it's not clear that all
security systems will consume plain strings).

The second change add a well-known error type for reporting unknown
security systems.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
166d2c7
@zyga
Revert "Make security system names private" 
This reverts commit 748dc6b.
069af80
@zyga
Tweak security system docstrings 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 1 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
490cca5
@zyga
Tweak comments on SecuritySnippet() 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 1 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
e1124e7
@zyga
Type names are FooError, variables are ErrFoo 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.06%) to 67.591%
Details
bdd3765
@zyga
Replace UnknownSecurityError with ErrUnknownSecurity 
This error will likely be checked for in a handful of places in the
whole codebase so we don't have to have a unique type for it. At the
same time we'll know exactly what the security system name was at that
point so carrying this around is useless.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 67.582%
Details
142d84b
caps/types.go
+ // TODO: switch to the real path later
+ path := c.Attrs["path"]
+ // Allow read, write and lock on the file designated by the path.
+ return ([]byte)(fmt.Sprintf("%s rwl,\n", path)), nil
@niemeyer

niemeyer Jan 14, 2016

Contributor

[]byte("foo") works.. no need for the extra parenthesis.

@zyga

zyga Jan 14, 2016

Contributor

Fixed, C habits die hard.

@niemeyer
Contributor

niemeyer commented Jan 14, 2016

LGTM.. just a trivial above.
@zyga
Remove unneeded parenthesis 
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
Integration tests — No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 67.582%
Details
420680a
@mvo5
Collaborator

mvo5 commented Jan 15, 2016

Looks good.

mvo5 added a commit that referenced this pull request Jan 15, 2016

@mvo5
Merge pull request #326 from zyga/type-security 
Allow types to hand out security snippets
981d961

@mvo5 mvo5 merged commit 981d961 into snapcore:master Jan 15, 2016
2 of 3 checks passed

2 of 3 checks passed

Integration tests No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 67.582%
Details

@zyga zyga deleted the zyga:type-security branch Feb 1, 2016

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