fix(security): close file-read RBAC gap + fail2ban arg-injection + update/HMAC hardening

Confirmed-security quick-wins from a full re-audit (6-agent subsystem fan-out,
Codex-reviewed). Each item ships with a regression test proven to fail pre-fix.

- files: gate file CONTENT reads (download / edit / FileEditor::load) behind the
  `operate` ability so a read-only viewer keeps LISTING/browse access but can no
  longer pull file bytes over the server credential (root -> /etc/shadow, keys,
  .env). Hide the download/edit/delete controls from viewers in the blade, and
  basename() Files::open() for parity with the other path builders.
- fail2ban: reject a leading dash/dot in validJail(). A jail name like "-h" or
  "--help" was parsed by fail2ban-client as an OPTION (argument-injection) rather
  than a positional jail; internal dashes ("nginx-http-auth") still pass.
- bruteforce: release the atomically-claimed ban-audit dedup slot when the
  AuditEvent write throws, so a transient DB failure no longer suppresses the
  auth.ip_banned audit for the whole 60s window (the ban itself is unaffected).
- update path: verify_update_request now FAILS CLOSED when UPDATE_HMAC_KEY is
  unset (was fail-open -> any ./run marker could drive a root update). Pin the
  self-update `git pull` to the recorded CLUSEV_BUILD_BRANCH and refuse a diverged
  checkout, so a `git checkout` in the tree cannot redirect the next root update.
- wireguard: normalise the DNS input to a single comma list and QUOTE the WG_DNS
  assignment in wg.env; an unquoted space previously split the assignment and
  broke the next `. "$WG_ENV"`.

13 new regression tests. 614 tests, Pint, shellcheck, Codex review all green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat/v1-foundation
boban 2026-07-05 16:05:16 +02:00
parent c914790a46
commit bc2f2b527f
12 changed files with 370 additions and 32 deletions

View File

@ -60,7 +60,11 @@ class Index extends Component
return;
}
$this->path = rtrim($this->path, '/').'/'.$name;
// basename() for parity with download()/edit()/confirmDelete(): a listing entry
// name is a single path segment, never a traversal fragment. $entries is a public,
// non-#[Locked] property, so a crafted /livewire/update could otherwise set name to
// ../.. and walk outside the browsed tree.
$this->path = rtrim($this->path, '/').'/'.basename($name);
$this->load();
}
@ -102,6 +106,11 @@ class Index extends Component
/** Stream a remote file to the browser as a download (SFTP get). */
public function download(int $index, FleetService $fleet)
{
// Reading a file's BYTES over the server credential (often root) discloses secrets a
// viewer must not reach (/etc/shadow, keys, .env). Browsing the LISTING stays open;
// pulling content requires operate, like every other file operation.
abort_unless(auth()->user()?->can('operate'), 403);
$name = $this->entries[$index]['name'] ?? null;
$active = $this->activeServer();
if ($name === null || ! $active || ! $active->credential_exists) {
@ -122,6 +131,10 @@ class Index extends Component
/** Open the view/edit modal for a file. */
public function edit(int $index): void
{
// The editor reads (and can write) file content — an operate action. Gate here so a
// viewer never even opens the modal; FileEditor::load() re-checks on the read itself.
abort_unless(auth()->user()?->can('operate'), 403);
$name = $this->entries[$index]['name'] ?? null;
$active = $this->activeServer();
if ($name === null || ! $active) {

View File

@ -78,6 +78,11 @@ class FileEditor extends ModalComponent
public function load(FleetService $fleet, Sftp $sftp): void
{
// The actual content read (SFTP get / readFile). Gate it directly — not just the
// Files::edit entry point — so file bytes the server credential can reach (root →
// /etc/shadow, keys, .env) are never disclosed to a read-only viewer.
abort_unless(auth()->user()?->can('operate'), 403);
$server = Server::find($this->serverId);
if (! $server) {
$this->error = __('common.server_not_found');

View File

@ -158,7 +158,9 @@ class BruteforceGuard
// honeypot ban) is still audited. A short 60s TTL (matching record()'s ban dedup) collapses the
// rapid scanner flood WITHOUT masking a legitimate later re-ban — e.g. after an operator unban,
// which is minutes of human time later, well past this window (so it is not tied to unban()).
if (Cache::add('bruteforce:banaudit:'.$canonical.':'.$reason, 1, 60)) {
$auditKey = 'bruteforce:banaudit:'.$canonical.':'.$reason;
if (Cache::add($auditKey, 1, 60)) {
try {
AuditEvent::create([
'user_id' => null,
'actor' => 'system',
@ -167,6 +169,13 @@ class BruteforceGuard
'ip' => $canonical,
'meta' => ['reason' => $reason],
]);
} catch (\Throwable $e) {
// The Cache::add above atomically CLAIMED the dedup slot before the write. If the
// write throws (a transient DB blip), release the slot so the 60s window is not
// burned with no audit row — the next attempt re-claims and re-audits this ban.
Cache::forget($auditKey);
throw $e;
}
}
return true;

View File

@ -398,7 +398,12 @@ class Fail2banService
private function validJail(string $jail): bool
{
return (bool) preg_match('/^[A-Za-z0-9._-]+$/', $jail);
// Must START with an alphanumeric/underscore: a leading '-' would be parsed by
// fail2ban-client as an OPTION (e.g. "-h", "--help") rather than a jail argument,
// and a leading '.' has no legitimate jail use. Internal '.-_' stay allowed so real
// names like "nginx-http-auth" pass. (Shell metacharacters are already inert via the
// base64 transport in runPrivileged; this closes the argument-injection confusion.)
return (bool) preg_match('/^[A-Za-z0-9_][A-Za-z0-9._-]*$/', $jail);
}
private function validIpOrCidr(string $v): bool

View File

@ -66,7 +66,11 @@ write_update_error() {
verify_update_request() {
local f="$1" key ts mac expect
key="$(grep -m1 '^UPDATE_HMAC_KEY=' "${CLUSEV_DIR}/.env" 2>/dev/null | cut -d= -f2-)"
[ -n "$key" ] || { log "UPDATE_HMAC_KEY unset — skipping request authentication"; return 0; }
# Fail CLOSED: with no key the marker cannot be authenticated, so refuse rather than run a
# ROOT update on an unauthenticated request. install.sh always backfills UPDATE_HMAC_KEY
# (set_kv), so a real install has one; a missing key means a broken/hand-edited .env — the
# operator re-runs install.sh (or sets the key) to restore dashboard-triggered updates.
[ -n "$key" ] || { log "UPDATE_HMAC_KEY unset — REFUSING update request (re-run install.sh to backfill the key)"; return 1; }
ts="$(sed -n '1p' "$f" 2>/dev/null)"
mac="$(sed -n '2p' "$f" 2>/dev/null)"
[ -n "$ts" ] && [ -n "$mac" ] || { log "update request unsigned/malformed — refusing"; return 1; }

View File

@ -509,14 +509,19 @@ _set_endpoint() {
_set_dns() {
local dns="$1"; require_setup
# One or more IPv4 addresses, comma/space separated (e.g. "10.0.0.1" or "1.1.1.1, 1.0.0.1").
# One or more IPv4 addresses, comma/space separated on INPUT (e.g. "10.0.0.1" or "1.1.1.1, 1.0.0.1").
# Only affects NEW peer configs (the DNS line is baked into each client config at creation).
case "$dns" in *[!0-9.,\ ]*|'') echo "bad dns" >&2; return 1 ;; esac
printf '%s' "$dns" | grep -qE '^[0-9]{1,3}(\.[0-9]{1,3}){3}([,[:space:]]+[0-9]{1,3}(\.[0-9]{1,3}){3})*$' || { echo "bad dns" >&2; return 1; }
# Normalise every comma/space run to a single comma before STORING: WireGuard's peer `DNS =`
# line wants a comma list, and — critically — an unquoted space in wg.env would split the
# WG_DNS assignment so the next `. "$WG_ENV"` runs the second address as a command. Store as a
# single comma-separated token and QUOTE the assignment (defence-in-depth) so sourcing is safe.
dns="$(printf '%s' "$dns" | tr -s ',[:space:]' ',' | sed -e 's/^,//' -e 's/,$//')"
if grep -q '^WG_DNS=' "$WG_ENV" 2>/dev/null; then
sed -i "s#^WG_DNS=.*#WG_DNS=${dns}#" "$WG_ENV"
sed -i "s#^WG_DNS=.*#WG_DNS=\"${dns}\"#" "$WG_ENV"
else
printf 'WG_DNS=%s\n' "$dns" >> "$WG_ENV" # older wg.env (pre-DNS) has no line yet
printf 'WG_DNS="%s"\n' "$dns" >> "$WG_ENV" # older wg.env (pre-DNS) has no line yet
fi
}

View File

@ -101,8 +101,10 @@
<span class="block truncate font-mono text-sm text-ink group-hover:text-accent-text">{{ $e['name'] }}/</span>
</button>
@else
@can('operate')
{{-- Filename opens the editor modal too inline modalTrigger keeps the
link layout but adds the pending state + timeout error toast. --}}
link layout but adds the pending state + timeout error toast.
operate-gated: reading content is not a viewer capability. --}}
<button type="button"
x-data="modalTrigger(null, {}, @js(__('shell.toast_modal_failed')), 3500)"
wire:click="edit({{ $loop->index }})" x-on:click="arm()"
@ -111,6 +113,10 @@
<span class="truncate">{{ $e['name'] }}</span>
<svg x-show="pending" x-cloak class="h-3 w-3 shrink-0 animate-spin" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2.5" aria-hidden="true"><path d="M21 12a9 9 0 1 1-6.219-8.56" stroke-linecap="round" /></svg>
</button>
@else
{{-- Viewer: read-only listing the filename is not a content-read trigger. --}}
<span class="block min-w-0 flex-1 truncate font-mono text-sm text-ink-2">{{ $e['name'] }}</span>
@endcan
@endif
</div>
@ -128,13 +134,14 @@
<span>{{ $e['modified'] }}</span>
</p>
{{-- Row actions --}}
{{-- Row actions content read (download/edit) and delete all require operate;
a viewer only browses the listing, so it sees no row actions. --}}
<div class="mt-2.5 flex flex-wrap items-center gap-1.5 pl-11 xl:mt-0 xl:justify-end xl:gap-1 xl:pl-0">
@can('operate')
@unless ($isDir)
<x-btn variant="secondary" wire:click="download({{ $loop->index }})">{{ __('files.download') }}</x-btn>
<x-modal-trigger variant="secondary" action="edit({{ $loop->index }})">{{ __('common.edit') }}</x-modal-trigger>
@endunless
@can('operate')
<x-modal-trigger variant="danger-soft" action="confirmDelete({{ $loop->index }})">{{ __('common.delete') }}</x-modal-trigger>
@endcan
</div>

View File

@ -2,6 +2,7 @@
namespace Tests\Feature;
use App\Models\AuditEvent;
use App\Models\BannedIp;
use App\Models\Setting;
use App\Services\BruteforceGuard;
@ -81,4 +82,49 @@ class BruteforceGuardBanTest extends TestCase
$g->record('203.0.113.5', 'login'); // ban via plain IPv4
$this->assertTrue($g->isBanned('::ffff:203.0.113.5')); // mapped form is the same ban
}
// ── banNow (honeypot first-hit ban) audit dedup ─────────────────────────────
public function test_bannow_audits_once_per_reason_within_the_window(): void
{
$g = $this->guard();
$g->banNow('203.0.113.8', 'honeypot');
$g->banNow('203.0.113.8', 'honeypot'); // same ip+reason inside 60s → the flood is deduped
$this->assertDatabaseCount('audit_events', 1);
$g->banNow('203.0.113.8', 'honeytoken'); // a DISTINCT reason is still audited
$this->assertDatabaseCount('audit_events', 2);
}
public function test_bannow_releases_the_audit_slot_when_the_write_fails(): void
{
$g = $this->guard();
// Make the FIRST audit insert throw (a transient DB blip), then heal.
$fail = true;
AuditEvent::creating(function () use (&$fail) {
if ($fail) {
$fail = false;
throw new \RuntimeException('db blip');
}
});
// The ban row must still be written; the audit fails and re-throws (the honeypot
// caller swallows it). The dedup slot must NOT stay claimed after a failed write.
try {
$g->banNow('203.0.113.7', 'honeypot');
} catch (\Throwable) {
// expected — banNow re-throws the audit failure
}
$this->assertDatabaseHas('banned_ips', ['ip' => '203.0.113.7']); // ban persisted despite audit failure
$this->assertDatabaseCount('audit_events', 0); // audit did not land
$this->assertFalse(Cache::has('bruteforce:banaudit:203.0.113.7:honeypot')); // slot released, not burned
// A retry inside the same 60s window now re-audits — the failure did not suppress it.
$g->banNow('203.0.113.7', 'honeypot');
$this->assertDatabaseHas('audit_events', ['action' => 'auth.ip_banned', 'ip' => '203.0.113.7']);
$this->assertDatabaseCount('audit_events', 1);
}
}

View File

@ -0,0 +1,99 @@
<?php
namespace Tests\Feature;
use App\Models\Server;
use App\Services\Fail2banService;
use App\Services\FleetService;
use App\Support\Os\OsDetector;
use App\Support\Os\OsProfile;
use Illuminate\Foundation\Testing\RefreshDatabase;
use Mockery;
use Tests\TestCase;
/**
* `fail2ban-client set <jail> banip <ip>` is built by interpolating the jail name. The
* base64 transport in runPrivileged() neutralises SHELL metacharacters, but a jail name
* beginning with `-` (e.g. "-h", "--help") is still parsed by fail2ban-client as an
* OPTION rather than a positional jail argument. The jail reaches this sink from a
* client-controlled source Fail2banBan::$jail (a wire:model property) and
* Fail2banBans::unbanIp($jail) (a method argument) so validJail() must reject a
* leading dash (and dot) while still accepting real jail names that contain an internal
* dash, like "nginx-http-auth".
*/
class Fail2banJailValidationTest extends TestCase
{
use RefreshDatabase;
protected function tearDown(): void
{
Mockery::close();
parent::tearDown();
}
private function server(): Server
{
return Server::create(['name' => 'box', 'ip' => '10.0.0.2', 'ssh_port' => 22, 'status' => 'online']);
}
/** FleetService whose privileged exec must NEVER run (guard fires first). */
private function fleetNeverRuns(): FleetService
{
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('runPrivileged')->never();
return $fleet;
}
private function service(FleetService $fleet, ?OsDetector $detector = null): Fail2banService
{
return new Fail2banService($fleet, $detector ?? Mockery::mock(OsDetector::class));
}
public function test_ban_rejects_a_jail_beginning_with_a_dash(): void
{
$res = $this->service($this->fleetNeverRuns())->ban($this->server(), '-h', '1.2.3.4');
$this->assertFalse($res['ok']);
$this->assertSame(__('backend.invalid_input'), $res['output']);
}
public function test_unban_rejects_a_double_dash_option_jail(): void
{
$res = $this->service($this->fleetNeverRuns())->unban($this->server(), '--all', '1.2.3.4');
$this->assertFalse($res['ok']);
$this->assertSame(__('backend.invalid_input'), $res['output']);
}
public function test_ban_rejects_a_jail_beginning_with_a_dot(): void
{
$res = $this->service($this->fleetNeverRuns())->ban($this->server(), '.evil', '1.2.3.4');
$this->assertFalse($res['ok']);
$this->assertSame(__('backend.invalid_input'), $res['output']);
}
/**
* A real jail with an INTERNAL dash must still pass validation proves the tightened
* regex is not over-broad. It gets PAST validJail and reaches support(), which we stub
* to report "unsupported"; the distinguishing signal is that the output is the support
* reason, NOT invalid_input (which would mean validJail wrongly rejected it).
*/
public function test_valid_jail_with_internal_dash_passes_validation(): void
{
// A real profile where fail2ban is unsupported (apk/openrc), so support() returns a reason.
$profile = new OsProfile('alpine', 'alpine', 'Alpine Linux', 'apk', 'none', 'openrc', []);
$reason = $profile->supports('fail2ban');
$this->assertNotNull($reason); // sanity: this profile really reports unsupported
$detector = Mockery::mock(OsDetector::class);
$detector->shouldReceive('detect')->andReturn($profile);
$res = $this->service($this->fleetNeverRuns(), $detector)
->unban($this->server(), 'nginx-http-auth', '1.2.3.4');
$this->assertFalse($res['ok']);
$this->assertSame($reason, $res['output']); // reached support(), not the validJail reject
}
}

View File

@ -238,6 +238,95 @@ class RbacOperateGateTest extends TestCase
->assertOk();
}
// ── operate: file CONTENT reads (download + editor view) ────────────────────
// Browsing the LISTING (names/perms/size) stays open to a viewer — see the browse
// test below. But pulling a file's BYTES (SFTP get) discloses secrets the server's
// credential can read (/etc/shadow, keys, .env) when the credential is root, which is
// beyond "read-only". So the content reads — Files::download, Files::edit (opens the
// editor) and FileEditor::load (the actual read) — require operate, like the writes.
public function test_viewer_cannot_download_a_file(): void
{
$this->actingAs($this->viewer());
$this->activeServer();
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('files')->andReturn([]);
$fleet->shouldReceive('getFile')->never(); // guard fires before the SFTP get
app()->instance(FleetService::class, $fleet);
Livewire::test(Files::class)
->set('entries', [['name' => 'shadow', 'type' => 'file', 'size' => 1, 'perms' => '-rw-------', 'owner' => 'root', 'modified' => 'now']])
->call('download', 0)
->assertForbidden();
}
public function test_operator_can_download_a_file(): void
{
$this->actingAs($this->operator());
$this->activeServer();
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('files')->andReturn([]);
$fleet->shouldReceive('getFile')->once()
->with(Mockery::type(Server::class), '/shadow')
->andReturn('root:x:...');
app()->instance(FleetService::class, $fleet);
Livewire::test(Files::class)
->set('entries', [['name' => 'shadow', 'type' => 'file', 'size' => 1, 'perms' => '-rw-------', 'owner' => 'root', 'modified' => 'now']])
->call('download', 0)
->assertOk(); // allowed through the guard, SFTP mocked away
}
public function test_viewer_cannot_open_the_file_editor(): void
{
$this->actingAs($this->viewer());
$this->activeServer();
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('files')->andReturn([]);
app()->instance(FleetService::class, $fleet);
Livewire::test(Files::class)
->set('entries', [['name' => 'x.conf', 'type' => 'file', 'size' => 1, 'perms' => '-rw-r--r--', 'owner' => 'root', 'modified' => 'now']])
->call('edit', 0)
->assertForbidden();
}
public function test_viewer_cannot_read_file_content_in_the_editor(): void
{
$this->actingAs($this->viewer());
$server = Server::create(['name' => 's', 'ip' => '10.0.0.9', 'ssh_port' => 22, 'status' => 'online']);
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('readFile')->never(); // guard fires before the remote read
$this->app->instance(FleetService::class, $fleet);
$this->app->instance(Sftp::class, Mockery::mock(Sftp::class));
Livewire::test(FileEditor::class, ['serverId' => $server->id, 'path' => '/etc/x.conf', 'name' => 'x.conf'])
->call('load')
->assertForbidden();
}
public function test_operator_can_read_file_content_in_the_editor(): void
{
$this->actingAs($this->operator());
$server = Server::create(['name' => 's', 'ip' => '10.0.0.9', 'ssh_port' => 22, 'status' => 'online']);
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('readFile')->once()
->with(Mockery::type(Server::class), '/etc/x.conf')
->andReturn(['content' => 'edited = wert', 'binary' => false, 'tooBig' => false]);
$this->app->instance(FleetService::class, $fleet);
$this->app->instance(Sftp::class, Mockery::mock(Sftp::class));
Livewire::test(FileEditor::class, ['serverId' => $server->id, 'path' => '/etc/x.conf', 'name' => 'x.conf'])
->call('load')
->assertOk() // allowed through the guard, SFTP mocked away
->assertSet('content', 'edited = wert');
}
// ── read-only: a viewer keeps READ access (proves read-only, not locked-out) ─
public function test_viewer_can_load_the_services_list(): void

View File

@ -2,6 +2,7 @@
namespace Tests\Feature;
use App\Livewire\Files\Index as Files;
use App\Livewire\Servers\Index as Servers;
use App\Livewire\Services\Index as Services;
use App\Models\Server;
@ -116,6 +117,47 @@ class RbacUiHidingTest extends TestCase
->assertDontSee(__('services.restart')); // but the action buttons are gone
}
// ── operate: Files\Index content controls (download / edit / delete) ────────
// The listing (browse) is open to a viewer; the content-read + write controls are not.
/** Stubs the SFTP directory read so the browser renders with one file row. */
private function stubFiles(): void
{
$fleet = Mockery::mock(FleetService::class);
$fleet->shouldReceive('files')->andReturn([
['name' => 'secrets.env', 'type' => 'file', 'size' => 12, 'perms' => '-rw-------', 'owner' => 'root', 'modified' => 'now'],
]);
app()->instance(FleetService::class, $fleet);
}
public function test_operator_sees_the_file_content_controls(): void
{
$this->actingAs($this->operator());
$this->activeServer();
$this->stubFiles();
Livewire::test(Files::class)
->call('load')
->assertOk()
->assertSee(__('files.download')) // download button present
->assertSee(__('common.edit')); // edit trigger present
}
public function test_viewer_does_not_see_the_file_content_controls(): void
{
$this->actingAs($this->viewer());
$this->activeServer();
$this->stubFiles();
Livewire::test(Files::class)
->call('load')
->assertOk()
->assertSee('secrets.env') // the listing is visible (browse stays open)
->assertDontSee(__('files.download')) // but no download button
->assertDontSee(__('common.edit')) // no edit trigger
->assertDontSee(__('common.delete')); // no delete trigger
}
// ── manage-network: sidebar WireGuard nav item ──────────────────────────────
public function test_admin_sees_the_wireguard_nav(): void

View File

@ -64,10 +64,24 @@ if [ "${CLUSEV_UPDATE_REEXEC:-0}" != 1 ]; then
# 1. let root operate on the clusev-owned working tree
git config --global --add safe.directory "$REPO_DIR" 2>/dev/null || true
# 2. fast-forward pull only — never auto-merge or throw away local edits
# 2. fast-forward pull only — never auto-merge or throw away local edits.
# PIN the pull to the branch recorded at install time (install.sh writes CLUSEV_BUILD_BRANCH
# into .env from the then-current HEAD) instead of implicitly following whatever branch is
# checked out now — otherwise anyone able to `git checkout` in this tree (the unprivileged
# clusev user) could silently redirect the next ROOT update to an arbitrary branch/fork.
# Refuse when the checkout diverges from it; fall back to a plain pull only when no branch
# was recorded (an install predating this field).
pin_branch="$(grep -m1 '^CLUSEV_BUILD_BRANCH=' .env 2>/dev/null | cut -d= -f2-)"
pull_args=(--ff-only)
if [ -n "$pin_branch" ]; then
current_branch="$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo '')"
[ "$current_branch" = "$pin_branch" ] \
|| die "Ausgecheckter Branch '${current_branch}' weicht vom Installations-Branch '${pin_branch}' ab — Update abgebrochen. Mit 'git checkout ${pin_branch}' zuruecksetzen und erneut versuchen."
pull_args=(--ff-only origin "$pin_branch")
fi
before_hash="$(sha256sum -- "$0" | cut -d' ' -f1)"
info "Hole Aktualisierungen (git pull --ff-only) ..."
timeout 300 git pull --ff-only \
timeout 300 git pull "${pull_args[@]}" \
|| die "git pull fehlgeschlagen — Timeout, fehlende Zugangsdaten zum privaten Repo, lokale Aenderungen oder divergierter Branch. Mit 'git status' und dem Repo-Zugriff (Token im origin-Remote) pruefen und erneut versuchen."
after_hash="$(sha256sum -- "$0" | cut -d' ' -f1)"