snappy: add default security profile if none is specified #416

Merged
merged 10 commits into from Feb 3, 2016

Conversation

Projects
None yet
4 participants
Collaborator

mvo5 commented Feb 2, 2016

Instead of forcing people to explicitly add the migration-skill do so implicitly if no other skill is specified.

mvo5 added some commits Feb 2, 2016

Update integration tests for new snap.yaml
Also skip some of the failover tests for now that look like they
are broken for unreleated reasons.
Contributor

zyga commented Feb 2, 2016

Looks good, +1

@mvo5 mvo5 closed this Feb 2, 2016

@mvo5 mvo5 reopened this Feb 2, 2016

Do not grant "network-client" security-cap by default
As discussed with Gustavo the default for the migration-skill
should be no implicit capabilties.
snappy/security.go
@@ -71,6 +71,12 @@ var (
SecurityCaps: []string{},
}
+ // the default migration skill if no default is uses in
@niemeyer

niemeyer Feb 2, 2016

Contributor

That doesn't sound right. We don't want the migration-skill to come in by default. We want people to explicitly mention it if they depend on anything non-default.

The permissions we discussed so far are:

  • Writing into the applications own space. That should definitely come by default. It did so in the old convention, and it should continue to do so in the new convention.
  • Network client access. That used to be a default in the old convention when nothing was specified, and we agreed to not have it by default in the new convention, at least for now. If people want network access, they need to explicitly mention migration-skill. That ensures we can go into either direction soon depending on what we decide at the sprint, without having to break anyone.

Is there anything else we're missing that exists in the default template of the old convention?

mvo5 added some commits Feb 2, 2016

Do not add "migration-skill" if no skill is specified
Instead, add a new defaultSecurityPolicy which defaults
to access to snaps own space but without "network-client".
Collaborator

mvo5 commented Feb 2, 2016

@niemeyer Thanks for the review! I changed this now to the following:

  • if there is a skill the security generation is done via the skill (currently only the migration skill)
  • if there is no skill specified a default security policy is generated that has access to its own space and no security-caps, i.e. no network-client cap
  • the migration-skill is left unchanged, the defaults of the old system are used if the user explicitly requests the migration skill (that means if the migration skill is used by default the network-client cap is granted)

I hope this addresses the concerns. Please let me know if there is anything I can do.

@mvo5 mvo5 changed the title from Bugfix/add migration skill to snappy: add default security profile if none is specified Feb 2, 2016

@@ -762,7 +768,12 @@ func generatePolicy(m *snapYaml, baseDir string) error {
if err != nil {
return err
}
+
+ // if no skill is specified, use the defaultSecurityPolicy
@niemeyer

niemeyer Feb 2, 2016

Contributor

// TODO This is not actually right. Even if there are skills, we still want to give the snap
// a default set of allowances, such as being able to read and write in its own directories
// and perhaps network access (we're still deciding on that one). So the real logic we
// want here is: give the snap a default set of permissions, and then whatever else the
// skills permit (migration or not). This is coming soon.

Contributor

niemeyer commented Feb 2, 2016

LGTM, let's please just have the comment indicated replaced by that text so it's more clear where we are going.

Collaborator

mvo5 commented Feb 2, 2016

Thanks a bunch! Comment added.

Collaborator

mvo5 commented Feb 2, 2016

retest this please

Member

elopio commented Feb 2, 2016

retest this please

mvo5 added a commit that referenced this pull request Feb 3, 2016

Merge pull request #416 from mvo5/bugfix/add-migration-skill
snappy: add default security profile if none is specified

@mvo5 mvo5 merged commit 704ade2 into snapcore:master Feb 3, 2016

3 checks passed

Integration tests Success 57 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 70.301%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment