Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add security system #312
Conversation
zyga
added some commits
Jan 11, 2016
jdstrand
reviewed
Jan 11, 2016
| +// legacySecurtySystem aids in the ongoing work to transition capability system | ||
| +// from hwaccess API to a more direct approach. It allows particular capability | ||
| +// types to define a common interface that doesn't expose hwaccess API anymore. | ||
| +type legacySecurtySystem struct { |
jdstrand
reviewed
Jan 11, 2016
| @@ -83,3 +86,52 @@ func (t *Type) MarshalJSON() ([]byte, error) { | ||
| func (t *Type) UnmarshalJSON(data []byte) error { | ||
| return json.Unmarshal(data, &t.Name) | ||
| } | ||
| + | ||
| +// GrantPermissions makes it possible for the package `snapName` to use hardware |
jdstrand
Jan 11, 2016
Contributor
'hardware' is probably not the right word here for this comment when considering pure security capabilities (eg, 'firewall-management' as opposed to 'camera')
|
I was asked to look at the approach (ie, I haven't done an in depth code review or tested this). The direction this is going looks good from a security capabilities definition/grant/revoke perspective. Thanks! |
zyga
added some commits
Jan 11, 2016
|
@jdstrand thanks for looking at this! |
chipaca
reviewed
Jan 12, 2016
| + sec := cap.Type.SecuritySystems[j] | ||
| + if err := sec.RevokePermissions(snapName, cap); err != nil { | ||
| + // XXX: Should we do something other than panic here? | ||
| + panic(fmt.Sprintf("unable to revoke partially granted permissions: %q", err)) |
chipaca
Jan 12, 2016
Member
eventually yes, there's got to be a big red "give up and work out the current state of the system from the top again" button in the Overseer
chipaca
reviewed
Jan 12, 2016
| + } | ||
| + // Grant all permissions required. | ||
| + for i := range cap.Type.SecuritySystems { | ||
| + sec := cap.Type.SecuritySystems[i] |
chipaca
reviewed
Jan 12, 2016
| +} | ||
| + | ||
| +// RevokePermissions undoes the effects of GrantPermissions. | ||
| +func (t *Type) RevokePermissions(snapName string, cap *Capability) error { |
|
this seems ok to me. The amount of code that's exactly line-by-line the same between Type.RevokePermissions and Type.GrantPermissions makes me think there's got to be a way of writing the code once, but it can happen later if you'd rather. |
niemeyer
reviewed
Jan 12, 2016
| @@ -42,3 +46,22 @@ type Capability struct { | ||
| func (c Capability) String() string { | ||
| return c.Name | ||
| } | ||
| + | ||
| +// SetAttr sets capability attribute to a given value. | ||
| +// TODO: remove temporary function implementation once attrtypes are merged. |
niemeyer
Jan 12, 2016
Contributor
Can we please drop these temporary methods altogether for the time being? They're purely a setter and a getter for a public map attribute, with literally zero benefit. That means we're simply adding cruft that needs to be removed later and that is hard to agree or disagree because the full replacement TODOs give them a blank check of existence.
niemeyer
reviewed
Jan 12, 2016
| +// system (syccomp, apparmor, etc) from the point of view of a capability. | ||
| +type securitySystem interface { | ||
| + // TODO: change snap to appropriate type after current transition | ||
| + GrantPermissions(snapName string, cap *Capability) error |
niemeyer
Jan 12, 2016
Contributor
We agreed to not have this in our call yesterday. Every single capability type in our system will be unique, precisely because its solo purpose of existence is informing the system of a new capacity which comes along with new security behavior. We cannot hand off an arbitrary capability to a security system and expect it will know what to do with it.
niemeyer
reviewed
Jan 12, 2016
| + // Fetch the attribute where the path is stored | ||
| + path, err := cap.GetAttr(sec.AttrName) | ||
| + if err != nil { | ||
| + return fmt.Errorf("%s, cannot get required attribute: %q", errPrefix, err) |
niemeyer
Jan 12, 2016
Contributor
Here the mismatch becomes obvious. How can the legacy security system know that an arbitrary capability which it has no knowledge about contains a given attribute?
|
@zyga I'm closing this for the time being. Once the points and design we discussed yesterday are addressed, please reopen it or push a new PR. |
zyga commentedJan 11, 2016
This branch sets the stage for pluggable security systems.
Each capability type can now refer to a list of security systems. The type can be asked to grant or revoke permission expressed by those systems. A trivial "legacy" hw-assign security system is provided.
The main intent of this branch is to discuss the interface as I have some code that uses this for apparmor and seccomp coming up.