From 2dab23cd9ef371d24a95dad555bd11322719a2c3 Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 31 Oct 2026 01:53:43 +0300 Subject: [PATCH] fix(gstfsd): only the panel, with the host's key, can change a VM's disk gstfsd, the root service on each compute that rewrites a shut-off VM's /etc/shadow and authorized_keys, did what any client on its port asked. Now it needs GSTFSD_KEY (64 hexadecimal characters, in the unit's /etc/default/webvirt-gstfsd) and does not start without it. Each connection gets a new challenge; the panel signs its request for it with HMAC-SHA256 and gstfsd checks the signature before any change, so a request recorded on one connection is refused on another. gstfsd signs its reply for the same challenge and the panel's nonce, and the panel accepts only that reply. Each compute has a gstfsd key, the host's GSTFSD_KEY: the compute forms and the API take it and never show it, and an empty value on an edit keeps it. The bootstrap script makes the key file once, readable by root only; the README shows how to make it and where the panel takes it. The traffic is still not encrypted: a remote panel's port stays open to the panel's address only. --- README.md | 9 +- computes/api/serializers.py | 13 +- computes/forms.py | 28 +++-- computes/migrations/0007_gstfsd_key.py | 19 +++ computes/models.py | 13 +- computes/tests/test_gstfsd_key.py | 49 ++++++++ computes/validators.py | 7 ++ conf/daemon/gstfsd | 64 ++++++++-- conf/systemd/webvirt-gstfsd.service | 2 + dev/libvirt-bootstrap.sh | 9 +- instances/tests/test_gstfsd.py | 168 +++++++++++++++++++++---- instances/tests/test_root_pass.py | 67 +++------- instances/views.py | 48 +++++-- 13 files changed, 388 insertions(+), 108 deletions(-) create mode 100644 computes/migrations/0007_gstfsd_key.py create mode 100644 computes/tests/test_gstfsd_key.py diff --git a/README.md b/README.md index 291e123f..760dafd3 100644 --- a/README.md +++ b/README.md @@ -263,16 +263,20 @@ A compute node is a KVM host the panel manages over libvirt. curl -fsSL https://raw.githubusercontent.com/retspen/webvirtcloud/master/conf/daemon/gstfsd | sudo tee /usr/local/bin/gstfsd > /dev/null sudo chmod +x /usr/local/bin/gstfsd curl -fsSL https://raw.githubusercontent.com/retspen/webvirtcloud/master/conf/systemd/webvirt-gstfsd.service | sudo tee /etc/systemd/system/webvirt-gstfsd.service > /dev/null + # its key, once: keep the file when you update gstfsd + [ -s /etc/default/webvirt-gstfsd ] || sudo sh -c 'umask 077 && echo "GSTFSD_KEY=$(python3 -c "import secrets; print(secrets.token_hex(32))")" > /etc/default/webvirt-gstfsd' + sudo chown root:root /etc/default/webvirt-gstfsd && sudo chmod 600 /etc/default/webvirt-gstfsd sudo systemctl daemon-reload && sudo systemctl enable webvirt-gstfsd && sudo systemctl restart webvirt-gstfsd + sudo cat /etc/default/webvirt-gstfsd # the key for step 4 ``` - It listens on 127.0.0.1:16510 only, which serves a panel on the same host. For a remote panel, run `sudo systemctl edit webvirt-gstfsd`, add `[Service]` and `Environment=GSTFSD_BIND_HOST=0.0.0.0 GSTFSD_ALLOW_REMOTE=1`, and restart it. It is unauthenticated: allow TCP 16510 only from the panel's IP. + gstfsd does only what the panel signs with this key. It listens on 127.0.0.1:16510 only, which serves a panel on the same host. For a remote panel, add `GSTFSD_BIND_HOST=0.0.0.0` and `GSTFSD_ALLOW_REMOTE=1` on lines of their own to `/etc/default/webvirt-gstfsd` and restart it. Its traffic is not encrypted, and a password hash crosses it: allow TCP 16510 only from the panel's IP, on a network you trust. A running VM gets them through its QEMU guest agent instead, without gstfsd. On RHEL-family guests, SELinux lets the agent add SSH keys only after `sudo setsebool -P virt_qemu_ga_manage_ssh on` inside the guest. A compute that ran gstfsd under supervisor: remove its program first (`/etc/supervisor/conf.d/gstfsd.conf` on Debian / Ubuntu, `/etc/supervisord.d/gstfsd.ini` on RHEL / openSUSE), run `sudo supervisorctl update`, and remove supervisor once nothing else uses it. -4. Add the compute on the panel's Computes page. +4. Add the compute on the panel's Computes page, with the `GSTFSD_KEY` value from step 3 as its gstfsd key. Firewall: the compute's VNC ports (`5900`–`65535`) must accept connections **only** from the panel's IP, never from public networks. @@ -604,6 +608,7 @@ A version from before v3.0.0 has no worker: `systemctl disable --now webvirt-wor - A running VM without a guest agent shows the IPv4 address the host learned by ARP (the lookup always failed before). - After a migration the VM's NVRAM file is deleted on the source, when it is in libvirt's `/var/lib/libvirt/qemu/nvram` and the destination does not share that directory (a VM created there later with the same name would start with those UEFI variables). - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3). +- gstfsd does only what the panel signs with the host's key: it does not start without `GSTFSD_KEY` in `/etc/default/webvirt-gstfsd`, and the panel needs the same value as the host's gstfsd key on its edit page. On every compute, update gstfsd and its unit and make the key as in [Adding a Compute Node](#adding-a-compute-node), step 3 (a remote panel's `GSTFSD_BIND_HOST` and `GSTFSD_ALLOW_REMOTE` move into that file from `systemctl edit webvirt-gstfsd`), then enter the key on each compute's edit page. - The themes ship compiled in `static/css/themes`; picking one on the Settings page only saves its name, so updates, rollbacks and recreated Docker containers keep it, and the "Theme SASS Path" setting is gone. libsass is no longer installed; `manage.py build_themes` recompiles the themes in a development setup. - With the bundled nginx config, browsers revalidate static files on each load (304 when unchanged), so an update or a theme change shows without a hard reload. Files cached before this update can stay until a hard reload (Ctrl+F5) of each affected page, or until the site's browser cache is cleared once; an external caching proxy may need a purge. - External snapshots use libvirt's snapshot API and need libvirt 9.9 or newer on the compute hosts. They work on UEFI VMs and running, paused or shut-off VMs, can save memory too or, on a running VM with a connected QEMU guest agent, freeze the guest's file systems (the default there), and a revert keeps disks added later (it detaches them). A snapshot is refused when a storage pool with the VM's disks has less free space than a share of those disks (10%, at least 1 GiB; both on the Settings page) plus, for the pool of the memory file, the VM's memory; this is checked before the snapshot only, nothing is reserved. A VM's snapshots form one chain: after a revert, delete the newer snapshots before taking another. Snapshot names no longer get an `s1.` prefix; existing ones keep theirs and can still be reverted and deleted, except that a snapshot an older version took of a UEFI VM can only be deleted. On NFS, libvirt removes merged snapshot files as the qemu user, who needs write access to the image directory; files it could not remove are named in a warning after the delete or revert. Deleting a VM with its data removes its snapshot files too, except a template it grew from and files another VM uses. diff --git a/computes/api/serializers.py b/computes/api/serializers.py index d260ee81..04b121ce 100644 --- a/computes/api/serializers.py +++ b/computes/api/serializers.py @@ -1,4 +1,5 @@ from computes.models import Compute +from computes.validators import validate_gstfsd_key from rest_framework import serializers from vrtManager.connection import CONN_SOCKET, CONN_SSH, CONN_TCP, CONN_TLS @@ -11,6 +12,13 @@ class ComputeSerializer(serializers.ModelSerializer): allow_blank=True, allow_null=True, ) + gstfsd_key = serializers.CharField( + style={"input_type": "password"}, + write_only=True, + required=False, + allow_blank=True, + validators=[validate_gstfsd_key], + ) # Use a radio input instead of a select input. conn_types = ( (CONN_SSH, "SSH"), @@ -24,6 +32,9 @@ class ComputeSerializer(serializers.ModelSerializer): # as on the web form, an empty password on an edit keeps the stored one return value or (self.instance.password if self.instance else value) + def validate_gstfsd_key(self, value): + return value or (self.instance.gstfsd_key if self.instance else value) + class Meta: model = Compute - fields = ["id", "name", "hostname", "login", "password", "type", "details", "migration_address"] + fields = ["id", "name", "hostname", "login", "password", "type", "details", "migration_address", "gstfsd_key"] diff --git a/computes/forms.py b/computes/forms.py index c67f2530..9caccb82 100644 --- a/computes/forms.py +++ b/computes/forms.py @@ -7,47 +7,55 @@ from computes.models import Compute from .validators import validate_hostname -class KeepPasswordMixin: - """The form never shows the stored password: left empty on an edit, it - stays as it is.""" +class KeepSecretsMixin: + """The form never shows the stored password or gstfsd key: left empty on + an edit, they stay as they are.""" def clean_password(self): return self.cleaned_data["password"] or self.instance.password + def clean_gstfsd_key(self): + return self.cleaned_data["gstfsd_key"] or self.instance.gstfsd_key -class TcpComputeForm(KeepPasswordMixin, forms.ModelForm): + +SECRET_WIDGETS = {"password": forms.PasswordInput(), "gstfsd_key": forms.PasswordInput()} + + +class TcpComputeForm(KeepSecretsMixin, forms.ModelForm): hostname = forms.CharField(validators=[validate_hostname]) type = forms.IntegerField(widget=forms.HiddenInput, initial=CONN_TCP) class Meta: model = Compute - widgets = {"password": forms.PasswordInput()} + widgets = SECRET_WIDGETS fields = "__all__" -class SshComputeForm(forms.ModelForm): +class SshComputeForm(KeepSecretsMixin, forms.ModelForm): hostname = forms.CharField(validators=[validate_hostname], label=_("FQDN/IP")) type = forms.IntegerField(widget=forms.HiddenInput, initial=CONN_SSH) class Meta: model = Compute + widgets = SECRET_WIDGETS exclude = ["password"] -class TlsComputeForm(KeepPasswordMixin, forms.ModelForm): +class TlsComputeForm(KeepSecretsMixin, forms.ModelForm): hostname = forms.CharField(validators=[validate_hostname]) type = forms.IntegerField(widget=forms.HiddenInput, initial=CONN_TLS) class Meta: model = Compute - widgets = {"password": forms.PasswordInput()} + widgets = SECRET_WIDGETS fields = "__all__" -class SocketComputeForm(forms.ModelForm): +class SocketComputeForm(KeepSecretsMixin, forms.ModelForm): hostname = forms.CharField(widget=forms.HiddenInput, initial="localhost") type = forms.IntegerField(widget=forms.HiddenInput, initial=CONN_SOCKET) class Meta: model = Compute - fields = ["name", "details", "hostname", "type", "migration_address"] + widgets = SECRET_WIDGETS + fields = ["name", "details", "hostname", "type", "migration_address", "gstfsd_key"] diff --git a/computes/migrations/0007_gstfsd_key.py b/computes/migrations/0007_gstfsd_key.py new file mode 100644 index 00000000..5daa593b --- /dev/null +++ b/computes/migrations/0007_gstfsd_key.py @@ -0,0 +1,19 @@ +# Generated by Django 5.2.18 on 2026-10-30 22:36 + +import computes.validators +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('computes', '0006_login_password_length'), + ] + + operations = [ + migrations.AddField( + model_name='compute', + name='gstfsd_key', + field=models.CharField(blank=True, default='', help_text="GSTFSD_KEY in the host's /etc/default/webvirt-gstfsd: the panel sets the root password and SSH key of a shut-off VM with it; empty: they cannot be set there", max_length=64, validators=[computes.validators.validate_gstfsd_key], verbose_name='gstfsd key'), + ), + ] diff --git a/computes/models.py b/computes/models.py index 961f3740..91633534 100644 --- a/computes/models.py +++ b/computes/models.py @@ -6,7 +6,7 @@ from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ from libvirt import libvirtError, virConnect -from computes.validators import validate_hostname, validate_login, validate_migration_address +from computes.validators import validate_gstfsd_key, validate_hostname, validate_login, validate_migration_address from vrtManager.connection import connection_manager from vrtManager.hostdetails import wvmHostDetails @@ -29,6 +29,17 @@ class Compute(Model): "empty: the host's own host name, which they must be able to resolve" ), ) + gstfsd_key = CharField( + _("gstfsd key"), + max_length=64, + blank=True, + default="", + validators=[validate_gstfsd_key], + help_text=_( + "GSTFSD_KEY in the host's /etc/default/webvirt-gstfsd: the panel sets the root password and SSH key " + "of a shut-off VM with it; empty: they cannot be set there" + ), + ) @property def migration_uri(self): diff --git a/computes/tests/test_gstfsd_key.py b/computes/tests/test_gstfsd_key.py new file mode 100644 index 00000000..f22875aa --- /dev/null +++ b/computes/tests/test_gstfsd_key.py @@ -0,0 +1,49 @@ +"""The gstfsd key of a compute, like its password: never shown, kept when +left empty on an edit, and only 64 hexadecimal characters.""" + +from django.contrib.auth import get_user_model +from django.test import TestCase +from django.urls import reverse + +from computes.models import Compute + +KEY, OTHER = "5e" * 32, "a1" * 32 + + +class GstfsdKeyTests(TestCase): + def setUp(self): + self.client.force_login(get_user_model().objects.create_superuser("gk-admin", "gk@example.com", "pw")) + + def edit(self, compute, key): + data = {"name": compute.name, "hostname": compute.hostname, "login": "admin", "password": "", + "type": compute.type, "details": "", "gstfsd_key": key} + self.client.post(reverse("compute_update", args=[compute.id]), data) + compute.refresh_from_db() + return compute.gstfsd_key + + def test_every_kind_of_compute_takes_it_and_keeps_it(self): + for conn_type, hostname in ((1, "192.0.2.10"), (2, "192.0.2.11"), (3, "192.0.2.12"), (4, "localhost")): + with self.subTest(type=conn_type): + compute = Compute.objects.create(name=f"gk{conn_type}", hostname=hostname, login="admin", type=conn_type) + self.assertEqual(self.edit(compute, KEY), KEY) + self.assertEqual(self.edit(compute, ""), KEY) + self.assertEqual(self.edit(compute, OTHER), OTHER) + page = self.client.get(reverse("compute_update", args=[compute.id])).content.decode() + self.assertIn('name="gstfsd_key"', page) + self.assertNotIn(OTHER, page) + + def test_a_key_that_is_not_64_hexadecimal_characters_is_refused(self): + compute = Compute.objects.create(name="gk-bad", hostname="192.0.2.13", login="admin", type=1, gstfsd_key=KEY) + for bad in ("5E" * 32, "5e" * 31, "zz" * 32): + with self.subTest(key=bad): + self.assertEqual(self.edit(compute, bad), KEY) + + def test_the_api_takes_it_but_never_shows_it(self): + compute = Compute.objects.create(name="gk-api", hostname="192.0.2.14", login="admin", type=1) + url = f"/api/v1/computes/{compute.id}/" + self.client.patch(url, {"gstfsd_key": KEY}, content_type="application/json") + self.client.patch(url, {"gstfsd_key": ""}, content_type="application/json") + self.assertEqual(self.client.patch(url, {"gstfsd_key": "x"}, content_type="application/json").status_code, 400) + compute.refresh_from_db() + self.assertEqual(compute.gstfsd_key, KEY) + self.assertNotIn("gstfsd_key", self.client.get(url).json()) diff --git a/computes/validators.py b/computes/validators.py index 7c6080cc..4e3620f5 100644 --- a/computes/validators.py +++ b/computes/validators.py @@ -51,3 +51,10 @@ def validate_migration_address(value): if _hostname.fullmatch(value): return raise ValidationError(_("Enter an IP address or a host name, without a scheme or port")) + + +def validate_gstfsd_key(value): + """The key gstfsd on the host signs with: GSTFSD_KEY in its + /etc/default/webvirt-gstfsd, 64 lowercase hexadecimal characters.""" + if not re.fullmatch(r"[0-9a-f]{64}", value): + raise ValidationError(_("Enter the 64 hexadecimal characters of GSTFSD_KEY in the host's /etc/default/webvirt-gstfsd")) diff --git a/conf/daemon/gstfsd b/conf/daemon/gstfsd index e74034b5..8ad51e5e 100644 --- a/conf/daemon/gstfsd +++ b/conf/daemon/gstfsd @@ -7,11 +7,26 @@ # killed, with its guestfs appliance, when the request is not done DEADLINE # seconds after the connection was accepted. # +# Only the panel may ask: it knows this host's key (GSTFSD_KEY, 64 hex +# characters, the gstfsd key of the host in the panel). Each connection gets +# a new challenge; the request and the reply carry an HMAC-SHA256 of it: +# +# gstfsd -> panel {"challenge": C}\n +# panel -> gstfsd {"nonce": N, "request": R, "mac": mac("request", C, N, R)} +# gstfsd -> panel {"reply": A, "mac": mac("reply", C, N, A)} +# +# R and A are the request and the reply as JSON text. A request recorded on +# one connection is refused on any other, and only the panel's own +# connection can get a reply it accepts. The traffic is not encrypted. +# +import hashlib +import hmac import ipaddress import json import os import re +import secrets import select import signal import socket @@ -21,6 +36,8 @@ import sys import threading import time +HEX64 = re.compile(r"[0-9a-f]{64}") +KEY = os.environ.get("GSTFSD_KEY", "").strip() PORT = int(os.environ.get("GSTFSD_PORT", 16510)) ADDRESS = os.environ.get("GSTFSD_BIND_HOST", "127.0.0.1") MAX_REQUEST = 64 * 1024 @@ -187,6 +204,22 @@ def read_request(sock, end): continue +def mac(kind, challenge, nonce, payload): + message = "\n".join(("gstfsd-v1", kind, challenge, nonce, payload)).encode() + return hmac.new(bytes.fromhex(KEY), message, hashlib.sha256).hexdigest() + + +def signed_request(sock, end, challenge): + """The client's nonce and request, if the panel signed it for this + connection's challenge; PermissionError otherwise, before any change.""" + frame = read_request(sock, end) + nonce, request, given = (frame.get(key) if isinstance(frame, dict) else None for key in ("nonce", "request", "mac")) + if not (isinstance(nonce, str) and HEX64.fullmatch(nonce) and isinstance(request, str) + and isinstance(given, str) and hmac.compare_digest(given, mac("request", challenge, nonce, request))): + raise PermissionError("the request is not signed with this host's gstfsd key") + return nonce, json.loads(request) + + def group_running(pgid): """Whether a process of the group still runs. A killed appliance whose parent is gone may stay a zombie until init reaps it; it holds no disk.""" @@ -268,29 +301,40 @@ def run_worker(data, end, sock): class MyTCPServerHandler(socketserver.StreamRequestHandler): def handle(self): - # Exactly one reply per request, success or error. + # Exactly one reply per request, success or error; signed only for a + # signed request. end = time.monotonic() + DEADLINE - try: - reply = run_worker(read_request(self.request, end), end, self.request) - except Exception as err: - reply = {'return': 'error', 'message': str(err)} + challenge, nonce = secrets.token_hex(32), None try: self.request.settimeout(REPLY_TIMEOUT) - self.request.sendall(json.dumps(reply).encode()) + self.request.sendall((json.dumps({"challenge": challenge}) + "\n").encode()) + nonce, data = signed_request(self.request, end, challenge) + reply = run_worker(data, end, self.request) + except Exception as err: + reply = {'return': 'error', 'message': str(err)} + answer = json.dumps(reply) + frame = {"reply": answer, "mac": mac("reply", challenge, nonce, answer) if nonce else ""} + try: + self.request.settimeout(REPLY_TIMEOUT) + self.request.sendall(json.dumps(frame).encode()) except OSError: pass # the client is gone def serve(): - # gstfsd is an unauthenticated root service that rewrites any VM's - # /etc/shadow and authorized_keys, so it only listens on loopback unless - # explicitly allowed. + # gstfsd is a root service that rewrites any VM's /etc/shadow and + # authorized_keys; its traffic is not encrypted, so it only listens on + # loopback unless explicitly allowed. + if not HEX64.fullmatch(KEY): + sys.stderr.write("gstfsd: GSTFSD_KEY must be 64 hexadecimal characters (lowercase); " + "set it in /etc/default/webvirt-gstfsd\n") + return 1 if not 0 < DEADLINE <= MAX_DEADLINE: sys.stderr.write("gstfsd: GSTFSD_DEADLINE must be above 0 and at most %d seconds\n" % MAX_DEADLINE) return 1 if not is_loopback(ADDRESS) and os.environ.get("GSTFSD_ALLOW_REMOTE") != "1": sys.stderr.write( - "gstfsd: refusing to listen on %s (unauthenticated root service). " + "gstfsd: refusing to listen on %s (a root service whose traffic is not encrypted). " "Set GSTFSD_ALLOW_REMOTE=1 to allow it on a trusted network.\n" % ADDRESS ) return 1 diff --git a/conf/systemd/webvirt-gstfsd.service b/conf/systemd/webvirt-gstfsd.service index ead89d6e..63d1b6e0 100644 --- a/conf/systemd/webvirt-gstfsd.service +++ b/conf/systemd/webvirt-gstfsd.service @@ -4,6 +4,8 @@ Wants=network-online.target After=network-online.target [Service] +# GSTFSD_KEY, the key the panel signs with (root, mode 600) +EnvironmentFile=/etc/default/webvirt-gstfsd ExecStart=/usr/bin/python3 /usr/local/bin/gstfsd Restart=on-failure RestartSec=10 diff --git a/dev/libvirt-bootstrap.sh b/dev/libvirt-bootstrap.sh index d0f7fd2a..980ae600 100644 --- a/dev/libvirt-bootstrap.sh +++ b/dev/libvirt-bootstrap.sh @@ -386,8 +386,15 @@ __check_end_of_life_versions # CentOS Install Functions # # gstfsd and its systemd unit, enabled and (re)started: from the clone this -# script runs in, else from GitHub +# script runs in, else from GitHub. Its key is made once and kept: the +# panel's copy of it (the host's gstfsd key) stays valid. install_gstfsd() { + if [ ! -s /etc/default/webvirt-gstfsd ]; then + key="$(python3 -c 'import secrets; print(secrets.token_hex(32))')" || return 1 + (umask 077 && printf 'GSTFSD_KEY=%s\n' "$key" > /etc/default/webvirt-gstfsd) || return 1 + fi + # only root reads it, also when it was there already + chown root:root /etc/default/webvirt-gstfsd && chmod 600 /etc/default/webvirt-gstfsd || return 1 src="$(dirname "$0")/.." if [ -f "$src/conf/daemon/gstfsd" ]; then install -m 755 "$src/conf/daemon/gstfsd" /usr/local/bin/gstfsd && diff --git a/instances/tests/test_gstfsd.py b/instances/tests/test_gstfsd.py index f1a691e8..0620c93f 100644 --- a/instances/tests/test_gstfsd.py +++ b/instances/tests/test_gstfsd.py @@ -1,3 +1,6 @@ +import contextlib +import hashlib +import hmac import json import os import socket @@ -7,8 +10,41 @@ import tempfile import time import unittest from pathlib import Path +from types import SimpleNamespace GSTFSD = Path(__file__).resolve().parent.parent.parent / "conf" / "daemon" / "gstfsd" +# the host's key (GSTFSD_KEY) and the panel's copy of it +KEY = "5e" * 32 + + +def mac(kind, challenge, nonce, payload, key=KEY): + message = "\n".join(("gstfsd-v1", kind, challenge, nonce, payload)).encode() + return hmac.new(bytes.fromhex(key), message, hashlib.sha256).hexdigest() + + +def challenge_of(s): + """The challenge gstfsd sends first, on one line.""" + line = b"" + while not line.endswith(b"\n"): + chunk = s.recv(1) + if not chunk: + raise ConnectionError("gstfsd closed the connection") + line += chunk + return json.loads(line)["challenge"] + + +def signed(challenge, data, key=KEY, nonce="0f" * 32): + """The request frame of the panel for this challenge.""" + request = json.dumps(data) + return json.dumps({"nonce": nonce, "request": request, "mac": mac("request", challenge, nonce, request, key)}).encode() + + +def reply_of(s, challenge=None, nonce="0f" * 32): + """gstfsd's reply; with the challenge, checked to be signed for it.""" + frame = json.loads(b"".join(iter(lambda: s.recv(4096), b""))) + if challenge is not None: + assert hmac.compare_digest(frame["mac"], mac("reply", challenge, nonce, frame["reply"])), frame + return json.loads(frame["reply"]) # A stand-in for libguestfs: the guest's operating systems ("roots", each with # its mountpoints), its files, and what is mounted live in a JSON file. @@ -116,7 +152,7 @@ class GstfsdTestCase(unittest.TestCase): os.environ, PYTHONPATH=self.tmp.name, FAKE_GUESTFS_STATE=str(self.state), - **extra, + **{"GSTFSD_KEY": KEY, **extra}, ) def _start(self, host, port, family=socket.AF_INET, **env): @@ -144,9 +180,10 @@ class GstfsdTestCase(unittest.TestCase): with socket.socket(family, socket.SOCK_STREAM) as s: s.settimeout(10) s.connect((host, port)) - s.sendall(json.dumps(data).encode()) + challenge = challenge_of(s) + s.sendall(signed(challenge, data)) # everything until gstfsd closes: more than one reply fails to parse - return json.loads(b"".join(iter(lambda: s.recv(4096), b""))) + return reply_of(s, challenge) def test_refuses_non_loopback_bind_without_opt_in(self): result = subprocess.run( @@ -202,6 +239,59 @@ class GstfsdTestCase(unittest.TestCase): self._start("127.0.0.1", port) return port + def test_only_a_request_signed_for_this_connection_is_done(self): + port = self._serve() + data = {"action": "password", "vname": "vm", "passwd": "$6$intruder"} + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.settimeout(10) + s.connect(("127.0.0.1", port)) + recorded = signed(challenge_of(s), data) # a request of another connection, never sent there + + def tampered(challenge): + frame = json.loads(signed(challenge, {"action": "password", "vname": "vm", "passwd": "$6$ok"})) + frame["request"] = json.dumps(data) + return json.dumps(frame).encode() + + for name, frame in ( + ("unsigned", lambda challenge: json.dumps(data).encode()), + ("another key", lambda challenge: signed(challenge, data, key="a1" * 32)), + ("changed after signing", tampered), + ("recorded on another connection", lambda challenge: recorded), + ("a nonce that is no nonce", lambda challenge: signed(challenge, data, nonce="x")), + ): + with self.subTest(name), socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.settimeout(10) + s.connect(("127.0.0.1", port)) + s.sendall(frame(challenge_of(s))) + reply = reply_of(s) + self.assertEqual(reply, {"return": "error", "message": "the request is not signed with this host's gstfsd key"}) + state = json.loads(self.state.read_text()) + self.assertEqual((state["writes"], state["history"]), (0, [])) # no appliance was started + + def test_it_does_not_start_without_a_key(self): + for key in ("", "5E" * 32, "5e" * 31): + with self.subTest(key=key): + result = subprocess.run([sys.executable, str(GSTFSD)], env=self._env(GSTFSD_KEY=key), + capture_output=True, text=True, timeout=20) + self.assertNotEqual(result.returncode, 0) + self.assertIn("GSTFSD_KEY", result.stderr) + + def test_the_panel_asks_with_the_host_s_key(self): + from unittest.mock import patch + + from instances import views + from vrtManager.util import OperationError + + port = self._serve() + compute = SimpleNamespace(name="host-a", hostname="127.0.0.1", gstfsd_key=KEY) + with patch.object(views, "GSTFSD_PORT", port): + self.assertEqual(views.gstfsd_request(compute, {"action": "password", "vname": "vm", "passwd": "$6$panel"}), + {"return": "success"}) + compute.gstfsd_key = "a1" * 32 # the key of another host + with self.assertRaisesRegex(OperationError, "not signed with this host's gstfsd key"): + views.gstfsd_request(compute, {"action": "password", "vname": "vm", "passwd": "$6$other"}) + self.assertIn("root:$6$panel:", json.loads(self.state.read_text())["files"]["/etc/shadow"]) + def test_root_password_is_set_on_the_guest_root(self): reply = self._request("127.0.0.1", self._serve(), {"action": "password", "vname": "vm", "passwd": "$6$new"}) self.assertEqual(reply, {"return": "success"}) @@ -278,14 +368,15 @@ class GstfsdTestCase(unittest.TestCase): def test_a_request_in_two_pieces(self): port = self._serve() - body = json.dumps({"action": "password", "vname": "vm", "passwd": "$6$parts"}).encode() with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.settimeout(10) s.connect(("127.0.0.1", port)) + challenge = challenge_of(s) + body = signed(challenge, {"action": "password", "vname": "vm", "passwd": "$6$parts"}) s.sendall(body[:10]) time.sleep(0.3) s.sendall(body[10:]) - reply = json.loads(b"".join(iter(lambda: s.recv(4096), b""))) + reply = reply_of(s, challenge) self.assertEqual(reply, {"return": "success"}) def test_a_request_over_the_limit_is_refused(self): @@ -293,8 +384,8 @@ class GstfsdTestCase(unittest.TestCase): with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.settimeout(10) s.connect(("127.0.0.1", port)) - s.sendall(b'{"action": "publickey", "vname": "vm", "key": "' + b"A" * 70000 + b'"}') - reply = json.loads(b"".join(iter(lambda: s.recv(4096), b""))) + s.sendall(signed(challenge_of(s), {"action": "publickey", "vname": "vm", "key": "A" * 70000})) + reply = reply_of(s) self.assertEqual(reply["return"], "error") self.assertEqual(json.loads(self.state.read_text())["writes"], 0) @@ -303,6 +394,7 @@ class GstfsdTestCase(unittest.TestCase): self._start("127.0.0.1", port, GSTFSD_DEADLINE="2") with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.connect(("127.0.0.1", port)) + challenge_of(s) s.settimeout(0.25) start = time.monotonic() reply = b"" @@ -316,7 +408,7 @@ class GstfsdTestCase(unittest.TestCase): except OSError: break self.assertTrue(reply, "gstfsd kept reading a request that never completes") - self.assertEqual(json.loads(reply)["return"], "error") + self.assertEqual(json.loads(json.loads(reply)["reply"])["return"], "error") def test_a_slow_write_is_stopped_at_the_deadline(self): # the whole change is bounded, not only its start: a write still @@ -391,7 +483,8 @@ class GstfsdTestCase(unittest.TestCase): proc = self._start("127.0.0.1", port, GSTFSD_DEADLINE="1") with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.connect(("127.0.0.1", port)) - s.sendall(b'{"action": ') + challenge_of(s) + s.sendall(b'{"nonce": ') time.sleep(1.5) # gstfsd times the request out and answers a closed socket reply = self._request("127.0.0.1", port, {"action": "password", "vname": "vm", "passwd": "$6$after"}) self.assertEqual(reply, {"return": "success"}) @@ -424,15 +517,16 @@ class GstfsdTestCase(unittest.TestCase): port = free_port() self._start("127.0.0.1", port, GSTFSD_DEADLINE="2") self._update_state(launch_seconds=3) - request = json.dumps({"action": "password", "vname": "vm", "passwd": "$6$slow"}).encode() with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.settimeout(10) s.connect(("127.0.0.1", port)) start = time.monotonic() + challenge = challenge_of(s) + request = signed(challenge, {"action": "password", "vname": "vm", "passwd": "$6$slow"}) s.sendall(request[:10]) time.sleep(1.5) s.sendall(request[10:]) - reply = json.loads(b"".join(iter(lambda: s.recv(4096), b""))) + reply = reply_of(s, challenge) self.assertEqual(reply["return"], "error") self.assertLess(time.monotonic() - start, 2.8) @@ -450,7 +544,7 @@ class GstfsdTestCase(unittest.TestCase): self._update_state(launch_seconds=2) with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.connect(("127.0.0.1", port)) - s.sendall(json.dumps({"action": "password", "vname": "vm", "passwd": "$6$gone"}).encode()) + s.sendall(signed(challenge_of(s), {"action": "password", "vname": "vm", "passwd": "$6$gone"})) time.sleep(0.5) time.sleep(3) # longer than the change would have taken self.assertEqual(json.loads(self.state.read_text())["writes"], 0) @@ -471,9 +565,10 @@ class GstfsdTestCase(unittest.TestCase): class GstfsdClientTestCase(unittest.TestCase): - """The panel side sends the whole request and reads the whole reply.""" + """The panel side checks that the reply is gstfsd's, signed for its request.""" - def test_a_reply_in_pieces_is_read_whole(self): + def ask(self, *sent): + """gstfsd_request against a server that sends these, in pieces.""" import threading from unittest.mock import patch @@ -483,20 +578,45 @@ class GstfsdClientTestCase(unittest.TestCase): server.bind(("127.0.0.1", 0)) server.listen(1) self.addCleanup(server.close) - received = [] def serve(): conn, _ = server.accept() - with conn: - received.append(conn.recv(65536)) - conn.sendall(b'{"return": ') - time.sleep(0.2) - conn.sendall(b'"success"}') + with conn, contextlib.suppress(OSError): # the panel may hang up first + conn.sendall(sent[0]) + conn.recv(65536) + for piece in sent[1:]: + time.sleep(0.1) + conn.sendall(piece) thread = threading.Thread(target=serve) thread.start() + self.addCleanup(thread.join, 5) + compute = SimpleNamespace(name="host-a", hostname="127.0.0.1", gstfsd_key=KEY) with patch.object(views, "GSTFSD_PORT", server.getsockname()[1]): - reply = views.gstfsd_request("127.0.0.1", {"action": "publickey", "vname": "vm", "key": "k"}) - thread.join(5) - self.assertEqual(reply, {"return": "success"}) - self.assertEqual(json.loads(received[0])["vname"], "vm") + return views.gstfsd_request(compute, {"action": "publickey", "vname": "vm", "key": "k"}) + + def test_a_reply_that_is_not_signed_is_refused(self): + from vrtManager.util import OperationError + + challenge = json.dumps({"challenge": "1a" * 32}).encode() + b"\n" + # something else on the port, which claims success in pieces, or sends what is no reply + for name, sent in ( + ("unsigned success", (challenge, b'{"reply": "{\\"return\\": ', b'\\"success\\"}", "mac": ""}')), + ("a mac that is no hex", (challenge, b'{"reply": "{}", "mac": "\xc3\xa9"}')), + ("a mac that is no text", (challenge, b'{"reply": "{}", "mac": "\\ud800"}')), + ("a challenge line without end", (b"{" + b" " * 70000,)), + ("a reply too long", (challenge, b'{"reply": "' + b"x" * 70000 + b'"}')), + ): + with self.subTest(name), self.assertRaises(OperationError): + self.ask(*sent) + + def test_a_host_without_a_key_is_not_asked(self): + from unittest.mock import patch + + from instances import views + from vrtManager.util import OperationError + + with patch("instances.views.socket.create_connection") as connect: + with self.assertRaisesRegex(OperationError, "Set the gstfsd key of host-a first"): + views.gstfsd_request(SimpleNamespace(name="host-a", hostname="127.0.0.1", gstfsd_key=""), {}) + connect.assert_not_called() diff --git a/instances/tests/test_root_pass.py b/instances/tests/test_root_pass.py index 0c889faf..49e4dc63 100644 --- a/instances/tests/test_root_pass.py +++ b/instances/tests/test_root_pass.py @@ -1,6 +1,4 @@ -import json -import socket -from unittest.mock import MagicMock, patch +from unittest.mock import patch from django.contrib.messages import get_messages from django.test import RequestFactory, TestCase @@ -9,6 +7,7 @@ from django.urls import reverse from computes.models import Compute from instances.models import Instance from libvirt import libvirtError +from vrtManager.util import OperationError class SetRootPassTestCase(TestCase): @@ -25,17 +24,12 @@ class SetRootPassTestCase(TestCase): uuid="87654321-4321-4321-4321-210987654321", ) - @patch("socket.socket") + @patch("instances.views.gstfsd_request") @patch.object(Instance, "proxy") - def test_set_root_pass_post(self, mock_proxy, mock_socket_cls): + def test_set_root_pass_post(self, mock_proxy, gstfsd): # Renamed on the host: gstfsd must get the name of this UUID's domain mock_proxy.instance.name.return_value = "renamed-on-host" mock_proxy.get_status.return_value = 5 # shut off - mock_sock = MagicMock() - mock_socket_cls.return_value = mock_sock - replies = iter([json.dumps({"return": "success"}).encode(), b""] * 2) - # one reply, then gstfsd closes the connection - mock_sock.recv.side_effect = lambda size: next(replies) response = self.client.post( reverse("instances:rootpasswd", args=[self.instance.id]), @@ -44,10 +38,9 @@ class SetRootPassTestCase(TestCase): # Redirects back to referer or instance detail self.assertEqual(response.status_code, 302) - # Check socket sent the command with hashed password - self.assertTrue(mock_sock.sendall.called) - sent_bytes = mock_sock.sendall.call_args[0][0] - data = json.loads(sent_bytes.decode()) + # gstfsd of the VM's host gets the hashed password + compute, data = gstfsd.call_args.args + self.assertEqual(compute, self.compute) self.assertEqual(data["action"], "password") self.assertEqual(data["vname"], "renamed-on-host") # SHA-512 crypt with a random salt, not the old fixed one @@ -62,44 +55,23 @@ class SetRootPassTestCase(TestCase): response = self.client.post(reverse("instances:rootpasswd", args=[99999]), {"passwd": "secret"}) self.assertEqual(response.status_code, 404) - @patch("socket.socket") + @patch("instances.views.gstfsd_request") @patch.object(Instance, "proxy") - def test_same_password_gets_a_different_salt_each_time(self, mock_proxy, mock_socket_cls): + def test_same_password_gets_a_different_salt_each_time(self, mock_proxy, gstfsd): mock_proxy.instance.name.return_value = "root-pass-vm" mock_proxy.get_status.return_value = 5 - mock_sock = MagicMock() - mock_socket_cls.return_value = mock_sock - replies = iter([json.dumps({"return": "success"}).encode(), b""] * 2) - # one reply, then gstfsd closes the connection - mock_sock.recv.side_effect = lambda size: next(replies) url = reverse("instances:rootpasswd", args=[self.instance.id]) self.client.post(url, {"passwd": "samePassword1"}) self.client.post(url, {"passwd": "samePassword1"}) - hashes = [json.loads(c[0][0].decode())["passwd"] for c in mock_sock.sendall.call_args_list] + hashes = [c.args[1]["passwd"] for c in gstfsd.call_args_list] self.assertEqual(len(hashes), 2) self.assertNotEqual(hashes[0], hashes[1]) - @patch("socket.socket") + @patch("instances.views.gstfsd_request", side_effect=OperationError("gstfsd error: timed out")) @patch.object(Instance, "proxy") - def test_unreachable_gstfsd_is_an_error_message_not_a_crash(self, mock_proxy, mock_socket_cls): - mock_proxy.instance.name.return_value = "root-pass-vm" - mock_proxy.get_status.return_value = 5 - mock_sock = MagicMock() - mock_socket_cls.return_value = mock_sock - mock_sock.connect.side_effect = socket.timeout("timed out") - - response = self.client.post( - reverse("instances:rootpasswd", args=[self.instance.id]), {"passwd": "x"} - ) - - self.assertEqual(response.status_code, 302) - mock_sock.settimeout.assert_called() - - @patch("socket.socket") - @patch.object(Instance, "proxy") - def test_add_public_key_unreachable_gstfsd_is_an_error_message(self, mock_proxy, mock_socket_cls): + def test_unreachable_gstfsd_is_an_error_message_not_a_crash(self, mock_proxy, gstfsd): from accounts.models import UserSSHKey from django.contrib.auth import get_user_model @@ -107,16 +79,11 @@ class SetRootPassTestCase(TestCase): key = UserSSHKey.objects.create(user=admin, keyname="k", keypublic="ssh-ed25519 AAAA k") mock_proxy.instance.name.return_value = "root-pass-vm" mock_proxy.get_status.return_value = 5 - mock_sock = MagicMock() - mock_socket_cls.return_value = mock_sock - mock_sock.connect.side_effect = ConnectionRefusedError() - - response = self.client.post( - reverse("instances:add_public_key", args=[self.instance.id]), {"sshkeyid": key.id} - ) - - self.assertEqual(response.status_code, 302) - mock_sock.settimeout.assert_called() + for name, data in (("rootpasswd", {"passwd": "x"}), ("add_public_key", {"sshkeyid": key.id})): + with self.subTest(name): + response = self.client.post(reverse(f"instances:{name}", args=[self.instance.id]), data) + self.assertEqual(response.status_code, 302) + self.assertEqual([str(m) for m in get_messages(response.wsgi_request)][-1], "gstfsd error: timed out") class GuestAgentAccessTestCase(TestCase): diff --git a/instances/views.py b/instances/views.py index 4a806e5c..7b7a6bc4 100755 --- a/instances/views.py +++ b/instances/views.py @@ -4,9 +4,12 @@ except ImportError: import crypt import difflib import functools +import hashlib +import hmac import json import os import re +import secrets import socket import subprocess import time @@ -955,24 +958,51 @@ def migrate_plan(request, pk): GSTFSD_PORT = 16510 GSTFSD_TIMEOUT = 120 # gstfsd answers within its 100 s limit for a request (GSTFSD_DEADLINE) +GSTFSD_MAX_FRAME = 64 * 1024 # a challenge or a reply of gstfsd is far shorter -def gstfsd_request(hostname, data): - """Send one request to gstfsd and return its JSON reply; an error reply or - a failed exchange raises OperationError with the reason.""" +def gstfsd_mac(key, kind, challenge, nonce, payload): + """The HMAC-SHA256 gstfsd signs and checks with (see conf/daemon/gstfsd).""" + message = "\n".join(("gstfsd-v1", kind, challenge, nonce, payload)).encode() + return hmac.new(bytes.fromhex(key), message, hashlib.sha256).hexdigest() + + +def gstfsd_request(compute, data): + """Send one request to the compute's gstfsd, signed with its key, and + return the reply gstfsd signed for it; an error reply or a failed or + unsigned exchange raises OperationError with the reason.""" + if not compute.gstfsd_key: + raise util.OperationError(_("Set the gstfsd key of %(compute)s first (Computes, edit the host)") % { + "compute": compute.name}) try: # the host name may resolve to IPv4 or IPv6 - s = socket.create_connection((hostname, GSTFSD_PORT), timeout=GSTFSD_TIMEOUT) + s = socket.create_connection((compute.hostname, GSTFSD_PORT), timeout=GSTFSD_TIMEOUT) except OSError as err: raise util.OperationError(_("gstfsd error: %(err)s") % {"err": err}) from err try: - s.sendall(json.dumps(data).encode()) + stream = s.makefile("rb") + challenge = json.loads(stream.readline(GSTFSD_MAX_FRAME))["challenge"] + nonce, request = secrets.token_hex(32), json.dumps(data) + s.sendall(json.dumps({"nonce": nonce, "request": request, + "mac": gstfsd_mac(compute.gstfsd_key, "request", challenge, nonce, request)}).encode()) # gstfsd sends one reply and closes the connection - reply = json.loads(b"".join(iter(lambda: s.recv(4096), b""))) - except (OSError, ValueError) as err: + answer = stream.read(GSTFSD_MAX_FRAME + 1) + if len(answer) > GSTFSD_MAX_FRAME: + raise ValueError("the reply is too long") + frame = json.loads(answer) + reply = json.loads(frame["reply"]) + except (OSError, ValueError, KeyError, TypeError) as err: raise util.OperationError(_("gstfsd error: %(err)s") % {"err": err}) from err finally: s.close() + given = frame.get("mac") + if not (isinstance(given, str) and re.fullmatch(r"[0-9a-f]{64}", given) and hmac.compare_digest( + given, gstfsd_mac(compute.gstfsd_key, "reply", challenge, nonce, frame["reply"]))): + # gstfsd refused the request (a wrong key): its error says so; any + # other reply that is not signed is no answer of gstfsd + refused = isinstance(reply, dict) and reply.get("return") == "error" + raise util.OperationError(_("gstfsd error: %(err)s") % { + "err": reply.get("message") if refused else _("the reply is not signed with the key")}) if reply["return"] != "success": raise util.OperationError(reply["message"]) return reply @@ -1014,7 +1044,7 @@ def set_root_pass(request, pk): try: with audit_vm(request, instance, "reset root password"): if status == 5: - gstfsd_request(instance.compute.hostname, data) + gstfsd_request(instance.compute, data) else: instance.proxy.set_user_password("root", passwd_hash) except (util.OperationError, libvirtError) as err: @@ -1054,7 +1084,7 @@ def add_public_key(request, pk): try: with audit_vm(request, instance, f"install SSH key {publickey.keyname}"): if status == 5: - gstfsd_request(instance.compute.hostname, data) + gstfsd_request(instance.compute, data) else: instance.proxy.add_authorized_key("root", publickey.keypublic) except (util.OperationError, libvirtError) as err: