interfaces,interfaces/apparmor: convert apparmor code to SecurityBackend

[zyga]

This branch converts the older apparmor support code into something that looks like the new SecurityBackend. It gains a lot of features, most notably, simplicity of the high-level interface,
but also support for old-security and much better test coverage.

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

[niemeyer]

Importing two previously unnecessary packages here just so it can "rename" a function feels like unnecessary boilerplate, in principle.

[zyga]

I replaced that with direct call to SecurityTag()

[niemeyer]

Nitpick: the developerMode bool should come right after snapInfo, as it's closely associated with it. This will also fix the inconsistency between Configure and CombineSnippets in that regard.

[zyga]

Yeah, makes sense, changed.

[niemeyer]

All these methods feel like unnecessary boilerplate. They're just referring to our own content elsewhere, so why not using the original content instead of giving new names that needs more source code and brain space to keep track of.

[zyga]

The reason those methods exist is that I'd like to create a common backend base that then uses them as implementation helper. I don't know if you consider this as worth the effort.

For clarity, the common backend base would then implement Configure() / Deconfigure() once, having access to SecuritySystem(), Directory() and a few others.

[niemeyer]

We discussed this API last week.

[zyga]

We discussed this but this doesn't change that there's something to share among the existing implementations. I you think the mild copy paste there is not something we should fix I can remove those methods.

EDIT: FYI, I'm not arguing on the public high-level interface. This is purely about the private interface that could be shared among all the current backends.

[niemeyer]

Why do we need this here? Doesn't feel like it's doing anything relevant.

[zyga]

So that we can mock apparmor parser away in a consistent way. Upcoming udev backend also uses the same method. I can just mock it directly in tests if you prefer it that way.

[niemeyer]

Yes, please. It has no reason to be in export_test.go, has much more logic than a trivial call to MockCommand, and is less flexible since as soon as you want more than exit 0 this won't work.

[zyga]

Removed in #763

[niemeyer]

Why isn't that simply

var defaultTemplate = `
...

?

[zyga]

I wanted to keep the real template, that cannot be mocked, in a constant for reference.

[niemeyer]

This const is not ever ever used besides to assign to defaultTemplate. No benefit.

[zyga]

Changed

[niemeyer]

Both this and the above function claim to offer an app-specific security tag, and they disagree about how it should look like.

We also need to get rid of WrapperNameForApp. This is not the place this convention should be defined.. (we're handling interfaces here).

[zyga]

SecurityTagForApp and everything else that doesn't use snap.Info is deprecated. It gets removed with the last backend.

[niemeyer]

Please replace the docs for both functions with:

// DEPRECATED: Remove after backends are converted to SecurityBackend.

[zyga]

Ack, done.

[niemeyer]

This for loop needs real love so it becomes readable.

The whole content should be out of this function and into the top-level, such complex types should not be defined inline, the slice should be out of here as well, the strings should have their content de-dupped, etc.

Let's please be a bit more caring about how our code looks.

[zyga]

I did my best to simplify this but I kept the strings as there are sadly each unique. Have a look if I can make anything else better.

[niemeyer]

This string should be defined only once and reused, instead of inlining it everytime. We should also avoid using such strings that break the indentation of the function. It makes reading much harder.

[zyga]

Done

[niemeyer]

Please define these strings only once and out of the function.

[zyga]

I did a pass that removes most of those but actually leaves this pair and a similar (but swapped) pair in TestUpdatingSnapTOOneWithFewerApps since they are unique to those tests. I'll do a second pass that makes those shared as well.

[zyga]

Done

[niemeyer]

What about killing FileName and ProfileName and using a single function everywhere?

[zyga]

That's fine too. I was trying to be explicit. If we don't want to ever use different profile name and different file name those can be merged.

[niemeyer]

"Ever" is a very long time that goes past our lives. There are three functions here: SecurityTag, ProfileName, FileName. Why do we have three functions that do nothing but return the same value that has exactly the same purpose, and that would actually break logic if diverged? (!!!)

[zyga]

FileName and ProfileName can^H could change separately. I don't argue that they should, I'm just explaining what the reasoning was behind this.

EDIT: given the coupling between profile name and filename where we keep the profile I'm now convinced. I'll get rid of the extra names.

[niemeyer]

Why is this not UnloadProfile(name), same as LoadProfile?

[zyga]

This part needs some cleanup/consideration of how/if we want to scan all loaded apparmor profiles. Earlier it seemed we do want to scan loaded profiles. This gave rise of the Profile struct. Now I'm not so sure. The only point of that struct is to convey profile name and enforcement mode together. If we don't need this information the API can be simplified.

[niemeyer]

The best is to go the opposite way, simplify first, make it complex if necessary.

[zyga]

Agreed.

[zyga]

I've simplified the API around this to work with simple strings.

[niemeyer]

That feels strange.. Where's the opening one? Why are we opening and closing in separate places?

[zyga]

The opening one is in the header. We could split this out but that would mean we'd have to parse the template more (template header is non-trivial). I don't think it's worth the effort given the complexity and the fact that just chopping the trailing } gets us what we want.

[niemeyer]

Okay.

[niemeyer]

The logic here is excessively wasteful: allocate a slice of slices to append slices to reallocate a slice to then join the slices. Let's please do it all in one shot: allocate once with the final length, write the final content into it.

[zyga]

Hmm, how can I do this in one go? I need to do have []snippet{header, rest..., footer} but this is not valid go syntax.

NOTE: The slice has the right capacity (so there is no re-allocation of s).

[niemeyer]

It's okay, I take that back. What you are doing is fine and I'm pedantic. Sorry.

[niemeyer]

First pass is done.. the change seems a good one, but it's a bit large, and consequently ended up with a largeish review. Sorry about that.

[zyga]

Thanks for the review. I replied to some of the comments inline. I'll do a pass at the code and ack all individual comments next.

[zyga]

This is ready for another review

[pedronis]

toggleProfiles now that is private?

[zyga]

Why toggle? It's not like this is always on-off back-forth kind of toggle.

[pedronis]

@zyga that was strawman, but observeChanges is really not ideal as a name, also why not have two functions one for the unload part and one for the load/reload

[zyga]

Yeah, I could do that. Good idea

[niemeyer]

Again, can we please kill the duplication in these snippets? The actual relevant difference in them is 5 lines, total! We don't need all 34 here. For instance:

content: commonPrefix + `
...

[zyga]

Ah, sorry I didn't think of this. Done

[niemeyer]

Thanks, much nicer.

[niemeyer]

These two functions should use bytes.Buffer values:

var buf bytes.Buffer
fmt.Fprintf(&buf, ...)
fmt.Fprintf(&buf, ...)
return buf.String()

(or return buf.Bytes(), depending on how that's used)

[zyga]

Done

[niemeyer]

Rather than building it just to call replace, can't we simply pass the arguments in as usual?

Same below.

[niemeyer]

Why are we working with strings above if the end goal is a []byte? Much easier to work with a bytes.Buffer all along and return the final bytes array with buf.Bytes(). You can even pass the bytes.Buffer into legacyVariables/etc for easily cooking the end result at once, instead of manipulating immutable strings everywhere.

[zyga]

Good point, I changed everything to use a buffer.

[jdstrand]

Minor nitpick-- the profile isn't attached to the process so much as the process is launched under the profile.

[zyga]

Reworded in 1be14a9

[jdstrand]

Please use "The header specifies the profile name that the launcher will use to launch a process under this profile."

[zyga]

Reworded in 7752275

[jdstrand]

I'm not sure why you are discussing binary attachment here, but since you are, you should say that snappy only utilizes the "abstract identifier".

[zyga]

Removed in earlier patches. I'm inclined to add the note about abstract identifier so that the next person that comes along to look at this won't have to do that much digging.

[zyga]

Edited in ec37dd4

[jdstrand]

The launcher doesn't care about /var/lib/snappy/apparmor/profiles because it doesn't touch the profile files-- it references the profile name of a profile that is already loaded into the kernel. ubuntu-core-launcher does care about the seccomp directory, /var/lib/snappy/seccomp/profiles, which is perhaps what you were thinking?

[zyga]

That's right, the path is only relevant to the systemd unit that loads those on boot. Corrected in 434301b

[jdstrand]

This is 'apparmor.service'

[zyga]

Spliced in 3992639 -- thanks

[jdstrand]

Also on install, right?

[zyga]

Yes though I didn't really mention that. This part is coming up next (~tomorrow-ish)

[jdstrand]

'was was'

[zyga]

Fixed in 4a5e1e5

[jdstrand]

/sys/class/ is perhaps brittle. I'm hoping this will go away with interfaces. The tunables/global and /tmp/ should be fine, but note if we change the spacing on '/tmp/ r,\n', it will fail the test (not a blocker, just pointing out it is brittle). A test for the ###PROFILEATTACH### replacement would be good though.

[jdstrand]

I see that the ###PROFILEATTACH### replacement tests are down below.

[zyga]

I'll keep this as-is as I hope we'll keep this up-to-date when working on the actual template text.

[jdstrand]

Why are this referred to as "legacy variables"? These are apparmor profile variables that are still in use in our policy.

[zyga]

Because they originate from 15.04 and we'll probably simplify things for 16.04 so that we use just the concepts of snap name, app name and security tag. This is still open for discussion, I just included it here. As a second reason "legacy variables" are always used for old-security since it has to be compatible with earlier releases.

[jdstrand]

Why "legacy" here? These variables are in use by interfaces/apparmor/template.go and we should continue to use them to keep the implementation clean and make maintenance easier.

[zyga]

Replied above.

[jdstrand]

I'm not sure why you are referring to old-security base templates here. AppArmor variables are "a good thing" regardless of using the old old-security system or the new interfaces system.

[zyga]

I was referring to old-security because those variables need to be maintained for old-security to work. Without old-security any set of variables can be used (AFAIK, perhaps I'm wrong) as long as they are changed in sync with template.go.

[jdstrand]

"I was referring to old-security because those variables need to be maintained for old-security to work. Without old-security any set of variables can be used (AFAIK, perhaps I'm wrong) as long as they are changed in sync with template.go."

I see, ok that context helps a lot. With old-security there needs to be coordination between ubuntu-core-security and snappy, without old-security, everything is contained within snappy (and as you said, coordinated with template.go).

[jdstrand]

APP_ID was previously defined as '<snap><appname><version>'. So the apparmor variables were:
APP_PKGNAME=<snap>
APP_APPNAME=<appname>
APP_VERSION=<version>

In this manner, APP_ID_DBUS was '<snap><appname><version>' translated to a dbus string, and APP_PKGNAME_DBUS was '<snap>' translated to a dbus string.

[zyga]

Thanks for explaining this

[jdstrand]

This seems simple but needs to be thought out and what variables are culled or added directly affects security policy and interactions. For example, APP_VERSION is removed in your straw man, but we previously said that apps should only have read access to other versions' data directories-- do we add APP_REVISION or revisit the decision on versioned data dirs? APP_*_DBUS is gone and would need to be replaced with something in the interface for dbus.

Let's leave all that for a future commit and proper discussion.

[zyga]

Agreed.

[jdstrand]

Introducing these now confuses policy since nothing in policy references these variables. I don't even know why we are exposes APP_SECURITY_TAG-- what is that? I suspect it is the profile name, but that is unneeded since apparmor already has a special @{profile_name} variable that the parser automatically creates.

I would prefer all but INSTALL_DIR be removed until this is properly choreographed with template.go and all the builtin/ policy.

[zyga]

I feel I should have left that part out to separate the discussion of what to have for 16.04 and what we have in the code today. Your remarks are valid.

[jdstrand]

In general I only suggested comment changes or clarified language. The one thing I'd like to see removed are all the modern apparmor variables in template_vars.go except INSTALL_DIR. We should instead coordinate changes to template_vars.go with template.go and builtin/* in the same commit.

Before we add modern variables and update the policy, I'm strongly in favor of focusing on getting the existing policy all working, applied and converting 15.04 frameworks to 16.04 interfaces first.

[zyga]

I've proposed all the comment changes in #765