Update: drop the API-key requirement from the update trigger
ci / build-test (push) Successful in 1m14s
ci / build-test (push) Successful in 1m14s
Owner's call: MeterVault__AllowInAppUpdate is now the whole gate. One click on the banner, no key, no prompt, and the REST endpoint no longer asks for one either. What that means, recorded so it is not rediscovered later: with the flag on, anything that can reach MeterVault 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, because the build comes from the owner's own repository. It becomes remote code execution if that repository is ever compromised. The flag still defaults off, and that default is now the only thing between an upgrade and an open trigger, so UpdateRunnerTests pins it along with the fact that configuring API keys does not imply consent to rebuild the host. Kept one guard, which is not authentication: the REST endpoint requires an X-MeterVault-Update header. Without it any website could POST to the endpoint through the browser of someone on the network — a plain HTML form is enough, and no key means nothing else would stop it. A form cannot set a custom header and a cross-origin fetch that tries is stopped by a preflight nothing here answers, so this costs a deliberate caller one flag and costs the button nothing, since it runs over the Blazor circuit rather than HTTP. The confirmation dialog stays, now purely as a guard against a stray click costing several minutes of downtime. Every triggered update is logged as a warning: with no key there is no caller to attribute it to, and the restart discards anything held in memory. Claude-Session: https://claude.ai/code/session_01V6joyergfvVLFEizH1hJLd
This commit is contained in:
@@ -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
|
||||
/// <summary>The operator has not set <c>MeterVault__AllowInAppUpdate</c>.</summary>
|
||||
NotEnabled,
|
||||
|
||||
/// <summary>No API key is configured, so no request could ever be authorised to do this.</summary>
|
||||
NoApiKeyConfigured,
|
||||
|
||||
/// <summary>This install has no update mechanism — a container is replaced, not updated in place.</summary>
|
||||
NotSupportedHere,
|
||||
}
|
||||
@@ -30,16 +25,16 @@ public sealed record UpdateLaunch(bool Started, string Message);
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// This is the most dangerous thing in the codebase, so the reasoning is written down. The updater
|
||||
/// runs <c>git reset --hard</c> and <c>dotnet publish</c> 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 <c>git reset --hard</c> and <c>dotnet publish</c> 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
|
||||
/// (<see cref="MeterVaultOptions.AllowAnonymousApi"/>) can never reach this: that flag opens reads,
|
||||
/// and opening reads must not open root.
|
||||
/// <see cref="MeterVaultOptions.AllowInAppUpdate"/> 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 <see cref="LaunchAsync"/> rather than trusted to callers.
|
||||
/// </remarks>
|
||||
public sealed class UpdateRunner(IOptions<MeterVaultOptions> options, ILogger<UpdateRunner> logger)
|
||||
{
|
||||
@@ -62,39 +57,10 @@ public sealed class UpdateRunner(IOptions<MeterVaultOptions> options, ILogger<Up
|
||||
return UpdateAvailability.NotEnabled;
|
||||
}
|
||||
|
||||
// Without a key nothing can authenticate, and this must never fall back to open access.
|
||||
if (_options.ApiKeys.Count == 0)
|
||||
{
|
||||
return UpdateAvailability.NoApiKeyConfigured;
|
||||
}
|
||||
|
||||
return IsSupportedHere ? UpdateAvailability.Allowed : UpdateAvailability.NotSupportedHere;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Launches the updater detached from this process and returns immediately.
|
||||
/// </summary>
|
||||
@@ -145,9 +111,10 @@ public sealed class UpdateRunner(IOptions<MeterVaultOptions> options, ILogger<Up
|
||||
string.IsNullOrWhiteSpace(error) ? "Could not start the update." : error);
|
||||
}
|
||||
|
||||
// Deliberately loud, and the only record that this happened: the update restarts the app,
|
||||
// so nothing written after this survives in memory.
|
||||
_logger.LogWarning("In-app update authorised and started as transient unit {Unit}", TransientUnit);
|
||||
// Deliberately loud. With no key there is no caller to attribute this to, so the log is
|
||||
// the only record that it happened at all — and the update restarts the app, so nothing
|
||||
// held in memory survives.
|
||||
_logger.LogWarning("In-app update started as transient unit {Unit}", TransientUnit);
|
||||
return new UpdateLaunch(true,
|
||||
"Update started. The service restarts when the rebuild finishes — this usually takes a few minutes.");
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user