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 API using curl or similar tools.
>
> Can you point out a security issue with those fallbacks?
No; however, there are currently eight different case combinations that
need both positive and negative tests -- as it stands you need at minimum
16 test cases for this API, and there's a good chance that each of those
16 cases really needs four or five test cases; a comprehensive test will
result in roughly eighty to one hundred tests.
That's why I think this should be a simpler API, the clear majority of
uses are going to be in the command line tools where in practice only two
or three of the eight combinations is ever actually going to be used. So
I suggest supporting only those and cut the number of tests in half.
> > - methods.go LinkUnitFiles() -- does this allow setting arbitrary services
> > to start?
>
> I have no idea, this is go-systemd's code and as far as I can tell, not
> part of the code which we actually use. LXD only uses the socket
> activation code from this project.
>
> Note that in the future, it'd be helpful to indicate the fullpath since
> I had to dig around a bit as this isn't part of the LXD upstream source
> tree.
ctags works great even on Go projects. Strongly recommended, then you can
use just vim -t LinkUnitFiles or :ts LinkUnitFiles (if there's several).
Run "ctags -R" to get started.
> > - container.go Daemonize() feels very misnamed
> > - container.go WantDaemonize() feels very misnamed
> > - container.go WantCloseAllFds() error return ErrCloseAllFdsFailed seems
> > wrong, the call chain doesn't look like it actually tries to close
> > anything, and based on the boolean passed in may not even want to.
> > - container.go ConfigKeys() is confusing, could someone give it a re-read
> > and make sure it does what it's supposed to do? TestConfigKeys() isn't
> > very comprehensive.
>
> I'm assuming all of the above refer to go-lxc and not to LXD's own
> container.go, again, giving full paths in such cases would save me
> valuable time.
>
> I'm not terribly familiar with this code since it's an external project
> maintained by Caglar, however those misnamed function as you call them
> are consistently named with the other LXC bindings and with LXC's own
> API, so I prefer consistency with C API over renaming everything to try
> and better describe what they do.
Well, alright, if they're just continuations of unfortunate names from
elsewhere, that's alright. I was hoping it was early enough to give them
good names.
> want_daemonize and want_close_all_fds both just set a flag and return 0
> if succesful, so the Go wrapping code looks right to me in that Err*
> will only be raised if the C API fails for some reason.
>
> As for ConfigKeys, the implementation looks correct, though a check for
> more than one argument could probably be added.
>
> ConfigKeys in the LXC C API (and its wrapping here) will return a list
> of config keys when passed NULL or a list of sub-keys when passed a key.
> The wrapper looks correct to me in that regard.
Thanks so much for giving this another look.
> > - goproxy/all.bash most variables used in commands need to be
> > double-quoted; *_test.go is unsafe with filenames starting with -;
> > should use ./*_test.go
>
> Unused by LXD. We'll probably switch to an in-house HTTP proxy for 16.04
> as the go-proxy dependency is rather massive for a pretty trivial
> feature (non-logging, non-caching, http proxy with CONNECT support).
Is this a task better done by squid or haproxy?
> > We can support lxd in main conditional upon some concessions
> > from the lxd team:
> >
> > - The security team will need help preparing security fixes for all
> > supported releases. This may come at the expense of feature development.
> > Additional headcount may be necessary to meet all business goals while
> > still providing acceptable support for supported releases.
>
> The LXC team already does so for all the other LXC related projects
> (lxc, cgmanager and lxcfs), so I don't expect this to be a problem.
>
> > - The security team will need help testing all updates for all supported
> > releases; this may include test cases, test frameworks, and
> > infrastructure.
>
> As with LXC, LXD has an upstream Jenkins instance that can test any
> branch and make sure we don't have regressions. The same tests are also
> run in autopkgtest and can easily be run locally.
>
> > - The lxd team will discuss with the technical board the best way to keep
> > lxd updated across all supported releases; this may benefit from being
> > kept level across all releases similar to Firefox. This may mandate not
> > using new Go language features introduced in future releases until those
> > features are on our oldest supported releases.
>
> The current plan (and this may change based on management request) is to
> do it LXC-style, with LXD 1.0 being tagged with 16.04 and then going
> into bugfix-only mode with a micro release exception.
>
> It's a model which has been working great for LXC and the LXC related
> projects (combined with PPA and backports for new versions) and one that
> I hope we'll be able to go with for LXD.
>
> > - The lxd team will break apart the vendorized Go dependencies. Discuss
> > with the Juju team which shared dependencies, if any, should be split
> > apart first, and aim to have all external dependencies removed from the
> > lxd package before 16.04 beta. See the Golang MIR bug for full details:
> > https://bugs.launchpad.net/ubuntu/+source/juju-mongodb/+bug/1267393
>
> This is planned, current status is:
>
> DEPENDENCY PACKAGE or PLAN
> --- ---
> dist/src/code.google.com/p/go-charset/ TODO: Implement tiny
> HTTP proxy?
> dist/src/github.com/chai2010/gettext-go/ golang-gettext-dev
> dist/src/github.com/dustinkirkland/golang-petname/ golang-petname-dev
> dist/src/github.com/elazarl/goproxy/ TODO: Implement tiny
> HTTP proxy?
> dist/src/github.com/godbus/dbus/ golang-dbus-dev
> dist/src/github.com/golang/protobuf/ golang-goprotobuf-dev
> dist/src/github.com/gorilla/context/ golang-context-dev
> dist/src/github.com/gorilla/mux/ golang-mux-dev
> dist/src/github.com/gorilla/websocket/ golang-websocket-dev
> dist/src/github.com/inconshreveable/go-vhost/ golang-vhost-dev
> dist/src/github.com/mattn/go-colorable/ TODO: Change logging
> system?
> dist/src/github.com/mattn/go-sqlite3/ TODO: Is
> golang-gosqlite-dev good enough?
> dist/src/github.com/olekukonko/tablewriter/ TODO: Alternatives?
> dist/src/github.com/satori/go.uuid/ golang-uuid-dev
> dist/src/github.com/stgraber/lxd-go-systemd/ golang-go-systemd-dev
> dist/src/github.com/stretchr/objx/ golang-objx-dev
> dist/src/github.com/stretchr/testify/ golang-testify-dev
> dist/src/github.com/syndtr/gocapability/
> golang-gocapability-dev
> dist/src/golang.org/x/crypto/ golang-go.crypto-dev
> dist/src/gopkg.in/check.v1/ golang-check.v1-dev
> dist/src/gopkg.in/flosch/pongo2.v3/ NEEDS PACKAGING
> dist/src/gopkg.in/inconshreveable/log15.v2/ TODO: Change logging
> system?
> dist/src/gopkg.in/lxc/go-lxc.v2/ NEEDS PACKAGING
> dist/src/gopkg.in/tomb.v2/
> golang-gopkg-tomb.v2-dev
> dist/src/gopkg.in/yaml.v2/ golang-yaml.v2-dev
>
> My plan is to open 16.04 with LXD switching to all the existing packaged
> ones, note that this means that 17 MIRs will be filed and sent your way.
>
> We'll then be packaging go-lxc and pongo2 and look at the remaining 6 to
> see whether we can drop them somehow and if not, package them.
>
> > Please confirm these conditions are acceptable.
>
> That's pretty much what we expected so I don't have any problem with
> that.
>
> So long as your team can handle dealing with 19 extra MIRs in a timely
> fashion, we should be fine.
I'd rather have them than have bundled code :)
Security team ACK for promoting lxd to main.
Thanks Stéphane
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1481507
Title:
[MIR] lxd
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/lxd/+bug/1481507/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
