Skip to content

Commit d69505f

Browse files
committed
fix: Refuse to publish artifacts that cannot be downloaded
Publish copied the staged objects to their published keys and committed. The copy succeeding only says the storage backend accepted the write; whether those bytes are reachable at the URL the server then advertises was never established, because uploads go through IStorageService and URLs come from Firmware:CdnBaseUrl and nothing tied the two together. An instance where those refer to different places - a Local backend on a deployment advertising a CDN, a bucket that CDN is not fronting - uploads every artifact successfully and publishes a version whose every download is a 404. Nothing about it looks wrong through the API: the version lists, the hashes are correct, the sizes are right. The client that finds out is a hub partway through an OTA update. That is what just happened on the dev instance: 65 artifacts uploaded, every hash verified server-side, published, and every one of them served 404. Publish now probes each artifact at the URL it is about to advertise, after promoting and before committing, and rolls the promotion back if any is unreachable - so a release that cannot be downloaded does not become a release. Probes run concurrently and retry briefly, to absorb edge propagation without waiting out a misconfiguration that will never resolve. A store that refuses HEAD answers 405, which counts as present: the object is evidently there, the method is not supported. VERIFYPUBLISHEDARTIFACTS turns it off for deployments that genuinely cannot reach their own CDN hostname. That does not make such a release good, it only stops the server being able to tell.
1 parent 41d309f commit d69505f

6 files changed

Lines changed: 205 additions & 1 deletion

File tree

‎README.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ through `appsettings.Custom.json`, user secrets or command line arguments.
5555
| `OPENSHOCK__FIRMWARE__STORAGE__TYPE` | x | `Local` | `Local`, `S3`, `BunnyCdn` |
5656
| `OPENSHOCK__FIRMWARE__STAGEDRELEASETTL` | | `01:00:00` | How long a release may sit in `staging` before it is aborted |
5757
| `OPENSHOCK__FIRMWARE__EDITINGRELEASETTL` | | `7.00:00:00` | How long a release may sit in `editing` before it is aborted |
58+
| `OPENSHOCK__FIRMWARE__VERIFYPUBLISHEDARTIFACTS` | | `true` | Check each artifact is retrievable at `CDNBASEURL` before publishing, and refuse to publish if not |
5859
| `OPENSHOCK__REPO__ID` | x | | `openshock-desktop-modules` |
5960
| `OPENSHOCK__REPO__NAME` | x | | `OpenShock Modules` |
6061
| `OPENSHOCK__REPO__AUTHOR` | x | | `OpenShock` |
@@ -233,6 +234,16 @@ POST /2/firmware/releases/{id}/publish promote to live
233234
DELETE /2/firmware/releases/{id} abort and clean up
234235
```
235236
237+
Publish promotes the staged objects to their published keys, then checks each one is actually
238+
retrievable at the URL it is about to advertise, and only commits if they all are. Uploading to
239+
storage and serving from `CDNBASEURL` are independent facts: an instance writing to a `Local`
240+
backend while advertising a CDN, or to a bucket that CDN is not fronting, uploads every artifact
241+
successfully and would otherwise publish a version whose every download is a 404. Nothing about
242+
that release looks wrong through the API — the hashes and sizes are right — so the client that
243+
finds out is a hub partway through an update. A failed check rolls the promotion back and returns
244+
`Firmware.ArtifactsNotRetrievable`. Set `VERIFYPUBLISHEDARTIFACTS=false` only where the server
245+
genuinely cannot reach its own CDN hostname.
246+
236247
Uploads carry a `sha256` form field mapping each artifact type to its expected hash, and the whole
237248
request is verified before anything is written. If the changelog fails to parse, init returns 400 and
238249
the build fails. With `?nofail` the release is created in `editing` status instead and cannot be

‎RepositoryServer/Config/FirmwareConfig.cs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,18 @@ public sealed class FirmwareConfig
1616
/// How long a release may remain in <c>editing</c> status before the cleanup job aborts it.
1717
/// </summary>
1818
public TimeSpan EditingReleaseTtl { get; init; } = TimeSpan.FromDays(7);
19+
20+
/// <summary>
21+
/// Whether publish checks that each artifact is retrievable at the URL it will advertise,
22+
/// refusing to publish when it is not.
23+
/// </summary>
24+
/// <remarks>
25+
/// On by default: a release whose artifacts 404 is indistinguishable from a healthy one through
26+
/// the API, and the client that finds out is a hub mid-update.
27+
///
28+
/// Turn it off only where the server genuinely cannot reach its own CDN hostname — split-horizon
29+
/// DNS, egress restrictions. Doing so does not make the release good, it only stops the server
30+
/// from being able to tell.
31+
/// </remarks>
32+
public bool VerifyPublishedArtifacts { get; init; } = true;
1933
}

‎RepositoryServer/Controllers/V2/Firmware/ReleasesController.cs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public class ReleasesController : OpenShockControllerBase
4646
private readonly ApiConfig _apiConfig;
4747
private readonly IDiscordNotificationService _discord;
4848
private readonly TimeProvider _timeProvider;
49+
private readonly PublishedArtifactVerifier _artifactVerifier;
4950
private readonly ILogger<ReleasesController> _logger;
5051

5152
public ReleasesController(
@@ -54,13 +55,15 @@ public ReleasesController(
5455
ApiConfig apiConfig,
5556
IDiscordNotificationService discord,
5657
TimeProvider timeProvider,
58+
PublishedArtifactVerifier artifactVerifier,
5759
ILogger<ReleasesController> logger)
5860
{
5961
_db = db;
6062
_storage = storage;
6163
_apiConfig = apiConfig;
6264
_discord = discord;
6365
_timeProvider = timeProvider;
66+
_artifactVerifier = artifactVerifier;
6467
_logger = logger;
6568
}
6669

@@ -519,6 +522,38 @@ public async Task<IActionResult> PublishRelease([FromRoute] Guid releaseId, Canc
519522
throw;
520523
}
521524

525+
// The copy above succeeded, which only says the storage backend accepted it. Whether those
526+
// bytes are reachable at the URL this server is about to advertise is a separate fact, and
527+
// one nothing here has ever established: uploads go through IStorageService, URLs come from
528+
// Firmware:CdnBaseUrl, and an instance where those two do not refer to the same place
529+
// publishes a version whose every artifact 404s. That release looks entirely healthy through
530+
// the API - correct hashes, correct sizes - and the client that discovers otherwise is a hub
531+
// partway through an OTA update.
532+
//
533+
// Checked before the transaction commits, so a release that cannot be downloaded does not
534+
// become a release. Rolls the promotion back on failure, exactly as a copy error does.
535+
var unreachable = await _artifactVerifier.FindUnreachableAsync(
536+
release.Version,
537+
release.StagedArtifacts.Select(a => (a.BoardId, a.ArtifactType)),
538+
ct);
539+
540+
if (unreachable.Count > 0)
541+
{
542+
await TryDeleteAllAsync(promoted, ct);
543+
544+
var detail = string.Join("; ", unreachable.Take(5).Select(u => $"{u.Url} -> {u.Detail}"));
545+
if (unreachable.Count > 5)
546+
{
547+
detail += $" (and {unreachable.Count - 5} more)";
548+
}
549+
550+
_logger.LogError(
551+
"Refusing to publish {Version}: {Count} artifact(s) not retrievable at the configured CDN base URL",
552+
release.Version, unreachable.Count);
553+
554+
return Problem(FirmwareError.FirmwareArtifactsNotRetrievable(detail));
555+
}
556+
522557
await using var transaction = await _db.Database.BeginTransactionAsync(ct);
523558

524559
var versionEntity = new FirmwareVersion

‎RepositoryServer/Errors/FirmwareError.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,15 @@ public static OpenShockProblem FirmwareSha256Mismatch(string detail) =>
3535
new("Firmware.Sha256Mismatch", "Uploaded artifact hash does not match the provided SHA-256 manifest", HttpStatusCode.BadRequest, detail);
3636
public static OpenShockProblem FirmwareManifestKeysMismatch(string detail) =>
3737
new("Firmware.ManifestKeysMismatch", "The SHA-256 manifest keys do not match the uploaded files", HttpStatusCode.BadRequest, detail);
38+
/// <summary>
39+
/// The artifacts were promoted but are not retrievable at the URLs this server advertises, so
40+
/// the release was rolled back instead of published.
41+
/// </summary>
42+
public static OpenShockProblem FirmwareArtifactsNotRetrievable(string detail) =>
43+
new("Firmware.ArtifactsNotRetrievable",
44+
"Published artifacts are not retrievable at the configured CDN base URL",
45+
HttpStatusCode.InternalServerError, detail);
46+
3847
public static OpenShockProblem FirmwareReleaseNotesNotFinalized =>
3948
new("Firmware.ReleaseNotesNotFinalized", "Release notes must be finalized before publish", HttpStatusCode.Conflict);
4049

‎RepositoryServer/Program.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
using System.Net;
1+
using System.Net;
22
using Asp.Versioning;
33
using EntityFramework.Exceptions.PostgreSQL;
44
using Microsoft.AspNetCore.Authentication;
@@ -306,6 +306,13 @@
306306
// invariants as the equivalent clicking.
307307
builder.Services.AddScoped<CatalogImportService>();
308308

309+
// Typed client: publish probes the artifact URLs it is about to advertise, and a hostname that
310+
// black-holes must fail the publish quickly rather than hold the request open.
311+
builder.Services.AddHttpClient<PublishedArtifactVerifier>(client =>
312+
{
313+
client.Timeout = TimeSpan.FromSeconds(15);
314+
});
315+
309316
// <---- Background cleanup ---->
310317
builder.Services.AddHostedService<StagedReleaseCleanupService>();
311318

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
using OpenShock.RepositoryServer.Config;
2+
using OpenShock.RepositoryServer.Enums;
3+
using OpenShock.RepositoryServer.Utils;
4+
using System.Net;
5+
6+
namespace OpenShock.RepositoryServer.Services;
7+
8+
/// <param name="Url">The URL that was probed, as it would be advertised to a hub.</param>
9+
/// <param name="Status">HTTP status observed, or null if the request never got one.</param>
10+
/// <param name="Detail">Why it failed, for the problem response and the log.</param>
11+
public sealed record UnreachableArtifact(string Url, HttpStatusCode? Status, string Detail);
12+
13+
/// <summary>
14+
/// Checks that published artifacts are actually retrievable at the URLs the server hands out.
15+
/// </summary>
16+
/// <remarks>
17+
/// Writing to storage and advertising a URL are two independent facts here: the server uploads
18+
/// through <see cref="IStorageService"/> and builds URLs from <c>Firmware:CdnBaseUrl</c>, and
19+
/// nothing has ever tied the two together. An instance whose storage is not the storage behind that
20+
/// hostname — a Local backend on a deployment that advertises a CDN, or a bucket the CDN is not
21+
/// fronting — uploads every artifact successfully, publishes successfully, and serves a version
22+
/// whose every download is a 404.
23+
///
24+
/// That state is invisible from the API. The version lists, the artifact hashes are correct, the
25+
/// sizes are right; only a client actually fetching the bytes finds out, and the clients here are
26+
/// hubs applying an OTA update. So the check belongs at publish, where it can still refuse.
27+
/// </remarks>
28+
public sealed class PublishedArtifactVerifier
29+
{
30+
private readonly HttpClient _http;
31+
private readonly ApiConfig _config;
32+
private readonly ILogger<PublishedArtifactVerifier> _logger;
33+
34+
public PublishedArtifactVerifier(
35+
HttpClient http, ApiConfig config, ILogger<PublishedArtifactVerifier> logger)
36+
{
37+
_http = http;
38+
_config = config;
39+
_logger = logger;
40+
}
41+
42+
/// <summary>
43+
/// Probes every artifact of a release. Returns the ones that could not be retrieved; an empty
44+
/// list means every artifact answered 200.
45+
/// </summary>
46+
/// <remarks>
47+
/// Probes run concurrently — a release is up to five artifacts per board across a dozen boards,
48+
/// and doing that serially would add real latency to every publish.
49+
///
50+
/// A freshly written object can take a moment to become visible at the edge, so a miss is
51+
/// retried a few times before it counts. The retry is deliberately short: it exists to absorb
52+
/// propagation, not to wait out a misconfiguration, which will never resolve no matter how long
53+
/// the publish blocks.
54+
/// </remarks>
55+
public async Task<IReadOnlyList<UnreachableArtifact>> FindUnreachableAsync(
56+
string version,
57+
IEnumerable<(Guid BoardId, FirmwareArtifactType Type)> artifacts,
58+
CancellationToken ct = default)
59+
{
60+
if (!_config.Firmware.VerifyPublishedArtifacts)
61+
{
62+
return [];
63+
}
64+
65+
var cdnBase = _config.Firmware.CdnBaseUrl.TrimEnd('/');
66+
67+
var probes = artifacts
68+
.Select(a => FirmwareArtifactFileNames.BuildUrl(cdnBase, version, a.BoardId, a.Type))
69+
.Distinct()
70+
.Select(url => ProbeAsync(url, ct));
71+
72+
var results = await Task.WhenAll(probes);
73+
74+
return results.Where(r => r is not null).Select(r => r!).ToList();
75+
}
76+
77+
private async Task<UnreachableArtifact?> ProbeAsync(string url, CancellationToken ct)
78+
{
79+
const int attempts = 3;
80+
UnreachableArtifact? last = null;
81+
82+
for (var attempt = 1; attempt <= attempts; attempt++)
83+
{
84+
if (attempt > 1)
85+
{
86+
await Task.Delay(TimeSpan.FromSeconds(2 * (attempt - 1)), ct);
87+
}
88+
89+
try
90+
{
91+
// HEAD, because the body is not wanted and some of these are megabytes. A store that
92+
// refuses HEAD reports 405, which is treated as reachable below rather than as a
93+
// failed publish - the object is evidently there, the method is not supported.
94+
using var request = new HttpRequestMessage(HttpMethod.Head, url);
95+
using var response = await _http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, ct);
96+
97+
if (response.IsSuccessStatusCode || response.StatusCode == HttpStatusCode.MethodNotAllowed)
98+
{
99+
return null;
100+
}
101+
102+
last = new UnreachableArtifact(url, response.StatusCode, DescribeStatus(response.StatusCode));
103+
}
104+
catch (HttpRequestException ex)
105+
{
106+
last = new UnreachableArtifact(url, null, $"request failed: {ex.Message}");
107+
}
108+
catch (TaskCanceledException) when (!ct.IsCancellationRequested)
109+
{
110+
last = new UnreachableArtifact(url, null, "timed out");
111+
}
112+
}
113+
114+
_logger.LogError(
115+
"Published artifact is not retrievable at {Url}: {Detail}", last!.Url, last.Detail);
116+
117+
return last;
118+
}
119+
120+
private static string DescribeStatus(HttpStatusCode status) => status switch
121+
{
122+
HttpStatusCode.NotFound =>
123+
"not found - the storage this server writes to is not the storage behind Firmware:CdnBaseUrl",
124+
HttpStatusCode.Forbidden or HttpStatusCode.Unauthorized =>
125+
"access denied - the artifact exists but is not publicly readable",
126+
_ => $"unexpected status {(int)status}"
127+
};
128+
}

0 commit comments

Comments
 (0)