| Message ID | 20170915090253.12201-1-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 15 September 2017 at 10:02, Fam Zheng <famz@redhat.com> 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
On Fri, 09/15 11:55, Peter Maydell wrote: > On 15 September 2017 at 10:02, Fam Zheng <famz@redhat.com> 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
On Fri, Sep 15, 2017 at 11:55:44AM +0100, Peter Maydell wrote: > On 15 September 2017 at 10:02, Fam Zheng <famz@redhat.com> 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
On 15 September 2017 at 12:40, Daniel P. Berrange <berrange@redhat.com> 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
On Fri, Sep 15, 2017 at 01:03:54PM +0100, Peter Maydell wrote: > On 15 September 2017 at 12:40, Daniel P. Berrange <berrange@redhat.com> 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
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 <famz@redhat.com> 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
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 <famz@redhat.com> 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
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 <famz@redhat.com> 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
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.
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