interfaces: generate real security file content

[zyga]

This patch builds on the existing foundation to generate non-fake
content of all the files related to security.

The internal security helper interface is extended to contain additional
data for any snap (version, origin and list of apps). This is required
to construct correct content and to locate it in the right directories.

The "xmanager" (read as cross-manager) file is a temporary helper before
all of this moves to the state engine and we can query the state
directly from there. It only exists so that we can ask about the
version, origin and list of apps in a given snap.

Tests are somewhat tweaked since now the real contents of the security
files is very long, a set of mocking functions is provided to replace
the real (very long) seccomp and apparmor header with a shorter version
that is easier to analyze and compare.

Finally, the repository method that writes all the security files (which
is also supposed to move to the interface manager) is extended to pass
the new arguments.

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

[zyga]

@jdstrand can you please review this?

[jdstrand]

Can we remove CLICK_DIR? It isn't used anywhere any more.

[zyga]

Sure, now or in next commit/branch?

[jdstrand]

On Thu, 2016-03-10 at 10:51 -0800, Zygmunt Krynicki wrote:

• header = strings.Replace(header, "###WRITES###", "", 1)
• return []byte(header)
  +}

+func (aa *appArmor) varsForApp(snapName, snapVersion, snapOrigin,
appName string) string {

• return fmt.Sprintf(+# Specified profile variables +@{APP_APPNAME}="%s" +@{APP_ID_DBUS}="%s" +@{APP_PKGNAME_DBUS}="%s" +@{APP_PKGNAME}="%s" +@{APP_VERSION}="%s" +@{INSTALL_DIR}="{/snaps,/gadget}" +# Deprecated: +@{CLICK_DIR}="{/snaps,/gadget}" +,
  Sure, now or in next commit/branch?

Whichever is easiest.

Jamie Strandboge | http://www.canonical.com

[jdstrand]

If these aren't going to be used anymore, perhaps instead of setting these to empty strings they should be removed from interfaces/apparmor.go. I would be ok with that happening in a separate commit.

[zyga]

Sure, will do. Added to TODO list :)

[jdstrand]

dbusPath here is a near code copy of dbusPath in ./snappy/security.go. What are the plans for snappy/security.go? I'm guessing it needs to stick around for old-security/security-policy and old-security/security-override? The former is going to need the same sorts of apparmor variables so shouldn't dbusPath() be refactored? I imagine there is other similar code that could be refactored in snappy/security.go and interfaces/security.go surrounding old-security and interfaces.

[zyga]

snappy/security.go will be killed IMHO (with the grand switch over to interfaces tomorrow-ish/next-week-ish)

[jdstrand]

Likewise for this CLICK_DIR

[jdstrand]

Likewise for this CLICK_DIR

[jdstrand]

Note, this isn't a valid seccomp profile entry. It would be nicer to have:
"# Mocked seccomp header\n"
"open\n"))

[zyga]

Oh, thanks, I'll correct that.

I got fooled by "deny open" syntax elsewhere.

[jdstrand]

FYI, 'deny kexec' is correct so this one doesn't need to change.

[jdstrand]

Functionally, I'm not seeing that this branch is used. I tested by modifying ./interfaces/security.go to include an extra comment in varsForApp(), then I built and installed hello-world with 'sudo ./snappy install hello-world', but I didn't see the extra comment in /var/lib/snappy/apparmor/profiles/hello-world*. Note, hello-world does not use old-security, so I was expecting this branch to generate the profiles.

Is this branch supposed to do that?

[zyga]

@jdstrand yes, this branch is supposed to bring more parts before we glue them together. None of this code is live yet, you can only observe it via unit tests.

[zyga]

I'm merging this as-is. We'll work on tweaks to things like profile names so that's everything is compliant with the requirements of the security team but I'd like to move forward.

[mvo5]

Sorry, I think it was not ideal that this got merged without a (go) code review, I don't see a +1 on this branch in the comments(?). There are some points that are worth looking at, e.g. the activeSnapMetaDataImpl is using return without parameters (relying on the named return part) which we generally avoid in snappy because it is considered harder to read. I understand that code review is slow sometimes and that it can feel like slowdown, but I think it would be better to ping on irc or telegram about stale branches. I will do a retrospect review on this next.

[zyga]

Hey @mvo5, I merged it because it was getting stale and the key here was the security side of the review which we did do. The temporary method you refer to will be removed before this thing is actually used (the method was pulled from a demo code I wrote a month ago and the design has evolved since).