[MIR] lxd-agent-loader

[Stéphane Graber]

Availability: Currently in universe
Rationale:
 LXD now supports virtual machines. In order for all features to work properly, an agent must
 be running in the image. As Ubuntu is a first class citizen in LXD, we'd like Ubuntu to ship
 with the integration bits needed to automatically integrate with LXD when running on such host.

 This package is made of two systemd units, both of which are keyed to only be considered if
 running on a LXD system (so they're always safe to have around and will not delay boot).

 The agent itself is loaded from the LXD host, guaranteeing that the same version is running
 in host and guest (required due to tightly coupled features/APIs). So this package will
 remain tiny and will most likely pretty much never need any updates.

Quality assurance:
 - No testsuite (doesn't contain anything but two systemd units)
 - No watch file (this is a native package) but debian/copyright includes details on where to find the source bits.

UI standards: Not a GUI package
Dependencies: none
Standards compliance: Complaint with current debian standards, lintian clean (including pedantic)
Maintenance: ~ubuntu-lxc owns this package (subscribed)
Background information: This is done ahead of seeding this package in the Ubuntu Cloud images
Security checks: No binaries included, systemd units only, setup to only trigger inside LXD VMs

[Mike Salvatore (mikesalvatore)]

I reviewed lxd-agent-loader 0.3 as checked into focal. This shouldn't be
considered a full audit but rather a quick gauge of maintainability.

lxd-agent-loader is just 2 systemd units. As such, this package does not really
have much of an attack surface to speak of. There's no code, just 2
configuration files. I see no reason why, from a security perspective, it should
be difficult to maintain 2 systemd units over the life of an LTS.

Security ACK for promoting lxd-agent-loader to main.

[Christian Ehrhardt  (paelzer)]

I'll do a MIR review on that later today or tomorrow morning

[Christian Ehrhardt  (paelzer)]

As with other such conditional services it is important that they won't consume cycles/memory in other places. But these are safe by having
   ConditionPathExists=/dev/virtio-ports/org.linuxcontainers.lxd

Thanks for that

[Christian Ehrhardt  (paelzer)]

I understand that for the purpose what it is supposed to be doing it has to has quite some capabilities. But essentially it is an externally controlled data (that you mount to a known place) that is then executed as-is.

I see that you have set the dependency to a "Requires" already which is good to only do that once the mount has worked. But I wonder if we should try to make this a bit more secure.
Also you chmod the path - all helpful to avoid people shoving other content there that is then executed.
But I was wondering if you happen to have a defined set of "things this agent will do". If so it would be great if it could use other users, private-directories and reduced capabilities limited to just the amount it needs. That would further restrict the potential mis-use.

I've seen you had a security pre-review already and Mike was fine, never the less I'd appreciate if you could consider further confinement for this in the long run.

[Christian Ehrhardt  (paelzer)]

Time for the formal review ...

[Summary]
The package is small and clean, the function is clear.
I'd be tempted to wonder about security, but you already have a security Ack.
Therefore I'm MIR-Acking this as well, as formally it seems fine to me.

There are still a few TODOs (not gating the MIR):
- further confinement would be nice to have
- are there tests anywhere that would e.g. catch if systemd changes?
  If yes please point to them, if no could you consider adding some?

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
- no -dev/-debug/-doc packages that need exclusion

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking

[Security]
OK:
- history of CVEs does not look concerning
- does not use webkit1,2
- does not use lib*v8 directly
- does not parse data formats
- does not open a port
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)

Problems:
- does run a daemon as root and executes externally controlled content
  I have outlined that in comment #4 already.
  Any further confinement that you could add which does not break your
  intended use case would be helpful.
- Empty CVE history for a package a few days old doesn't actually count :-)

[Common blockers]
OK:
- does not FTBFS currently
- The package has a team bug subscriber
- no translation present, but none needed for this case (user visible)?
- not a python package, no extra constraints to consider int hat regard
- no new python2 dependency

Problems:
- does not have a test suite that runs at build time
- does not have a test suite that runs as autopkgtest
=> I assume you exercise that in LXD tests somewhere?

[Packaging red flags]
OK:
- Ubuntu does not carry a delta
- Ubuntu does carry a delta, but it is reasonable and maintenance under control
- symbols tracking not applicable for this kind of code.
- d/watch is present and looks ok
- Ubuntu update history is too short to evaluate, but it is owned by one
  of our teams so it should be fine
- the current release is packaged
- promoting this does not seem to cause issues for MOTUs that so far
  maintained the package
- no massive Lintian warnings
- d/rules is rather clean
- Does not have Built-Using

[Upstream red flags]
OK:
- no Errors/warnings during the build
- no incautious use of malloc/sprintf (as far as I can check it)
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of user nobody
- no use of setuid
- no important open bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- no embedded source copies
- not part of the UI for extra checks

[Stéphane Graber (stgraber)]

 - "further confinement would be nice to have"
This service is used to implement both the "lxc file" set of commands and the "lxc exec" set of commands. As such it needs to be able to read and write every file on the system and must be allowed to spawn unconfined commands. I don't see how either of those can be implemented if we were to confine this. sshd is similarly not confined and that's pretty much the realm in which this agent is in.

 - "tests to detect changes"
We do have daily tests of all our VM images which do exercise the agent. Those are run on the upstream Jenkins and will typically flag any systemd changes months before they hit Ubuntu (because of other distros being more aggressive on updating systemd).
https://jenkins.linuxcontainers.org/job/lxd-test-images/restrict=master,type=vm/ (currently failing due to issues on older distros which we're expecting to fix today/tomorrow)

 - "Empty CVE history for a package a few days old doesn't actually count"
The agent binary itself has been in LXD since September and has been widely used, so while someone wouldn't have filed a CVE against "lxd-agent-loader" (and we never expect to have one on that), they would have filed one against lxd itself as the project shipping the lxd-agent binary.

 - "does not have a test suite that runs at build time"
 - "does not have a test suite that runs as autopkgtest"
Testing the agent is pretty tricky because you need to run the tests on a physical host (nested VMs don't work due to vsock address conflicts), so this effectively excludes the build and test environments used in Ubuntu. As mentioned above though, we do have daily testing of the agent on all distros that we have images for. Those run on dedicated physical hardware in the upstream CI environment.

[Stéphane Graber (stgraber)]

https://code.launchpad.net/~stgraber/ubuntu-seeds/+git/ubuntu/+merge/381171

[Christian Ehrhardt  (paelzer)]

Thanks for the extra explanations Stéphane.
Yeah I agree if it does file and exec there isn't much you can do to confine it :-/

But hey I was +1 already and only suggesting, so we are good.

[Stéphane Graber (stgraber)]

Promoting to main now before I can update ubuntu-meta for the seed change:

Override component to main
lxd-agent-loader 0.3 in focal: universe/admin -> main
lxd-agent-loader 0.3 in focal amd64: universe/misc/optional/100% -> main
lxd-agent-loader 0.3 in focal arm64: universe/misc/optional/100% -> main
lxd-agent-loader 0.3 in focal armhf: universe/misc/optional/100% -> main
lxd-agent-loader 0.3 in focal i386: universe/misc/optional/100% -> main
lxd-agent-loader 0.3 in focal ppc64el: universe/misc/optional/100% -> main
lxd-agent-loader 0.3 in focal s390x: universe/misc/optional/100% -> main
Override [y|N]? y
7 publications overridden.

[Stéphane Graber (stgraber)]

And uploaded the updated ubuntu-meta.

Marking Fix released as the package is now in main.