[MIR] lxd

This depends on the golang portion of bug #1267393 *or* depends on lxd being built with gccgo on all arches.

All the administrative stuff (builds fine, has team bug subscriber, no bugs, etc) seem fine minus a comment below. I'm sure it will be well maintained, since Canonical is looking after it. But this runs a daemon and is security sensitive, so I'm going to have the security team look over code and how we set up the users/groups in postinst.

As for the problem I mentioned earlier: why aren't the tests run? I see why you might not be able to run them in debian/rules, but they could run as a dep8 test surely?

Assigning this to ubuntu-security for a review, since lxd itself runs a root daemon full of potential scary, and also to speak on their current stance on the golang issue.

This is also embedding libyaml and sqlite in the binaries. This is another Go-links-statically-which-is-weird-on-ubuntu issue. Which I'll defer again to the security team's enthusiasm for maintaining an embedded parser and database.

As of LXD 0.17, we now have autopkgtest which runs the full testsuite.

FYI, this is queued behind bug #1267393 which we are looking at now. No one from the security team has looked at LXD yet, but you might want to take a look at this comments to get ahead of the review:
https://bugs.launchpad.net/ubuntu/+source/juju-mongodb/+bug/1267393/comments/62
https://bugs.launchpad.net/ubuntu/+source/juju-mongodb/+bug/1267393/comments/63

This comment is mostly a note-to-self...

The current plan is to add lxd, lxd-client, lxd-utils and nova-compute-lxd to supported-misc-servers seed and add lxd and lxd-client to cloud-image seed. That to-do is on me, and I'll do this as soon as the MIR is complete.

I reviewed lxd version 0.18-0ubuntu2 as checked into wily. This should not
be considered a full security audit, indeed I reviewed significantly less
code than I would have liked due to time constraints.

This is a much larger codebase than I anticipated; it's much more than
"just" wrappers around lxc libraries. The vendorized code common to Go
programs also drastically contributes to the code size and it's quite
difficult to see which portions may be important to the lxd package.
Breaking apart the dependencies would help future reviews.

That said, I did read through the entire API spec (not the implementation,
sadly) and have only limited feedback to give. lxd appears to sidestep
many of the problems that have plagued OpenStack's Glance by not having
any tenants.

The simplicity in the end-user interface is amazing and frankly I wish
lxd would grow support for qemu/kvm as well. No existing VM management
system is anywhere near this elegant.

The one issue I found that I felt required a CVE was handled very well.

The shell scripts feel poorly-reviewed; shellcheck found many issues, the
most troubling are summarized below.

I think the project (and package) would benefit from a pruning phase; I
have the feeling that some things were simply re-implemented because it
was easy, or existing pieces reused because they already existed, but
dragged along their own dependencies. I think a week or two of looking at
the entire package sources with an eye towards removing what could be
removed would be a benefit to the project.

Go seems an odd choice for a systems programming language; much of its
design seems contrary to working directly with POSIX-defined interfaces.
There seemed to be a lot of code dedicated towards handling the boundaries
between the C world and the Go world and while it read much like other
interfaces (e.g. Python modules implemented in C, Erlang NIFs) it does
seem like there's a lot of ancillary code compared to application code.

Some more specific comments:

- loads of vendorized go code, needs to package dependencies, see
  http://launchpad.net/bugs/1267393 comments 62, 63, 68
- I'm scared of the code to automatically add admin users to lxd's
  interface. It seems dangerous compared to "sudo adduser sarnold lxd ;
  newgrp lxd".
- "lxc config set core.trust_password SECRET" -- this exposes password via
  ps, top, etc; examples should use "lxc config edit core.trust_password"
  instead
- idmapset.go ToLxcString() outputs a different format than parse() reads
- getFromMap() enforces UTF-8 encoding on username. How does this handle
  invalid UTF-8 in /etc/subxid files? Do any other utilities enforce UTF-8
  formatting of this file? Which other utilities may perform case folding?
  Do they all perform UTF-8 case folding in this fashion?
- POST to /1.0/images conflates fingerprint with alias -- I think
  fingerprint should always be included for integrity regardless of alias
  presence or absence
- POST to /1.0/certificates has multiple conditionals with "friendly"
  fallbacks. I think this is a mistake, I think the client should supply
  the certificate and name without friendly fallbacks to using the current
  connection's param...

On Sat, Oct 10, 2015 at 04:42:46AM -0000, Seth Arnold wrote:
> I reviewed lxd version 0.18-0ubuntu2 as checked into wily. This should not
> be considered a full security audit, indeed I reviewed significantly less
> code than I would have liked due to time constraints.
>
> This is a much larger codebase than I anticipated; it's much more than
> "just" wrappers around lxc libraries. The vendorized code common to Go
> programs also drastically contributes to the code size and it's quite
> difficult to see which portions may be important to the lxd package.
> Breaking apart the dependencies would help future reviews.
>
> That said, I did read through the entire API spec (not the implementation,
> sadly) and have only limited feedback to give. lxd appears to sidestep
> many of the problems that have plagued OpenStack's Glance by not having
> any tenants.
>
> The simplicity in the end-user interface is amazing and frankly I wish
> lxd would grow support for qemu/kvm as well. No existing VM management
> system is anywhere near this elegant.
>
> The one issue I found that I felt required a CVE was handled very well.
>
> The shell scripts feel poorly-reviewed; shellcheck found many issues, the
> most troubling are summarized below.

I didn't know of shellcheck, I'll be sure to run it against the LXD
shell scripts (basically just the lxd-bridge init script + the LXD
testsuite) and fix anything that seems relevant.

> I think the project (and package) would benefit from a pruning phase; I
> have the feeling that some things were simply re-implemented because it
> was easy, or existing pieces reused because they already existed, but
> dragged along their own dependencies. I think a week or two of looking at
> the entire package sources with an eye towards removing what could be
> removed would be a benefit to the project.

We have some of that planned (see below).

> Go seems an odd choice for a systems programming language; much of its
> design seems contrary to working directly with POSIX-defined interfaces.
> There seemed to be a lot of code dedicated towards handling the boundaries
> between the C world and the Go world and while it read much like other
> interfaces (e.g. Python modules implemented in C, Erlang NIFs) it does
> seem like there's a lot of ancillary code compared to application code.

I can certainly agree with at least some of that. Though ultimately the
choice of programming language wasn't up to us and it's not something we
can really change at this point.

Since LXD is built on a fair amount of C libraries (which is unusual for
Go projects), we do have a fair amount of binding code (go-lxc,
utils_linux.go, ...), it's pretty repetitive code which is usually
pretty self-contained.

The rest tends to be pure Go as is the norm in this ecosystem...

> Some more specific comments:
>
> - loads of vendorized go code, needs to package dependencies, see
> http://launchpad.net/bugs/1267393 comments 62, 63, 68

We have the list of those, a good bunch are already packaged, we'll have
to package at least two new ones and move away/replace a few more.

> - I'm scared of the code to automatically add admin users to lxd's
> interface. It ...

Launchpad looks confused by this e-mail reply, it's far longer than what the web interface shows, so you'll have to click on the message title to get the raw version of it.

On Sun, Oct 11, 2015 at 02:43:06AM -0000, Stéphane Graber wrote:
> I didn't know of shellcheck, I'll be sure to run it against the LXD
> shell scripts (basically just the lxd-bridge init script + the LXD
> testsuite) and fix anything that seems relevant.

Stéphane gave me this on irc: //github.com/lxc/lxd/pull/1208

> > - I'm scared of the code to automatically add admin users to lxd's
> > interface. It seems dangerous compared to "sudo adduser sarnold lxd ;
> > newgrp lxd".
>
> That code was basically copy/paste from what's being done in libvirt and
> was a requirement to have a good first user experience (though the
> newgrp part is still annoying...).

Ah, then I also don't like the same code in libvirt. While it might be
strictly true that having 'libvirt' or 'lxd' group is morally equivalent
to being a full administrator, and vice-versa, I still don't care for it.

> > - "lxc config set core.trust_password SECRET" -- this exposes password via
> > ps, top, etc; examples should use "lxc config edit core.trust_password"
> > instead
>
> Note that the "set" command as since grown support for reading from
> stdin, so "echo password | lxc config set core.trust_password -", or
> directly reading from a file (to avoid echo showing up), would now work.

Nice, sounds useful for other tools too.

> > - idmapset.go ToLxcString() outputs a different format than parse()
> reads
>
> I'd have to re-check the code, but I'm assuming parse() is meant to
> parse the /etc/sub{uid,gid} format, whereas .ToLxcString outputs a
> string compatible with lxc's lxc.id_map option.

Yes; it just seems unfortunate to have two formats for one thing, and the
inability to feed the output into the parser is odd. (See for example
xmodmap(1) -pke -- who hasn't looked at that and thought that it's strange
and odd?)

> > - getFromMap() enforces UTF-8 encoding on username. How does this handle
> > invalid UTF-8 in /etc/subxid files? Do any other utilities enforce UTF-8
> > formatting of this file? Which other utilities may perform case folding?
> > Do they all perform UTF-8 case folding in this fashion?
>
> All good questions. I do know from experience that UTF-8 usernames are a
> thing and things usually work fine, but I'm pretty sure we've never
> stress tested this particular bit of code :)

If lxd is supposed to be the sole application on a server it's probably
fine, it can fall into the case of "don't do that". But if lxd is supposed
to live on existing systems too, it'd be worth making sure these things
work.

> > - POST to /1.0/images conflates fingerprint with alias -- I think
> > fingerprint should always be included for integrity regardless of alias
> > presence or absence
>
> You are referring to the POST to /1.0/images which triggers a
> cross-server copy.

Ah, that makes sense. Just so long as every image upload includes an
integrity check.

>
> > - POST to /1.0/certificates has multiple conditionals with "friendly"
> > fallbacks. I think this is a mistake, I think the client should supply
> > the certificate and name without friendly fallbacks to using the current
> > connection's parameters.
>
> This is to make it easy to script the...

Added to the seeds, will promote in a bit.

Promoted, closing bug report, thanks.

Stephane! Please follow the MIR protocol. Security team ACK isn't technically the same as a MIR approval. Notice how Seth didn't set this bug to be Fix Committed.

A security team audit is sometimes used in MIRs, but it is not the full review. For that, we look at packaging, whether there are important outstanding bugs, maintainership in Debian, whether the package has a team bug subscriber in Ubuntu, etc.

Not much more on top of the security audit. But a little more.

Anyway, I gave things a quick once over, and they are fine. Approved.

Re-opening this and demoting to universe due to lack of golang in main at this point.

Let's leave this as Fix Committed, so when it's deps get promoted, the archive team knows this can also be promoted.

I have repromoted the lxd binary packages to main, while leaving the source in universe, so that these can be included in images while the golang MIR is being finalized.

Thanks Steve! Seems quite reasonable.
On Oct 14, 2015 7:41 PM, "Steve Langasek" <email address hidden>
wrote:

> I have repromoted the lxd binary packages to main, while leaving the
> source in universe, so that these can be included in images while the
> golang MIR is being finalized.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1481507
>
> Title:
> [MIR] lxd
>
> Status in lxd package in Ubuntu:
> Fix Committed
>
> Bug description:
> [Availability]
> In universe since 15.04 and available for all architectures. Using
> golang for those architectures it supports and gccgo for the rest.
>
> [Rationale]
> LXD is a new container manager based on liblxc which offers a REST API
> to manage containers and container images across multiple hosts.
>
> It's developed by Canonical and the LinuxContainers community and
> meant to become the new one to interact with LXC containers, both
> locally and at scale. As a result, we want it as widely available and
> supported as LXC itself.
>
> It's planned for LXD to become pre-installed in cloud images as well
> as snappy images. In this configuration, LXD will be socket activated
> to limit resource usage on systems that don't actively use it and it
> will not be listening on any network port by default.
>
> [Security]
> LXD hasn't had any security issue so far.
>
> The LXD daemon runs as root, containers spawned by it are then
> typically running unprivileged with apparmor, seccomp, capabilities
> and cgroup restrictions through the use of LXC.
>
> The LXD daemon listens to a local unix socket, only accessible to
> members of the lxd group. Through that unix socket it's then possible
> to get the daemon to bind a tcp port for network operations.
>
> [Quality assurance]
> LXD basically just works when it's installed, the daemon is auto-started
> through socket activation and any member of the admin or sudo group is
> granted access to lxd upon installation.
>
> There are no debconf prompts in the lxd packages.
>
> Upstream is pretty much only made of Ubuntu developers so we expect a
> very good relationship here. As it stands, there aren't any serious
> bug with LXD in Ubuntu.
>
> The package is actively maintained in Ubuntu by upstream, it's not in
> Debian.
>
> LXD isn't tied to any specific hardware.
>
> The testsuite cannot be run at package build time due to strict
> requirements on kernel, network and root access.
>
> A debian watch file is included to track new releases.
>
> [Dependencies]
>
> LXD build-depends on golang which is currently subject to a separate
> MIR.
>
> [Standards compliance]
>
> LXD complies with the current Debian standards.
>
> [Maintenance]
>
> Upstream is maintaining the Ubuntu packages as well as various daily
> builds and backports.
>
> [Background information]
> Nothing special to report.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/ubuntu/+source/lxd/+bug/1481507/+subscriptions
>

golang has now been promoted to main, so moving the lxd source to main as well.

$ change-override -c main -t lxd
Override component to main
lxd 0.20-0ubuntu3 in wily: universe/admin -> main
Override [y|N]? y
1 publication overridden.

The promotion of this package was premature and it doesn't meet the security requirements. Furthermore, the packaging is not meeting the requirements for a Go package in main. Specifically:
1. it is is not using dh-golang
2. debian/control use Built-Using: ${misc:Built-Using} for each non'-dev' binary package
3. it depends on gccgo for powerpc ppc64el (AIUI, you should use golang-go and it will pull in gccgo if needed)
4. it doesn't use any golang-*-dev packages when they are available in the archive and hasn't broken out other embedded libraries. Seth pointed this out in his review ("The lxd team will break apart the vendorized Go dependencies").

Of those, '1' is not a hard requirement for this MIR, but I strongly recommend you consider it for using the golang-*-dev packages (see http://pkg-go.alioth.debian.org/packaging.html; might want to use dh-make-golang). '3' should also be fixed, unless Foundations says otherwise. '2' must be fixed (but that is easy).

Which leaves '4': embedded sources with corresponding source in the archive:
- dist/src/golang.org/x/crypto: use golang-go.crypto-dev (part of juju MIR)
- dist/src/github.com/chai2010/gettext-go: use golang-gettext-dev, needs MIR
- dist/src/github.com/dustinkirkland/golang-petname: use golang-petname-dev, needs MIR
- dist/src/github.com/godbus/dbus: use golang-go-dbus-dev (part of juju MIR)
- dist/src/github.com/golang/protobuf: use golang-goprotobuf-dev, needs MIR
- dist/src/github.com/inconshreveable/go-vhost: use golang-vhost-dev, needs MIR
- dist/src/github.com/gorilla/context: use golang-context-dev, needs MIR
- dist/src/github.com/gorilla/mux: use golang-mux-dev, needs MIR
- dist/src/github.com/gorilla/websocket: use golang-websocket-dev, needs MIR
- dist/src/github.com/mattn/go-sqlite3: use golang-gosqlite-dev?, needs MIR
- dist/src/github.com/satori/go.uuid: use golang-uuid-dev, needs MIR
- dist/src/github.com/stretchr/objx: use golang-objx-dev, needs MIR
- dist/src/github.com/stretchr/testify: use golang-testify-dev, needs MIR
- dist/src/github.com/syndtr/gocapability: use golang-gocapability-dev, needs MIR
- dist/src/gopkg.in/check.v1: use golang-check.v1-dev (part of juju MIR)
- dist/src/gopkg.in/tomb.v2: use golang-gopkg-tomb.v2-dev, needs MIR
- dist/src/gopkg.in/yaml.v2: golang-yaml.v2-dev (juju is using golang-goyaml but trying to go to golang-yaml.v2-dev)

These have no corresponding source in the archive, and should be broken out:
- dist/src/code.google.com/p/go-charset
- dist/src/github.com/elazarl/goproxy
- dist/src/github.com/mattn/go-colorable
- dist/src/github.com/olekukonko/tablewriter
- dist/src/gopkg.in/flosch/pongo2.v3
- dist/src/gopkg.in/inconshreveable/log15.v2 (maybe choose one of the other
  options that are already in the archive?)

These seem LXD specific and seem ok to leave embedded(?):
- dist/src/github.com/stgraber/lxd-go-systemd
- dist/src/gopkg.in/lxc/go-lxc.v2 (should this be broken out?)

Stephane mentioned "We have the list of those, a good bunch are already packaged, we'll have to package at least two new ones and move away/replace a few more." At the time I took this to mean it was all in flight, but I still don't see that it has happened yet. ...

Based on the latest comment from the security team, I'm backing the lxd source back down to universe pending the resolution of these outstanding issues.

For the juju team, they are using golang-*-dev sources if they exist in the archive and leaving breaking out other embedded copies to 16.04. If this would help lxd for what I mentioned under "These have no corresponding source in the archive, and should be broken out", we can file a bug and not block on those.

> 3. it depends on gccgo for powerpc ppc64el (AIUI, you should use
> golang-go and it will pull in gccgo if needed)

> '3' should also be fixed, unless Foundations says otherwise.

This is at parity with juju-core, which has:
  gccgo-5 [!amd64 !i386 !armhf] | gccgo-go [!amd64 !i386 !armhf], golang-go (>= 2:1.2) [amd64 i386 armhf]

So no concern on this from the Foundations end.

Jamie. What you're saying seems at odds with what Seth said:
"Security team ACK for promoting lxd to main."

So I don't believe I've done anything prematurely and have in fact waited for a security team ack, which I got.

We are now past final freeze with this package installed in official images, so putting it back to universe seems pretty impractical.

As for your other points:

1) Should be easy to fix
2) same
3) doesn't really matter and makes backporting to trusty significantly easier as we don't have go 1.5 with the nice meta packages in there yet. We'll change that once we've managed to get 1.5 into trusty-backports so we can keep following the "don't modify the source packaging" rule of backports.
4) our plan (and that's what i communicated to Seth and anyone else who asked) is to switch to using packaged dependencies with the first 16.04 upload. I will definitely NOT do this in 15.10 as there is absolutely no way for us to offer any kind of support on this particular version combination. I expect it will be a multi-month effort to bump some of those packages, distro-patch some of the others, ... to end up with a combination which works for lxd and for everyone else without causing us all different weird issues.

As I believe was communicated to you over IRC. I am on vacation until after 15.10 is out the door. I have a couple of hours at random airports on the way back from Seattle right now and will try to address 1) and 2) during that time.

Given when we got the security feedback (less than a week ago) and the fact that said feedback told us we were good to promote, that's the absolute maximum I'm ready to do for this cycle.

Got most of those changes working though dh-golang isn't flexible enough to accommodate for a source package building 4 different binaries using different package names as well as two shared packages.
It's very much designed for packages which use a single package and usually, single binary.

So I have to jump through quite a few hoops to workaround this, made more complicated by having to also support trusty's old dh-golang for our backport.

Uploaded after doing some comparison of binary packages to make sure the new bits are there and that nothing disappeared in the process.

The dh overrides are a bit gross but the bits of dh-golang that I ended up having to skip have been replaced by the exact same calls dh-golang would have done, should it have been able to understand our structure (read the dh-golang code).

On 17/10/2015 9:55 am, "Jamie Strandboge" <email address hidden> wrote:

> 3. it depends on gccgo for powerpc ppc64el (AIUI, you should use
golang-go and it will pull in gccgo if needed)

lxd doesn't build with golang-go 1.5 on ppc64el, the port doesn't have full
support for cgo (1.6 will if someone ever gets around to reviewing my
branches)

This bug was fixed in the package lxd - 0.20-0ubuntu4

---------------
lxd (0.20-0ubuntu4) wily; urgency=medium

  * Use dh-golang. (LP: #1481507)
  * Fix some lintian warnings (debconf related)

 -- Stéphane Graber <email address hidden> Fri, 16 Oct 2015 21:32:41 -0700

@Michael - yes, I'm aware of that but my understanding was that build depending on golang-go only would pull in gccgo on the architectures that needed it, like ppc64el. Per Foundations comment, the architectures can stay as is for now.

@Stephane - thank you for the 0.20-0ubuntu4 upload.

Also, as Michael said, security team approval alone is not enough for approval and so regardless of the misunderstanding surrounding the embedded code copes, the other issues to be done before acceptance.

Regarding the embedded copies, I appreciate that you have resources dedicated to fixing this. I guess I am just surprised both that it will take so long for using at least the copies in the archive (the juju team seemed to not have too terribly much trouble with it). Also, I communicated the need for splitting this out on IRC at least a month ago and in the juju/golang/lxd mir interlock weekly hangout. Unfortunately, miscommunication and/or misunderstanding on both sides seems to have gotten us into the position we are in now (it was never communicated directly to me and I missed the 'current plan' in comment #9, I guess due to the LP snafu with that comment).

At this point, there is so little time for the LXD team to make the changes and they've already resourced the work for the embedded copies for 16.04. I've created bug #1507156 to track breaking out the embedded code copies. Since 0.20-0ubuntu4 addresses the other packaging concerns, marking Fix Committed.

Well, the bug is already Fix Released so I'll simply adjust the overrides.

Override component to main
lxd 0.20-0ubuntu4 in wily: universe/admin -> main
Override [y|N]? y

On 18 October 2015 at 02:59, Jamie Strandboge <email address hidden> wrote:
> @Michael - yes, I'm aware of that but my understanding was that build
> depending on golang-go only would pull in gccgo on the architectures
> that needed it, like ppc64el. Per Foundations comment, the architectures
> can stay as is for now.

The thing is on ppc64el golang-go gets you "real" Go -- but that
doesn't build lxd.