Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Fam Zheng]

On Fri, 09/15 11:47, Philippe Mathieu-Daudé wrote:
> why not generate 1 key in
> tests/vm/ (or clever ~/.cache/qemu-vm/ already used by those scripts) once
> when the make vm-test rule is called, that would be 1 key per repository
> clone (or 1 per user using ~/.cache).

Like I explained elsewhere, the BSD images must be generated after the keys in
order for the pub key to be added to the guest authorized_keys. And there is no
automatic way to do so. That's why the keys are committed to the repo, not
generated (otherwise we'd just use ~/.ssh pub keys).

Fam

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Philippe Mathieu-Daudé]

Hi Daniel,

On 09/15/2017 08:40 AM, Daniel P. Berrange wrote:

On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote:

[...]


So, before I commit an ssh private key to our git repo,
can you explain why it's ok that this is public? The
commit message for the relevant patch doesn't really say.


IIUC, the public part of the key gets exposed to the guest images via
cloud-init metadata. During boot the guest read this metadata and add
the public key to authorized_keys. The private key is used by the test
suite on the host so that it can now login to the guests.

So the risk here is that if these guests were exposed to the LAN in any
way, someone could grab our private key and login to these guests.

What saves us is that the VMs are run with user mode slirp networking
so AFAICT, aren't exposed to the LAN.  So as long as we don't change
this to any kind of real networking, I think its acceptable to have
the private key in it and doesn't expose developer's workstations to
undue risk and avoids consuming system entropy to generate new keys

> during build.

which systems are you worried about? build-farms or developer's stations?

why do you want to generate more than 1 key? why not generate 1 key in 
tests/vm/ (or clever ~/.cache/qemu-vm/ already used by those scripts) 
once when the make vm-test rule is called, that would be 1 key per 
repository clone (or 1 per user using ~/.cache).


Distrib aren't using the test suite in binary packages.

Regards,

Phil.

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Fam Zheng]

On Fri, 09/15 13:31, Daniel P. Berrange wrote:
> On Fri, Sep 15, 2017 at 08:21:53PM +0800, Fam Zheng wrote:
> > On Fri, 09/15 12:40, Daniel P. Berrange wrote:
> > > On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote:
> > > > On 15 September 2017 at 10:02, Fam Zheng  wrote:
> > > > > The following changes since commit 
> > > > > 04ef33052c205170c92df21ca0b4be4f3b102188:
> > > > >
> > > > >   tcg/tci: do not use ldst label (never implemented) (2017-09-11 
> > > > > 19:24:05 +0100)
> > > > >
> > > > > are available in the git repository at:
> > > > >
> > > > >   git://github.com/famz/qemu.git tags/test-and-build-pull-request
> > > > >
> > > > > for you to fetch changes up to 
> > > > > be78fe670401af14e6d63fce5c5467f751207871:
> > > > >
> > > > >   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
> > > > >
> > > > > 
> > > > >
> > > > > 
> > > > >
> > > > > Alex Bennée (4):
> > > > >   docker: ensure NOUSER for travis images
> > > > >   docker: docker.py make --no-cache skip checksum test
> > > > >   docker: don't install device-tree-compiler build-deps in 
> > > > > travis.docker
> > > > >   docker: reduce noise when building travis.docker
> > > > >
> > > > > Fam Zheng (34):
> > > > >   docker: Update ubuntu image
> > > > >   docker: Enable features explicitly in test-full
> > > > >   tests/docker: Clean up paths
> > > > >   gitignore: Ignore vm test images
> > > > >   qemu.py: Add "wait()" method
> > > > >   scripts: Add archive-source.sh
> > > > >   tests: Add a test key pair
> > > > 
> > > > So, before I commit an ssh private key to our git repo,
> > > > can you explain why it's ok that this is public? The
> > > > commit message for the relevant patch doesn't really say.
> > > 
> > > IIUC, the public part of the key gets exposed to the guest images via
> > > cloud-init metadata. During boot the guest read this metadata and add
> > > the public key to authorized_keys. The private key is used by the test
> > > suite on the host so that it can now login to the guests.
> > > 
> > > So the risk here is that if these guests were exposed to the LAN in any
> > > way, someone could grab our private key and login to these guests.
> > > 
> > > What saves us is that the VMs are run with user mode slirp networking
> > > so AFAICT, aren't exposed to the LAN.  So as long as we don't change
> > > this to any kind of real networking, I think its acceptable to have
> > > the private key in it and doesn't expose developer's workstations to
> > > undue risk and avoids consuming system entropy to generate new keys
> > > during build.
> > 
> > The hostfwd does listen on a dynamic port on 0.0.0.0, so does vnc. I didn't
> > really care since it's for temporary guests and for me convenience 
> > outweighed a
> > bit.  The VM test is indeed less restricted than the docker ones such as in 
> > that
> > network is always available. Should it be a problem?
> 
> AFAICT there's no functional reason why it needs to listen on 0.0.0.0,
> instead of 127.0.0.1, so general security best practice says it should
> not expose this listening port on LAN interfaces for the developers
> machine, even if we think the risk is low.

Yes, makes sense, let's change it. The only disadvantage of 127.0.0.1 is if the
test is run on a remote host, you don't have to ssh to the host and proxy from
there to login to the guest. The test is automated, so accessing guest may be a
rare need outside patchew (a few months ago I frequently need to diagnose
hanging tests on patchew, no idea how this vm test will do :).

Fam

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 08:21:53PM +0800, Fam Zheng wrote:
> On Fri, 09/15 12:40, Daniel P. Berrange wrote:
> > On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote:
> > > On 15 September 2017 at 10:02, Fam Zheng  wrote:
> > > > The following changes since commit 
> > > > 04ef33052c205170c92df21ca0b4be4f3b102188:
> > > >
> > > >   tcg/tci: do not use ldst label (never implemented) (2017-09-11 
> > > > 19:24:05 +0100)
> > > >
> > > > are available in the git repository at:
> > > >
> > > >   git://github.com/famz/qemu.git tags/test-and-build-pull-request
> > > >
> > > > for you to fetch changes up to be78fe670401af14e6d63fce5c5467f751207871:
> > > >
> > > >   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
> > > >
> > > > 
> > > >
> > > > 
> > > >
> > > > Alex Bennée (4):
> > > >   docker: ensure NOUSER for travis images
> > > >   docker: docker.py make --no-cache skip checksum test
> > > >   docker: don't install device-tree-compiler build-deps in travis.docker
> > > >   docker: reduce noise when building travis.docker
> > > >
> > > > Fam Zheng (34):
> > > >   docker: Update ubuntu image
> > > >   docker: Enable features explicitly in test-full
> > > >   tests/docker: Clean up paths
> > > >   gitignore: Ignore vm test images
> > > >   qemu.py: Add "wait()" method
> > > >   scripts: Add archive-source.sh
> > > >   tests: Add a test key pair
> > > 
> > > So, before I commit an ssh private key to our git repo,
> > > can you explain why it's ok that this is public? The
> > > commit message for the relevant patch doesn't really say.
> > 
> > IIUC, the public part of the key gets exposed to the guest images via
> > cloud-init metadata. During boot the guest read this metadata and add
> > the public key to authorized_keys. The private key is used by the test
> > suite on the host so that it can now login to the guests.
> > 
> > So the risk here is that if these guests were exposed to the LAN in any
> > way, someone could grab our private key and login to these guests.
> > 
> > What saves us is that the VMs are run with user mode slirp networking
> > so AFAICT, aren't exposed to the LAN.  So as long as we don't change
> > this to any kind of real networking, I think its acceptable to have
> > the private key in it and doesn't expose developer's workstations to
> > undue risk and avoids consuming system entropy to generate new keys
> > during build.
> 
> The hostfwd does listen on a dynamic port on 0.0.0.0, so does vnc. I didn't
> really care since it's for temporary guests and for me convenience outweighed 
> a
> bit.  The VM test is indeed less restricted than the docker ones such as in 
> that
> network is always available. Should it be a problem?

AFAICT there's no functional reason why it needs to listen on 0.0.0.0,
instead of 127.0.0.1, so general security best practice says it should
not expose this listening port on LAN interfaces for the developers
machine, even if we think the risk is low.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Fam Zheng]

On Fri, 09/15 12:40, Daniel P. Berrange wrote:
> On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote:
> > On 15 September 2017 at 10:02, Fam Zheng  wrote:
> > > The following changes since commit 
> > > 04ef33052c205170c92df21ca0b4be4f3b102188:
> > >
> > >   tcg/tci: do not use ldst label (never implemented) (2017-09-11 19:24:05 
> > > +0100)
> > >
> > > are available in the git repository at:
> > >
> > >   git://github.com/famz/qemu.git tags/test-and-build-pull-request
> > >
> > > for you to fetch changes up to be78fe670401af14e6d63fce5c5467f751207871:
> > >
> > >   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
> > >
> > > 
> > >
> > > 
> > >
> > > Alex Bennée (4):
> > >   docker: ensure NOUSER for travis images
> > >   docker: docker.py make --no-cache skip checksum test
> > >   docker: don't install device-tree-compiler build-deps in travis.docker
> > >   docker: reduce noise when building travis.docker
> > >
> > > Fam Zheng (34):
> > >   docker: Update ubuntu image
> > >   docker: Enable features explicitly in test-full
> > >   tests/docker: Clean up paths
> > >   gitignore: Ignore vm test images
> > >   qemu.py: Add "wait()" method
> > >   scripts: Add archive-source.sh
> > >   tests: Add a test key pair
> > 
> > So, before I commit an ssh private key to our git repo,
> > can you explain why it's ok that this is public? The
> > commit message for the relevant patch doesn't really say.
> 
> IIUC, the public part of the key gets exposed to the guest images via
> cloud-init metadata. During boot the guest read this metadata and add
> the public key to authorized_keys. The private key is used by the test
> suite on the host so that it can now login to the guests.
> 
> So the risk here is that if these guests were exposed to the LAN in any
> way, someone could grab our private key and login to these guests.
> 
> What saves us is that the VMs are run with user mode slirp networking
> so AFAICT, aren't exposed to the LAN.  So as long as we don't change
> this to any kind of real networking, I think its acceptable to have
> the private key in it and doesn't expose developer's workstations to
> undue risk and avoids consuming system entropy to generate new keys
> during build.

The hostfwd does listen on a dynamic port on 0.0.0.0, so does vnc. I didn't
really care since it's for temporary guests and for me convenience outweighed a
bit.  The VM test is indeed less restricted than the docker ones such as in that
network is always available. Should it be a problem?

We can probably add restrict=on to slirp and listen on loopback.

Fam

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 01:03:54PM +0100, Peter Maydell wrote:
> On 15 September 2017 at 12:40, Daniel P. Berrange  wrote:
> > IIUC, the public part of the key gets exposed to the guest images via
> > cloud-init metadata. During boot the guest read this metadata and add
> > the public key to authorized_keys. The private key is used by the test
> > suite on the host so that it can now login to the guests.
> >
> > So the risk here is that if these guests were exposed to the LAN in any
> > way, someone could grab our private key and login to these guests.
> >
> > What saves us is that the VMs are run with user mode slirp networking
> > so AFAICT, aren't exposed to the LAN.
> 
> If I'm reading the right bit of the script we run QEMU with a
> hostfwd specification using 0.0.0.0 as the host part -- doesn't
> that listen on all interfaces including the LAN ones?

Actually yes, you are right, my bad.

That needs to be fixed to use 127.0.0.1 for sure.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Peter Maydell]

On 15 September 2017 at 12:40, Daniel P. Berrange  wrote:
> IIUC, the public part of the key gets exposed to the guest images via
> cloud-init metadata. During boot the guest read this metadata and add
> the public key to authorized_keys. The private key is used by the test
> suite on the host so that it can now login to the guests.
>
> So the risk here is that if these guests were exposed to the LAN in any
> way, someone could grab our private key and login to these guests.
>
> What saves us is that the VMs are run with user mode slirp networking
> so AFAICT, aren't exposed to the LAN.

If I'm reading the right bit of the script we run QEMU with a
hostfwd specification using 0.0.0.0 as the host part -- doesn't
that listen on all interfaces including the LAN ones?

thanks
-- PMM

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Daniel P. Berrange]

On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote:
> On 15 September 2017 at 10:02, Fam Zheng  wrote:
> > The following changes since commit 04ef33052c205170c92df21ca0b4be4f3b102188:
> >
> >   tcg/tci: do not use ldst label (never implemented) (2017-09-11 19:24:05 
> > +0100)
> >
> > are available in the git repository at:
> >
> >   git://github.com/famz/qemu.git tags/test-and-build-pull-request
> >
> > for you to fetch changes up to be78fe670401af14e6d63fce5c5467f751207871:
> >
> >   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
> >
> > 
> >
> > 
> >
> > Alex Bennée (4):
> >   docker: ensure NOUSER for travis images
> >   docker: docker.py make --no-cache skip checksum test
> >   docker: don't install device-tree-compiler build-deps in travis.docker
> >   docker: reduce noise when building travis.docker
> >
> > Fam Zheng (34):
> >   docker: Update ubuntu image
> >   docker: Enable features explicitly in test-full
> >   tests/docker: Clean up paths
> >   gitignore: Ignore vm test images
> >   qemu.py: Add "wait()" method
> >   scripts: Add archive-source.sh
> >   tests: Add a test key pair
> 
> So, before I commit an ssh private key to our git repo,
> can you explain why it's ok that this is public? The
> commit message for the relevant patch doesn't really say.

IIUC, the public part of the key gets exposed to the guest images via
cloud-init metadata. During boot the guest read this metadata and add
the public key to authorized_keys. The private key is used by the test
suite on the host so that it can now login to the guests.

So the risk here is that if these guests were exposed to the LAN in any
way, someone could grab our private key and login to these guests.

What saves us is that the VMs are run with user mode slirp networking
so AFAICT, aren't exposed to the LAN.  So as long as we don't change
this to any kind of real networking, I think its acceptable to have
the private key in it and doesn't expose developer's workstations to
undue risk and avoids consuming system entropy to generate new keys
during build.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Fam Zheng]

On Fri, 09/15 11:55, Peter Maydell wrote:
> On 15 September 2017 at 10:02, Fam Zheng  wrote:
> > The following changes since commit 04ef33052c205170c92df21ca0b4be4f3b102188:
> >
> >   tcg/tci: do not use ldst label (never implemented) (2017-09-11 19:24:05 
> > +0100)
> >
> > are available in the git repository at:
> >
> >   git://github.com/famz/qemu.git tags/test-and-build-pull-request
> >
> > for you to fetch changes up to be78fe670401af14e6d63fce5c5467f751207871:
> >
> >   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
> >
> > 
> >
> > 
> >
> > Alex Bennée (4):
> >   docker: ensure NOUSER for travis images
> >   docker: docker.py make --no-cache skip checksum test
> >   docker: don't install device-tree-compiler build-deps in travis.docker
> >   docker: reduce noise when building travis.docker
> >
> > Fam Zheng (34):
> >   docker: Update ubuntu image
> >   docker: Enable features explicitly in test-full
> >   tests/docker: Clean up paths
> >   gitignore: Ignore vm test images
> >   qemu.py: Add "wait()" method
> >   scripts: Add archive-source.sh
> >   tests: Add a test key pair
> 
> So, before I commit an ssh private key to our git repo,
> can you explain why it's ok that this is public? The
> commit message for the relevant patch doesn't really say.

It's under tests/, and the key is only used to access a temporarily spawned test
VM.

Fam

Re: [Qemu-devel] [PULL v4 00/38] Test and build patches

[Peter Maydell]

On 15 September 2017 at 10:02, Fam Zheng  wrote:
> The following changes since commit 04ef33052c205170c92df21ca0b4be4f3b102188:
>
>   tcg/tci: do not use ldst label (never implemented) (2017-09-11 19:24:05 
> +0100)
>
> are available in the git repository at:
>
>   git://github.com/famz/qemu.git tags/test-and-build-pull-request
>
> for you to fetch changes up to be78fe670401af14e6d63fce5c5467f751207871:
>
>   buildsys: Move rdma libs to per object (2017-09-15 15:05:24 +0800)
>
> 
>
> 
>
> Alex Bennée (4):
>   docker: ensure NOUSER for travis images
>   docker: docker.py make --no-cache skip checksum test
>   docker: don't install device-tree-compiler build-deps in travis.docker
>   docker: reduce noise when building travis.docker
>
> Fam Zheng (34):
>   docker: Update ubuntu image
>   docker: Enable features explicitly in test-full
>   tests/docker: Clean up paths
>   gitignore: Ignore vm test images
>   qemu.py: Add "wait()" method
>   scripts: Add archive-source.sh
>   tests: Add a test key pair

So, before I commit an ssh private key to our git repo,
can you explain why it's ok that this is public? The
commit message for the relevant patch doesn't really say.

thanks
-- PMM