Binary with an underscore fails to produce an apparmor profile

[Brandon Schaefer]

If you create a binary with an underscore in its name, then snappy build/install the snap. Everything works just fine. Execpt it doesnt build its apparmor profile. A binary with an underscore in it causes aa-clickhook to fail. As it splits the binary on '_' and expects 3 split words. Any underscores will cause that assumption to fail.

Though it does say in the snappy docs thats a '_' is an unsupported character, it should still give an error while building vs producing a valid snap installing 100% and then just not working.

Example binary in package.yaml that will cause the error in a profile not being produced:
binaries:
 - name: bin/document_viewer
   description: "ubuntu core app document viewer"
   start: ./bin/document_viewer
   security-template: unconfined

[Mark Shuttleworth (sabdfl)]

Good catch.

Please can we make sure we use the same code for validation of names at
build and install time? Or at least an common test suite?

Thanks,

Mark

[John Lenton (chipaca)]

A binary with an underscore in the name is rejected by the review tool,

Errors
------
 - lint_hooks_env_what_valid
 malformed application name: 'env_what'
hello-world_1.0.17_all.snap: FAIL

the review tool is optional (and even if installed right now it isn't run as part of `build`; it was disabled when we were moving too fast for it to keep up), but the store will reject things based on it. While there are some very basic sanity checks inside `build`, I'm not sure we want to implement more there unless we're moving to that and away from the review tool itself; maintaining all the checks in both places would be a lot of duplicated effort.

We should probably re-enable running it on build, though.

[Alexander Sack (asac)]

there are two things in here:

 1. we should make snappy build fail instead of producing packages that cannot be installed.
 2. we should support _ in binary names. Don't see a reason why not right now.

Would suggest to 2. in a new bug.

on 1. i would suggest that review tools clearly separate tests that check if package is valid to install from those that implement our store policy (e.g. apps with changes to sandbox don't get auto approved).

[Oliver Grawert (ogra)]

well, we should instead start supporting binaries with underscores :)

there are definitely plenty of them out there ...
while enabling the tools again might help catching issues in the yaml, this particular issue should still be fixed at its root instead of shielding it by checks though ...

(probably worth opening a separate bug for enabling the review tools again so we can track it (IIRC this is still waiting for better readable error messages in the tools with more descriptive suggestions for fixing the catched errors)

[Mark Shuttleworth (sabdfl)]

On 02/07/15 11:37, Alexander Sack wrote:
> 2. we should support _ in binary names. Don't see a reason why not right now.

No, we won't support _ in package names, it is poor taste. - is fine.

Mark

[Oliver Grawert (ogra)]

@mark ... this is about executable binaries that have an underscore in their name, not about package names ...
i don't think we can enforce such a thing for this usecase ...

[Mark Shuttleworth (sabdfl)]

Ah, OK, I misunderstood. Yes, we must allow _ in binary names, just not
in package names.

Mark

[Jamie Strandboge (jdstrand)]

This yaml (taken from the bug description) isn't valid snappy yaml according to meta.md:
binaries:
 - name: bin/document_viewer
   description: "ubuntu core app document viewer"
   start: ./bin/document_viewer
   security-template: unconfined

Apart from the '_' in the name filed, it is using 'start' with 'binaries'. 'services' already supports '_' for the binary name specified in the 'start' field. Eg, this is valid:
services:
 - name: document-viewer
   description: "ubuntu core app document viewer"
   start: ./bin/document_viewer
   security-template: unconfined

For binaries, we also currently support '_' for the binary specified in the 'exec' field. Eg, this is also valid:
binaries:
- name: document-viewer
   description: "ubuntu core app document viewer"
   exec: ./bin/document_viewer
   security-template: unconfined

However, because 'exec' in 'binaries' is considered optional, there is the problem area:
name: foo
version: 1.0
binaries:
- name: ./bin/document_viewer
   description: "ubuntu core app document viewer"
   security-template: unconfined

With this yaml, snappy transposes './bin/document_viewer' to 'document_viewer' when creating the APP_ID (https://wiki.ubuntu.com/AppStore/Interfaces/ApplicationId). The APP_ID uses '_' as a delimiter, and the APP_ID is used to differentiate between 'services' and 'binaries' shipped within a snap for the systemd service definitions, cli binaries and apparmor profiles. As mentioned, the review tools will catch this (though they are currently disabled-- they're caught up now and could be re-enabled).

Historically (ie Touch), there was a clear distinction between package names and binary executable names. It was decided that '_' would not be supported in package names, app names and versions, because, like Mark said, it was considered poor taste and because it wasn't an allowed character in Debian versioning schemes anyway. '_' was then agreed upon as the delimiter for the APP_ID in part because it is cli-friendly (no shell-escapes needed). Fast forward to today on snappy and the '_' is still the delimiter for the APP_ID and this all works fine for 'services' yaml and 'binaries' yaml that specifies 'exec'. With 'binaries' without 'exec' though, the binary executable is being used as the 'app name' part of the APP_ID and as we've seen, the transposition that snappy does isn't accounting for the '_' in the 'name' field of 'binaries' that don't specify 'exec'.

The easiest and least regression-prone fix is to make snappy build fail/review tools error when specifying '_' in the name field when not using exec, but to then make a helpful suggestion to either use 'exec' to specify the binary or rename the binary. If after making this change it is felt that it is not enough, we can explore other options. Eg, snappy could be made to transpose the '_' of the name field into something else which is doable but more regression-prone since anything parsing the package.yaml to obtain the APP_ID would have to do the same transposition. It is also possible to change the definition of the APP_ID, but this is very regression-prone when considering Ubuntu Personal (where the APP_ID is used by various trusted hel...

[Brandon Schaefer (brandontschaefer)]

The issue for the '_' was the assumed 3 word split on a '_' right? So then cant we just move to make it so a split on a binary would use split[0], split[1] + ... + split[n-1], split[n]? This way we still get the 3 strings needed, but we support a arbitrary number of split words produced by splitting on '_'?

[Jamie Strandboge (jdstrand)]

That falls into the category of redefining the APP_ID, which is doable, but as mentioned, there is a lot that uses the APP_ID and making a change like this needs to be carefully researched, planned, coordinated and executed (especially when considering the Touch move to Ubuntu Personal). The APP_ID concept is part of application lifecycle, trusted helpers (eg, online accounts, media-hub, mediascanner, thumbnailer, ...), scopes-runner, unity8, systemd services, content-hub, snappy binaries, UAL, ubuntu-core-launcher, the store, and security policy (this list is certainly not comprehensive). The APP_ID is (currently) rigidly defined as <pkgname>_<appname>_<version> where '_' is not allowed in <pkgname>, <appname> or <version> and things are implemented using this definition. Since we already have a mechanism for people to work within the (currently) defined system (ie, use 'exec' in the 'binaries' yaml), why not just make it easy for people to use this mechanism (ie, my suggested option)? If people feel strongly that the APP_ID should be redefined, I think the discussion should be raised to the architects team.

[Jamie Strandboge (jdstrand)]

I might not have connected a dot or two on this, so let me mention a couple of other things. Obviously, it is easy fixing aa-clickhook to handle an extra '_' -- apparmor userspace and the kernel certainly doesn't care (the seccomp policy generation would also need to be adjusted similarly). However, other things are affected by this seemingly simple change. Since the apparmor profile name would now allow '_' in the appname, different trusted helpers that look at the the connecting process may parse the profile name in various ways and their parsing would need to be verified and potentially adjusted. Unity8 and UAL application lifecycle would have to be examined since the APP_ID is derived from the package's packaging and it needs to have the APP_ID match the generated profile name. Snappy would have to be adjusted to make sure that the generated systemd units and bin-path can handle the new apparmor profile name. ubuntu-core-launcher looks at the package.yaml to derive the APP_ID and it too needs to have the APP_ID match the generated profile name. The profile name or some subset of it may be used as a key in a trust database (eg, online accounts, trust-store, ...) that may have to be reworked. Scopes would need to be looked at to make sure they are using the correctly named app-specific endpoints. Similarly, apps on Ubuntu Touch have some app-specific directories on the filesystem that use the appname (specifically, scopes, online accounts and QML caching)-- these services and/or policy would have to be revisited to make sure they are doing the right thing with an optionally additional '_'.

Again, it is possible, just a lot of details to get right (hence getting architects' team involvement on if it is worth it and if so, direction on the implementation).

[Jamie Strandboge (jdstrand)]

Whoops, this is wrong: "ubuntu-core-launcher looks at the package.yaml to derive the APP_ID" - that is all handled by snappy autogeneration of the systemd units and bin-path (sorry, meant to delete that before clicking 'Post comment').

[Jamie Strandboge (jdstrand)]

> 1. we should make snappy build fail instead of producing packages that cannot be installed.
...
> on 1. i would suggest that review tools clearly separate tests that check if package is valid to install from those that implement our store policy (e.g. apps with changes to sandbox don't get auto approved).

In general, the tests are separated in this manner-- there are 'lint' tests, 'security' tests, etc in different files. For example, in this case, the test that failed is a lint test (eg, from John's example, "lint_hooks_env_what_valid malformed application name: 'env_what'").

Also note, I think it is vitally important for snappy build to produce correct binaries. I also think snappy install should guard against malformed input (especially since it is run as root! :), but I also think the review tools are an important gateway to block crafted snaps from ever hitting devices. Regardless of if '_' should be valid or not, IMO the review tools should be re-enabled in snappy build and there should be a commit policy for snappy that does a 'snappy build' for any changes to yaml and a corresponding MP to the review tools as needed. I discussed this previously in meetings on IRC-- there is agreement on the approach and this work is already started and in a trello card.

[Leo Arias (elopio)]

The review tools are still disabled, but that's tracked in bug #1466241.

Nobody seemed to argue against the workaround of suggesting the user to add the exec, so I think that's the way to go.

@Jamie, are the review tools now suggesting this when they find an underscore?

[Zygmunt Krynicki (zyga)]

This bug is from the 15.04 era when snapd was significantly different. Currently apps cannot use underscore (_) in the application names and this is properly validated.