diff --git a/CLAUDE.md b/CLAUDE.md index d083ac7..17887cf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,7 +82,7 @@ sources (Tasmota/HA/MQTT/manual/CSV) **Time & DST (SDD §10):** store UTC everywhere; bucket and display in the instance timezone (default `Europe/Berlin`). "Daily cost" boundaries are local-midnight — use `time_bucket(..., 'Europe/Berlin')`. -**In-app update (`UpdateRunner`):** the dashboard shows a banner when a newer tag exists (`UpdateCheckService`, cached, never blocks a render). Triggering an update is **off by default** and needs three independent conditions: `MeterVault__AllowInAppUpdate`, at least one configured API key, and a caller presenting one (constant-time compare). `AllowAnonymousApi` deliberately cannot reach it — the updater builds whatever is on the branch and the LXC runs the app as root, so this is root-equivalent. Launches detached via `systemd-run` because the update restarts the service. Treat any change here as security-critical; `UpdateRunnerTests` pins the gate. +**In-app update (`UpdateRunner`):** the dashboard shows a banner when a newer tag exists (`UpdateCheckService`, cached, never blocks a render). Triggering an update is **off by default**; `MeterVault__AllowInAppUpdate` is the *only* gate — no API key, by explicit owner decision. With it on, anything that can reach the app can trigger a rebuild+restart as root (realistically a DoS, since the build comes from the owner's own repo; RCE if that repo is compromised). The REST endpoint additionally requires an `X-MeterVault-Update` header — a CSRF guard, not auth, so a foreign page cannot drive it via a LAN browser. Launches detached via `systemd-run` because the update restarts the service. Treat any change here as security-critical; `UpdateRunnerTests` pins that the flag defaults off and that API keys alone don't enable it. **Secrets (SDD §6.4):** broker/HA tokens are **never** stored in DB plaintext. Two forms, chosen per connector in the admin UI: a *reference* (`token_env`/`password_env` naming an env var or Docker secret path) resolved at runtime, or *encrypted at rest* (`token_enc`/`password_enc`) via `SecretProtector` over the ASP.NET Core data-protection key ring. Exactly one survives a save; `EndpointSecret.Resolve` is the single resolution path (encrypted wins). The key ring lives outside the app directory (`MeterVault__DataProtectionKeyPath`, default `/var/lib/metervault/keys`) because the LXC updater republishes `/opt/metervault`. `ExportService` drops `*_enc` values — they are bound to the originating key ring. diff --git a/README.md b/README.md index 9726db0..2f91b2e 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ Configuration is via environment variables (`Section__Key` double-underscore map | `MeterVault__SeedReferenceData` | `true` to load the bundled demo dataset on first start (idempotent) | | `MeterVault__DataProtectionKeyPath` | Where the key ring for UI-entered connector secrets lives (default `/var/lib/metervault/keys`) | | `MeterVault__UpdateCheckEnabled` | `false` to stop the dashboard checking for a newer release | -| `MeterVault__AllowInAppUpdate` | `true` to allow updates triggered from the UI/API — grants root-equivalent access to anyone with an API key; see below | +| `MeterVault__AllowInAppUpdate` | `true` to allow updates triggered from the UI/API — no key required, so anything that can reach MeterVault can trigger one; see below | | `MeterVault__UpdateCheckUrl` | Tag listing consulted by that check (repoint at a fork; blank also disables it) | The REST API is **closed by default**: with no `ApiKeys` configured and `AllowAnonymousApi` off, it @@ -96,20 +96,25 @@ sent — cached for six hours, and it never blocks or fails a page render. Turn `POST /api/v1/system/update` endpoint for scripting it from Home Assistant or `curl`: ```bash -curl -X POST http://metervault:8760/api/v1/system/update -H "X-Api-Key: $METERVAULT_API_KEY" +curl -X POST http://metervault:8760/api/v1/system/update -H "X-MeterVault-Update: 1" ``` Both pull the latest source, rebuild, and restart the service — a few minutes during which MeterVault is unavailable. Readings are untouched; ingestion resumes on restart. LXC only: containers are replaced by pulling a new image, and the endpoint reports that rather than pretending. -> **Understand what this grants before enabling it.** The updater builds whatever is on the branch, -> and the LXC runs MeterVault as **root** — so a valid API key becomes arbitrary code execution on -> that host. Three things must all hold before anything runs: the opt-in above, at least one -> configured API key, and a caller presenting one. In particular `MeterVault__AllowAnonymousApi` can -> **never** reach it — opening reads must not open root — and the UI button asks for the key every -> time rather than remembering it, because the UI itself has no login. Leave this off unless the UI -> is behind an authenticating proxy or on a network you fully trust. +> **That flag is the whole gate — there is no key and no prompt.** With it on, anything that can +> reach MeterVault can trigger a rebuild and restart. Realistically that is a repeatable denial of +> service (minutes of downtime and a busy CPU per request), not code injection, because the build +> comes from your own repository — but it becomes remote code execution if that repository is ever +> compromised. It defaults off. Enable it only on a network you trust, or behind an authenticating +> proxy. +> +> The `X-MeterVault-Update` header is **not** authentication: it stops a *different website* driving +> the endpoint through the browser of someone on your network, which a plain HTML form could +> otherwise do. The UI button does not need it — it runs over the Blazor circuit, which a foreign +> page cannot reach. Every triggered update is logged as a warning, since with no key there is no +> caller to attribute it to. Secrets (broker/HA tokens) are **never** stored in the database as plaintext. Each connector picks one of two forms: the *name* of an environment variable, resolved at runtime, or the secret typed diff --git a/src/App/Api/ApiEndpoints.cs b/src/App/Api/ApiEndpoints.cs index 07c18f5..6bed5d0 100644 --- a/src/App/Api/ApiEndpoints.cs +++ b/src/App/Api/ApiEndpoints.cs @@ -25,6 +25,9 @@ public static class ApiEndpoints { private const int MaxReadingsPerRequest = 5000; + /// Header confirming an update request was made on purpose rather than by a foreign page. + public const string UpdateRequestHeader = "X-MeterVault-Update"; + public static IEndpointRouteBuilder MapMeterVaultApi(this IEndpointRouteBuilder app) { var api = app.MapGroup("/api/v1").AddEndpointFilter().WithTags("MeterVault"); @@ -59,9 +62,6 @@ public static class ApiEndpoints return Results.Ok(new IngestResult(written, updated, rejected, ignored)); }).WithSummary("Ingest one or more readings (idempotent). Lets Home Assistant push."); - // Not on the shared group filter: that one lets requests through anonymously when - // AllowAnonymousApi is set, and opening reads must never open a root shell. This checks the - // presented key itself, in constant time, on top of the group filter. api.MapPost("/system/update", async (HttpContext http, UpdateRunner runner, CancellationToken ct) => { if (runner.Availability is not UpdateAvailability.Allowed) @@ -71,22 +71,25 @@ public static class ApiEndpoints title: runner.Availability switch { UpdateAvailability.NotEnabled => "In-app update is disabled. Set MeterVault__AllowInAppUpdate=true.", - UpdateAvailability.NoApiKeyConfigured => "In-app update requires at least one configured API key.", _ => "This install has no in-place update mechanism (containers are replaced, not updated).", }); } - if (!runner.IsAuthorised(http.Request.Headers[ApiKeyFilter.HeaderName].ToString())) + // Not authentication — the operator opted out of that. This only stops a *different site* + // driving the endpoint through the browser of someone on this network: a plain HTML form + // cannot set a custom header, and a cross-origin fetch that tries is stopped by the + // preflight, which nothing here answers. Costs a deliberate caller one flag. + if (!http.Request.Headers.ContainsKey(UpdateRequestHeader)) { - return Results.Problem(statusCode: StatusCodes.Status401Unauthorized, - title: "A valid API key is required to start an update."); + return Results.Problem(statusCode: StatusCodes.Status400BadRequest, + title: $"Send the {UpdateRequestHeader} header to confirm this is a deliberate request."); } var launch = await runner.LaunchAsync(ct); return launch.Started ? Results.Accepted(value: new { message = launch.Message }) : Results.Problem(statusCode: StatusCodes.Status500InternalServerError, title: launch.Message); - }).WithSummary("Start an in-place update (LXC only; requires opt-in and a valid API key)."); + }).WithSummary($"Start an in-place update (LXC only; requires MeterVault__AllowInAppUpdate and the {UpdateRequestHeader} header)."); api.MapGet("/meters", async (MeterVaultDbContext db, CancellationToken ct) => Results.Ok(await db.Meters.AsNoTracking() diff --git a/src/App/Components/Shared/UpdateBanner.razor b/src/App/Components/Shared/UpdateBanner.razor index b37bee4..81cd78c 100644 --- a/src/App/Components/Shared/UpdateBanner.razor +++ b/src/App/Components/Shared/UpdateBanner.razor @@ -3,6 +3,10 @@ @inject UpdateRunner Runner @inject ISnackbar Snackbar +@* No key prompt: the operator opted out of that (MeterVault__AllowInAppUpdate is the whole gate). + The confirmation stays — not as a security control, but because a stray click costs several + minutes of downtime while the rebuild runs. *@ + @* Renders nothing at all unless a newer release genuinely exists — no "you are up to date" noise. *@ @if (_status is { UpdateAvailable: true, Running: { } running, Latest: { } latest }) { @@ -31,16 +35,10 @@ This pulls the latest source, rebuilds it, and restarts the service. It takes a few minutes, during which MeterVault is unavailable. Readings are not affected — ingestion resumes on restart. - @* The UI has no login, so the key is what actually authorises this — not the click. It is - typed each time rather than remembered, so a browser left open on this page is not a - standing permission to execute code on the host. *@ - Cancel - + @(_starting ? "Starting…" : "Update now") @@ -51,7 +49,6 @@ private string _command = ""; private bool _confirmOpen; private bool _starting; - private string? _apiKey; private readonly DialogOptions _dialogOptions = new() { MaxWidth = MaxWidth.ExtraSmall, FullWidth = true }; protected override void OnInitialized() @@ -82,14 +79,6 @@ _starting = true; try { - if (!Runner.IsAuthorised(_apiKey)) - { - // Same message either way: whether a key is wrong or the deployment has none is not - // something an unauthenticated caller should be able to distinguish. - Snackbar.Add("That API key was not accepted.", Severity.Error); - return; - } - var launch = await Runner.LaunchAsync(); Snackbar.Add(launch.Message, launch.Started ? Severity.Success : Severity.Error); if (launch.Started) @@ -99,8 +88,6 @@ } finally { - // Never keep the key in component state: this circuit outlives the dialog. - _apiKey = null; _starting = false; } } diff --git a/src/Infrastructure/Update/UpdateRunner.cs b/src/Infrastructure/Update/UpdateRunner.cs index 4beb4b4..82ac17e 100644 --- a/src/Infrastructure/Update/UpdateRunner.cs +++ b/src/Infrastructure/Update/UpdateRunner.cs @@ -1,6 +1,4 @@ using System.Diagnostics; -using System.Security.Cryptography; -using System.Text; using MeterVault.Infrastructure.Options; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -15,9 +13,6 @@ public enum UpdateAvailability /// The operator has not set MeterVault__AllowInAppUpdate. NotEnabled, - /// No API key is configured, so no request could ever be authorised to do this. - NoApiKeyConfigured, - /// This install has no update mechanism — a container is replaced, not updated in place. NotSupportedHere, } @@ -30,16 +25,16 @@ public sealed record UpdateLaunch(bool Started, string Message); /// /// /// This is the most dangerous thing in the codebase, so the reasoning is written down. The updater -/// runs git reset --hard and dotnet publish against whatever is on the branch, and in -/// the LXC the app runs as root — so anything able to trigger it can execute arbitrary code as root. -/// The web UI has no authentication, so "reachable from the dashboard" alone would mean any device -/// on the network could take the host. +/// runs git reset --hard and dotnet publish against whatever is on the branch, then +/// restarts the service, and in the LXC the app runs as root. /// -/// Three independent conditions must therefore hold: the operator opted in explicitly, at least one -/// API key exists, and the caller presented one. The opt-in is not merely a convenience toggle — with -/// it off there is no code path to launch at all. Notably an anonymous-API deployment -/// () can never reach this: that flag opens reads, -/// and opening reads must not open root. +/// is the only gate, by explicit operator choice — +/// no key, no prompt. With it on, anything that can reach the UI can trigger a rebuild-and-restart: +/// on the realistic threat model that is a repeatable denial of service (minutes of downtime and a +/// pegged CPU per request) rather than code injection, since the build comes from the operator's own +/// repository — but it becomes full remote code execution if that repository is ever compromised. +/// With it off there is no code path to launch at all, which is why it defaults off and why the +/// check is repeated inside rather than trusted to callers. /// public sealed class UpdateRunner(IOptions options, ILogger logger) { @@ -62,39 +57,10 @@ public sealed class UpdateRunner(IOptions options, ILogger - /// Whether the presented key authorises an update. Compared in constant time: a naive comparison - /// leaks key material through response timing to a caller who can retry indefinitely. - /// - public bool IsAuthorised(string? providedKey) - { - if (string.IsNullOrEmpty(providedKey) || _options.ApiKeys.Count == 0) - { - return false; - } - - var provided = Encoding.UTF8.GetBytes(providedKey); - var matched = false; - foreach (var candidate in _options.ApiKeys) - { - // No early exit: check every key so the time taken does not reveal which one matched. - var expected = Encoding.UTF8.GetBytes(candidate); - matched |= CryptographicOperations.FixedTimeEquals(provided, expected); - } - - return matched; - } - /// /// Launches the updater detached from this process and returns immediately. /// @@ -145,9 +111,10 @@ public sealed class UpdateRunner(IOptions options, ILogger -/// The gate in front of the in-app update. Triggering it runs a build of whatever is on the branch, -/// as root on the LXC, so every one of these is a case where getting it wrong hands the host over. +/// The gate in front of the in-app update. AllowInAppUpdate is the whole gate by explicit +/// operator choice — no key — so the flag defaulting off is the only thing standing between an +/// upgrade and a network-triggerable rebuild-and-restart as root. /// /// /// None of these launch anything: they assert the conditions that must hold before a launch @@ -15,64 +16,24 @@ namespace MeterVault.Integration.Tests; /// public sealed class UpdateRunnerTests { - private const string Key = "correct-horse-battery-staple"; - [Fact] public void Disabled_by_default() { - // The single most important assertion here: an operator who has not thought about this does - // not get a remote-code-execution endpoint by upgrading. + // The single most important assertion in this file: with no key required, an operator who + // upgrades without reading the notes must not silently acquire an open trigger. Assert.False(new MeterVaultOptions().AllowInAppUpdate); Assert.Equal(UpdateAvailability.NotEnabled, NewRunner(new MeterVaultOptions()).Availability); } [Fact] - public void Refuses_when_enabled_but_no_api_key_is_configured() + public void Configuring_api_keys_alone_does_not_enable_it() { - // Otherwise "enabled" would mean anyone at all, since there would be no key to present. - var runner = NewRunner(new MeterVaultOptions { AllowInAppUpdate = true }); + // The two settings are independent: having an API key for the REST API is not consent to + // rebuild the host. + var options = new MeterVaultOptions(); + options.ApiKeys.Add("some-key"); - Assert.Equal(UpdateAvailability.NoApiKeyConfigured, runner.Availability); - } - - [Fact] - public void An_anonymous_api_deployment_cannot_reach_it() - { - // AllowAnonymousApi opens reads. It must not open root: with no keys configured this stays - // shut regardless of that flag. - var runner = NewRunner(new MeterVaultOptions - { - AllowInAppUpdate = true, - AllowAnonymousApi = true, - }); - - Assert.Equal(UpdateAvailability.NoApiKeyConfigured, runner.Availability); - Assert.False(runner.IsAuthorised(null)); - Assert.False(runner.IsAuthorised("")); - Assert.False(runner.IsAuthorised("anything")); - } - - [Theory] - [InlineData(null)] - [InlineData("")] - [InlineData("wrong")] - [InlineData("correct-horse-battery-stapl")] // prefix of a valid key - [InlineData("correct-horse-battery-staple ")] // trailing space - [InlineData("CORRECT-HORSE-BATTERY-STAPLE")] // case differs - public void Rejects_anything_that_is_not_exactly_a_configured_key(string? provided) - { - Assert.False(NewRunner(Enabled()).IsAuthorised(provided)); - } - - [Fact] - public void Accepts_an_exact_key_and_any_of_several() - { - var options = Enabled(); - options.ApiKeys.Add("second-key"); - var runner = NewRunner(options); - - Assert.True(runner.IsAuthorised(Key)); - Assert.True(runner.IsAuthorised("second-key")); + Assert.Equal(UpdateAvailability.NotEnabled, NewRunner(options).Availability); } [Fact] @@ -88,8 +49,8 @@ public sealed class UpdateRunnerTests [Fact] public void Reports_unsupported_where_there_is_no_in_place_updater() { - // On a dev box or in a container there is no /usr/bin/update, so a fully configured runner - // still declines rather than half-running something. + // On a dev box or in a container there is no /usr/bin/update, so an enabled runner still + // declines rather than half-running something. if (UpdateRunner.IsSupportedHere) { return; // running on a provisioned LXC; the negative case cannot be observed here @@ -98,12 +59,7 @@ public sealed class UpdateRunnerTests Assert.Equal(UpdateAvailability.NotSupportedHere, NewRunner(Enabled()).Availability); } - private static MeterVaultOptions Enabled() - { - var options = new MeterVaultOptions { AllowInAppUpdate = true }; - options.ApiKeys.Add(Key); - return options; - } + private static MeterVaultOptions Enabled() => new() { AllowInAppUpdate = true }; private static UpdateRunner NewRunner(MeterVaultOptions options) => new(Microsoft.Extensions.Options.Options.Create(options), NullLogger.Instance);