Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Feature/native security policygen regen #55
Conversation
jdstrand
and others
added some commits
Oct 21, 2015
niemeyer
reviewed
Nov 10, 2015
| + return "", &errPolicyNotFound{"template", sp, template} | ||
| +} | ||
| + | ||
| +func (sp *securityPolicyType) findCaps(caps []string, template string) ([]string, error) { |
niemeyer
Nov 10, 2015
Contributor
It would be nice to have more documentation for methods and functions in general. This case, for example, is a non-trivial function which I would appreciate knowing upfront what its intention is. Even reviewing the logic, it's not quite clear that indeed it does what it's supposed to do.
niemeyer
reviewed
Nov 10, 2015
| + break | ||
| + } | ||
| + } | ||
| + if found == false { |
niemeyer
Nov 10, 2015
Contributor
i can't tell if this is a bug or not, but the way this loop is implemented is at least curious. If the function receives a caps list of [ExistingCap, NotExistingCap], it returns no errors, but if the list is [NotExistingCap, ExistingCap] it returns an error. Is this a bug, or intended behavior? Again, docs explaining what the intention is would be nice.
mvo5
Nov 10, 2015
Collaborator
Thanks, good catch! I'm pretty sure this is a bug (@jdstand please correct me if I'm wrong). The loops are too hard to follow IMHO so I re-factored into smaller functions that are hopefully easier to follow. I also added a test for the bug you noticed.
|
I've provided a few initial comments, but the branch size won from my attention span. I'll need to return to it. |
mvo5
and others
added some commits
Nov 10, 2015
|
mvo and I went through the additions and bugs and this is ok to merge with the latest commits. There is additional work that should be done, but it is minor and should not block this already large pull request. Thanks to everyone for the reviews and especially thanks to mvo for all the help on the remaining issues this week. |
niemeyer
reviewed
Nov 14, 2015
| + * `read-paths`: a list of additional paths that the app can read | ||
| + * `write-paths`: a list of additional paths that the app can write | ||
| + * `abstractions`: a list of additional abstractions for the app | ||
| + * `syscalls`: a list of additional syscalls that the app can use |
niemeyer
Nov 14, 2015
Contributor
This is an area I really hope we can clean up with capabilities, but I'm not yet sure we'll have time for 16.04. It feels like way too many variations for something that should be internal to the system, and offered via an abstraction that we own.
niemeyer
reviewed
Nov 14, 2015
| @@ -168,8 +162,12 @@ func AddHWAccess(snapname, device string) error { | ||
| return ErrInvalidHWDevice | ||
| } | ||
| + if strings.Contains(snapname, "_") { |
niemeyer
reviewed
Nov 14, 2015
| + const securityProfile = `open | ||
| +write | ||
| +connect | ||
| +` |
niemeyer
Nov 14, 2015
Contributor
FWIW, I generally try to avoid formatting strings like this, and give preference to one of these because they are seemingly more readable as they hold the content aligned:
const s = `
a
b
c
`
func f() {
const s = "a\nb\nc"
const s = "" +
"a\n" +
"b\n" +
"c\n"
)
niemeyer
reviewed
Nov 14, 2015
| + fn := filepath.Join(dir, capName) | ||
| + newCaps, err := readSingleCapFile(fn) | ||
| + // its ok if the file does not exist | ||
| + if err != nil && os.IsNotExist(err) { |
niemeyer
Nov 14, 2015
Contributor
There are a couple of instances of this. FWIW, IsNotExist(err) implies err != nil. In general these functions are built to take nil into account, as it's handy in cases like this.
niemeyer
reviewed
Nov 14, 2015
| + | ||
| + for _, c := range []byte(s) { | ||
| + if strings.IndexByte(allowed, c) >= 0 { | ||
| + dbusStr += fmt.Sprintf("%c", c) |
mvo5
Nov 16, 2015
Collaborator
Is there a benefit for using string(c) here? I kind of like the symmetry between the if and the else part but if you think string(c) is more readable I'm happy to change it.
niemeyer
reviewed
Nov 14, 2015
| + | ||
| +// Generate a string suitable for use in a DBus object | ||
| +func dbusPath(s string) string { | ||
| + dbusStr := "" |
niemeyer
Nov 14, 2015
Contributor
We have some legroom as this isn't super performance sensitive, but ideally for building strings we use either []byte for something trivial, or a bytes.Buffer most of the time. Building it with strings implies paying the cost of strings being immutable.
mvo5
Nov 16, 2015
Collaborator
Indeed, good point. I fixed this to use a byte buffer now, please let me know if there is anything else I should consider or if the new version is ok (and more "go-ish").
niemeyer
reviewed
Nov 14, 2015
| - logger.Noticef("Failed to read %q: %v", fn, err) | ||
| +// TODO: once verified, reorganize all these | ||
| +func (sa *securityAppID) appArmorVars() string { | ||
| + aavars := "\n# Specified profile variables\n" |
mvo5
Nov 16, 2015
Collaborator
Thanks, I changed that code to use a single fmt.Sprintf()instead of multiple string concats. If thats not sufficient I'm happy to change to use byte.Buffer (but it seems fmt.Sprintf is using a byte buffer internally already.
niemeyer
reviewed
Nov 14, 2015
| + prefix := findWhitespacePrefix(t, "###POLICYGROUPS###") | ||
| + for _, line := range p { | ||
| + if len(line) == 0 { | ||
| + aacaps += line + "\n" |
niemeyer
reviewed
Nov 14, 2015
| + for _, service := range m.ServiceYamls { | ||
| + err := service.generatePolicyForServiceBinary(m, service.Name, baseDir) | ||
| + if err != nil { | ||
| + foundError = err |
niemeyer
Nov 14, 2015
Contributor
Same as the other note: this will hide all errors but the last one.
mvo5
Nov 16, 2015
Collaborator
Indeed, I was wondering about this too but forgot to ask @jdstrand what the right behavior is here. I guess it should simply fail on the first error instead of tryint to generate security policies(?).
niemeyer
reviewed
Nov 14, 2015
| + if _, ok := p.(*SnapPart); !ok { | ||
| + continue | ||
| + } | ||
| + basedir := p.(*SnapPart).basedir |
niemeyer
Nov 14, 2015
Contributor
part, ok := p.(*SnapPart)
if !ok { continue }
... filepath.Join(part.basedir, ...)
In our ideal world, we'll have an InstalledSnap that gives access to its base directory via a public API.
mvo5
Nov 16, 2015
Collaborator
Thanks! I fixed that double type assertion. And we have a "part.Dir()" in the squashfs branch iirc (and if not we will add it :)
niemeyer
reviewed
Nov 14, 2015
| + err := svc.generatePolicyForServiceBinary(s.m, svc.Name, s.basedir) | ||
| + if err != nil { | ||
| + logger.Noticef("Failed to regenerate policy for %s: %v", svc.Name, err) | ||
| + foundError = err |
niemeyer
Nov 14, 2015
Contributor
A bit strange that it continues but only the last error is tracked. Makes it look like the problem was smaller than it actually was. One option in these cases is to build an error value which is an aggregation of all found errors. Please ping me for a real example doing that if you're interested.
mvo5
Nov 16, 2015
Collaborator
Thanks! Same question as above for @jdstrand if we should rather fail here on the very first error (and if not, what is the benefit to continue)?
|
Okay, I admit partial defeat, as I don't want to block you on this anymore. The branch is big, dense and dependent on external logic (apparmor, seccomp details), which makes it hard to keep tracking in my mind the most relevant detail which is whether the logic works or not.. so please feel free to merge this once you're happy. Having more time using it will be much more useful. |
mvo5
added some commits
Nov 16, 2015
|
Thanks a lot @niemeyer for the review. I agree its a tricky branch. I think once we resolved the two open questions about the error handling with @jdstrand then we can merge this one and iterate on it to make it nicer. The subsequent branches should be easier to review as they will not introduce radical changes like this one (which effectively reimplements most of aa-clickhook). |
mvo5
added some commits
Nov 16, 2015
|
retest this please |
|
retest this please |
added a commit
that referenced
this pull request
Nov 17, 2015
mvo5
merged commit eb45d7b
into
snapcore:master
Nov 17, 2015
added a commit
to zyga/snapd
that referenced
this pull request
Nov 7, 2016
mvo5 commentedNov 4, 2015
This branch implements native apparmor and seccomp generation from snappy itself. Once this branch lands we can remove aa-clickhook from the image. Its unfortunately huge. It is split up:
feature/native-security-policygen
feature/native-security-policygen-add-hwaccess
feature/native-security-policygen-compare
so I could split it into smaller chunks, but I'm not sure this helps much as the first branch is already pretty big :/
Anyway, early feedback would be great. I will also ask Jamie to see if there is anything missing. Please do not merge before Jamie had a chance to have a look.