[kdbrooks]

Adds Azure Blob Storage as a third archive source alongside S3 and the local filesystem, and makes both cloud backends optional Cargo features.

Fully tested with real Azure Storage accounts, managed identities, service principals. Azurite is a good option for integration testing without needing Azure.

S3 ETag pinning prevents rati from serving up bad data and poisoning the cache when the blob changes under it. Now it fails loudly and asks for a restart.

Closes #3

AI Disclosure - AI was used to help develop this specifically Claude Opus 5 xhigh effort. The code was reviewed and tested throughly by me and this PR description was written without AI.

Testing

Fixture is a real Valhalla build of Indiana — 477 MB, 226 tiles plus index.bin, members spanning 792 B to 44.5 MB. The largest is well past the SDK's 4 MiB DEFAULT_DOWNLOAD_PARTITION_SIZE, so partitioned downloads get exercised and not just single-range reads.

Unit tests and lints across every feature combination, plus each commit independently:

Build	Tests	Clippy -Dwarnings
default (s3 + azure)	32	clean
--no-default-features	24	clean
--features s3	26	clean
--features azure	30	clean

cargo fmt clean, cargo metadata --locked passes. All three commits build, test and lint on their own, so the refactor can be verified as behaviour-preserving before the feature is read.

End to end, serving tiles over HTTP:

Source	Credential	Result
local tar	—	200 / 404 / 400 paths
Azurite	anonymous	226/226 byte-exact, 304, gzip negotiation
Azure Blob	Azure CLI	226/226 byte-exact, ETag, Last-Modified, 304, gzip
Azure Blob	service principal (secret)	tiles byte-exact
Azure Blob	managed identity, Container Apps	14/14 byte-exact over the public internet

The storage account ran with allowSharedKeyAccess = false, so every one of those authenticated through Entra — there was no key path to fall back on.

Two behaviours worth calling out because they're hard to test by inspection:

• ETag pinning. Replacing the blob under a running server took a tile read from 200 to 500, with 412 ConditionNotMet in the log. Without the pin that read returns 200 and wrong bytes, cached as immutable.
• SAS redaction. Serving through a real SAS URL: 226 tiles served, sig= present in no log line.

Not verified: Workload Identity (needs an AKS cluster with the webhook), Azure Pipelines (needs an ADO service connection), and client certificate (builds under its feature, but no certificate-backed service principal was exercised). Credential selection for all three is unit-tested; the token exchange isn't.

[kinkard]

Went through this against a local build. The commit split is clean — all three build, test and lint on their own, I checked — and every number in the description is accurate: 37 / 23 / 24 / 36 / 37 tests, clippy clean with -Dwarnings across all five combos, fmt clean. The feature gating really does deliver, 255 crates down to 89 with no dead code under any combo. ETag pinning is implemented correctly and the comment explaining the cache-poisoning failure mode is better than most production code manages.

I also traced the Azure SDK error path, since it looked like the likely SAS leak: it's clean. typespec_client_core wraps reqwest errors in CustomMessage, whose Display drops the inner error — the one that would have appended for url (...&sig=...). Fragile, but it holds.

Two things to fix before merge: the unredacted {source} in storage/mod.rs, and the // fix 3: leftover in azure.rs.

The rest is about size. rati is meant to stay small and readable by people, and this adds ~500 non-test lines for one backend with ~300 of those in credential selection, plus ~104 README lines that are mostly Azure operator documentation. Comments inline.

---

AI Disclosure - AI was used to help with reviewing this PR ;)

[kinkard]

Leftover from a development task list — "fix 3" doesn't refer to anything in the repo. The same comment in s3.rs:85 is written without the prefix.

[kdbrooks]

Gone, these comments were duplicates. This guard is now hoisted up to Storage::read_range.

[kinkard]

No if_match here, while Azure pins every ranged read to the ETag (azure.rs:474). The comment there explains why: replace the archive under a running server and unpinned reads mix bytes from two versions, served back with Cache-Control: immutable.

That argument applies to S3 unchanged. aws-sdk-s3 has .if_match(etag) on GetObject; it needs the ETag carried in the Storage::S3 variant the way AzureBlob carries it.

Deliberate, or just didn't get carried over?

[kdbrooks]

This is deliberate, I didn't want to change the behavior of S3. I do think this change is worth doing for S3 as well. Let me know if you want that in this PR or a follow up.

The cache poisoning is real and I was able to reproduce it with Azurite.

[kinkard]

This PR, please — as its own commit before the azure one, the way you did the short-read fix.

Reasoning: it's a cache-poisoning bug that exists on main today, independent of Azure, and shipping two backends with different safety properties is worse than shipping neither pinned. Someone reading storage.rs six months from now shouldn't have to work out why one backend fails loudly on a republished archive and the other serves stale offsets under Cache-Control: immutable.

It's about five lines — .if_match(etag) on the get_object builder, plus carrying the etag in Storage::S3 the way AzureBlob already does.

Good that you reproduced it on Azurite. Worth a line in the commit message saying what the failure looks like, since it's the sort of thing that gets "simplified" away later.

[kdbrooks]

S3 is done in this PR as a separate commit. Both Azure and S3 commits have comments on the failure mode and reproduction.

[kinkard]

The OpenSSL claim checks out — openssl v0.10.81 ← azure_identity v1.0.0. But that reads as an argument for not supporting certificate auth, rather than for a third feature to carry it.

Dropping it removes a feature, a CredentialKind arm, a detection row, a README section and a build combo. Is anyone asking for certificate auth?

[kdbrooks]

azure-client-certificate and CredentialKind arm is gone. openssl is out of Cargo.lock.

[kinkard]

This covers the smallest build, but not the two that actually ship — --features s3 and --features azure alone. You verified both by hand; nothing stops them rotting.

cargo clippy above is also missing --all-targets, so test code isn't linted.

[kdbrooks]

Both fixed, the matrix now covers all combinations.

[kinkard]

On the split itself: storage as a boundary is right. archive.rs was doing tar parsing and I/O both, and it's down from 1063 to 878.

Four files is where I'd push back. local.rs is 79 lines — one open, one read — and s3.rs is 117. Folding both into mod.rs gives a single ~370-line storage.rs beside azure.rs, and one less hop when following a read. The feature gate works inline just as well:

#[cfg(feature = "s3")]
mod s3 { ... }

azure.rs at 819 lines clearly earns its own file. Not a blocker — your call on which reads better.

[kinkard]

azure.rs at 819 lines clearly earns its own file.

Actually that's a good sign that something should be simplified or less vibe-coded... How is it possible that AWS requires 7 times less code?

[kdbrooks]

Refactored to match your structure, good suggestion! 370 matched the outcome exactly.

I cut this down another 143 lines. Most of the cut was credential selection.

On AWS vs. Azure, it comes down to a few details:

• The credentials / identity libraries. Before version 1.0 of the rust azure_identity crate there was a nice DefaultAzureCredential that aligned with the aws_config::load_defaults() call in the s3 code. That was deliberately removed in 1.0 so you have to explicitly pick the credential type.
• ETag pinning exists on the Azure side and not on S3
• The s3 protocol in the URL makes parsing the S3 URLs unambiguous

[kinkard]

Your DefaultAzureCredential explanation checks out — I looked at the crate rather than take it on trust. azure_identity 1.0.0 exports only concrete credentials (azure_cli, client_secret, managed_identity, workload_identity, …) and the string DefaultAzureCredential appears nowhere in its source. So the asymmetry with aws_config::load_defaults() is real and not something you invented.

Breaking the gap down, non-test lines:

Concern	S3	Azure
Recognising the URL	4	~66
Getting a credential	1	~148
Open + read bytes	~73	~125

• URL recognition — forced by Azure. s3:// is a scheme nothing else can claim; blob URLs are bare https://. But it's forced only for crediting, not for routing, which is the azure.rs:55 comment.
• Credentials — the SDK's fault that you have to choose, but rati's choice to expose the choice. That's the thread above.
• Open + read — I'd retract part of my round-1 complaint here. I'd called the ranged probe a wasted round trip versus get_properties(); now that the 512 bytes feed ArchiveSource::prefetch, Azure opens in one request where S3 needs HeadObject plus a read. That's better than the S3 path, not worse. The rest of the column is the ETag pinning S3 doesn't have.

So roughly 215 of the ~280-line gap is genuinely forced, and I can point at the cause for each. It's less vibe-coded than the raw number suggests. The discretionary part is the credential layer, and that's where I'd spend the cuts — not on the I/O, which is fine.

On the fold itself: 370 matching was coincidence and the file reads in the right order. See the note on storage.rs:166 about the inner mod blocks — that's the last bit of the split still standing.

[kdbrooks]

The credential layer was reworked and I cut out all the Azure command arguments and instead detect them from the env in a way consistent with other Azure SDKs. That cut the code down and clean up the README.md and cli.

[kdbrooks]

@kinkard Thanks for the thorough review! I took nearly every suggestion. Let me know if this needs more changes and we can keep iterating.

[kinkard]

Re-verified everything on f2b4d667. Tests 35 / 22 / 23 / 34 across the four combos, clippy --all-targets -Dwarnings clean on each, fmt clean, and all four commits build/test/lint independently — I rechecked that since the commit list was restructured. c349cb63 is properly self-contained at +12/-1 and lands before the azure commit. ETag pinning survived the refactor intact, the hoisted short-read guard is correct, and there's no dead code or stale docs from the deletions. 13 of 14 items landed.

Thanks for the rework — it's a real improvement and you've been precise about what you changed.

That said, I want to change the direction of one thing rather than keep trimming it, and be straight that my round-1 comment asked the wrong question.

I asked "which of these seven do you expect anyone to use," which invited a trim, and you trimmed to five. The question I should have asked is whether rati should be selecting Azure credentials at all. I don't think it should. CredentialKind is Azure's problem model showing up in rati's CLI — a tile server's --help shouldn't be teaching anyone the difference between workload identity and a service principal. Details in the azure.rs:86 thread, with a ~20-line sketch.

Same instinct behind the is_azure_url comment: rati will never have a generic HTTP backend, so any http(s) URL is Azure by elimination and none of the host-matching is needed for routing. That one deletes the branch the round-1 SAS leak lived in, makes Azurite work with no special case, and lets custom domains and Front Door work — which they can't today.

Between the two, the backend lands around 200 lines instead of 359, with no Azure vocabulary in rati's interface. The I/O half is already good and I'm not asking you to touch it — a one-request open that prefetches the first 512 bytes is tighter than what S3 does, and the pinning is a safety property S3 is missing.

One regression to fix regardless of direction: removing non_empty means an empty RATI_AZURE_USER_ASSIGNED_ID now reaches IMDS as client-id=&, which can silently resolve to the system-assigned identity. One-line fix on main.rs:66.

On the S3 question you asked — yes, in this PR. Reasoning in that thread.

[kinkard]

Removing non_empty went one step too far — this is a regression, not a simplification.

clap with env turns a present-but-empty variable into Some(""), not None. So RATI_AZURE_USER_ASSIGNED_ID="" — an unset Helm value or compose variable — reaches build_credential, which wraps it unconditionally:

let options = user_assigned.map(|id| ManagedIdentityCredentialOptions {
    user_assigned_id: Some(UserAssignedId::ClientId(id.to_string())),

and azure_identity formats it verbatim into client-id=&. Best case an opaque 400 from the token endpoint. On the App Service and Container Apps endpoints an empty id can resolve to the system-assigned identity — a different principal than the operator asked for, with nothing in the log saying so.

require() at azure.rs:184-193 already does exactly the right thing (.filter(|v| !v.is_empty())); this path just doesn't get it. No value_parser needed:

Suggested change

	user_assigned: self.azure_user_assigned_id.as_deref(),
	user_assigned: self.azure_user_assigned_id.as_deref().filter(|id| !id.is_empty()),

My fault for arguing the 8 lines weren't worth it — I was looking at the clap error message and not at what an empty id does to IMDS.

[kdbrooks]

This issue was removed when the Azure CLI arguments were removed.

[kinkard]

This arm disappears entirely under the is_azure_url change — see the comment on azure.rs:55.

Worth noting because it's the branch the round-1 SAS leak lived in. Deleting the branch deletes the bug class rather than keeping the fix around to be re-broken.

[kdbrooks]

Done, this arm is gone. is_azure_url is a simple scheme check.

[kinkard]

BlobClient::new's two error paths embed the raw URL — format!("{blob_url} is not a valid base URL") and format!("{blob_url} must use https") — and this lands in Error::Io, which main.rs .expect()s straight to stderr with the SAS in it.

Unreachable today only because select_credential_kind forces Anonymous on every non-https URL, so the must use https branch can't fire with Some(credential). That's the one spot where the redaction discipline depends on a third-party crate's internals instead of your own code.

Suggested change

	.map_err(|e| Error::Io(format!("creating blob client: {e}")))?;
	let client = BlobClient::new(parsed, credential, None)
	.map_err(|_| Error::Io(format!("creating blob client for {}", redact(url))))?;

[kdbrooks]

I added a comment explaining why the SDK error is dropped.

[kinkard]

redact() now runs on local filesystem paths too. An archive at /data/tiles?v2.tar reports stat(/data/tiles) failed: No such file or directory — pointing at a path that isn't the one you passed.

Cosmetic, but it turns a clear error into a confusing one. Either apply redact only to URL-shaped sources, or say in the doc comment at :44-49 that it truncates paths at ? as well.

[kdbrooks]

Fixed in redact(), it only truncates http:// and https:// sources now.

[kinkard]

Two things here don't match the code.

--azure-credential <kind> does not skip detection entirely — azure.rs:145 returns Anonymous before the override is consulted:

if insecure || !blob_host || has_sas_token(url) {
    return CredentialKind::Anonymous;
}
if let Some(kind) = override_kind { return kind; }

So --azure-credential client-secret against a SAS-bearing URL is silently discarded. Your own test asserts it (azure.rs:487-490). For the plaintext case that's correct and should stay; for the SAS case, throwing away an explicit operator choice without a warning is a footgun.

Row 1 is also missing a condition — the code checks !blob_host too, so an unrecognised host lands in row 1 as well.

Most of this evaporates if the credential layer collapses (see the azure.rs:86 thread).

[kdbrooks]

This issues are gone now that now Azure CLI args exist anymore.

[kinkard]

c349cb63 is a behaviour fix with a good commit message and no test. grep -rn "short read" src/ finds only the format string.

The guard itself is right — I checked all seven read_range call sites in archive.rs and none intentionally overshoots EOF (scan_tar_headers clamps with CHUNK_SIZE.min(archive_size - offset)), so no false positives, and using != rather than < catches an over-long response too. It just deserves a test pinning it.

[kdbrooks]

Test is there now.

[kinkard]

The fold landed at 370 lines, but local and s3 survive as nested inline mod blocks — so the four-file split became a four-module split in one file.

The cost is visible: six pub(super) markers (:177, :212, :243, :250, :300) that would be plain private fns, and two duplicated use blocks. The only thing the module buys is one #[cfg(feature = "s3")], which sits just as well on the individual items.

Dissolving both into plain functions is what I was asking for. Not a blocker.

[kdbrooks]

Dissolved. local and s3 are plain private functions now.

[kinkard]

Three lines still read as Azure documentation rather than rati documentation:

• this one — Storage Blob Data Reader is Azure RBAC, and the same fact is already in the error at azure.rs:275-276, which is where an operator actually hits it
• :70-72 — "a container image without the Azure CLI falls through to row 5 and fails looking for az" is troubleshooting prose for a failure the error message should carry
• :94 — the Content-Encoding note: the second clause (confuses CDNs in front of rati) earns its place, the first is Azure trivia

The cut so far is real, not reshuffled — this is the last 10%.

Structural note: Azure gets 44 lines and its own ###, S3 gets one table row and no prose. Worth being a decision rather than a drift.

[kdbrooks]

This is gone, now the docs are only how Rati handles environment settings and which credentials it selects. Let me know if you want an S3 section in the README.md to get parity with Azure.

[kdbrooks]

@kinkard - Thanks again! I reworked the credentials layer to completely remove all Azure command line arguments and instead detect them from the env in a way consistent with every other Azure SDK. I'd been wondering the same thing: whether to remove credential selection entirely. The README.md documents the order / priority of env vars. I took your suggestions with two exceptions:

• Removing the Service Principal assumes rati runs on Azure. A managed identity only works on Azure. MS's own wording is that a workload running "on-premises, on a developer's machine, or in another cloud … can't use an Azure managed identity directly", and workload identity federation covers an enumerated set of issuers. For those reasons, I kept Service Principal but the cost is minimal. Azure CLI was removed so the service principal is the only non-anonymous way to run locally now.

• The suggestion to use ManagedIdentityCredential::new(None) would break our production use case, a user assigned managed identity. You now specify the user-assigned managed identity implicitly by setting AZURE_CLIENT_ID without a full service-principal triple, as documented in README.md. To use the system-assigned managed identity, leave AZURE_CLIENT_ID unset.

is_azure_url is a scheme check now; has_blob_host_suffix survives only as the credential gate, per your comment. Dropped .blob.core.cloudapi.de (the SDK marks that cloud [Obsolete("closed on October 29th, 2021")]). Consequence worth naming: a CDN/custom-domain front end now reads anonymously or by SAS only.

The empty-id regression you flagged is fixed structurally: the flag is gone, and the env reader treats empty as unset, so an empty AZURE_CLIENT_ID can't reach IMDS as client-id=& — it deliberately selects the system-assigned identity, and the startup log says which one was chosen.

S3 ETag pinning added as its own commit before the Azure one. Tests were also added to verify both the pinning and short-read guards.

[kdbrooks]

We are using this in production and it's working well for us. Anything else to address here? I can get S3 up to parity with Azure Blob in README.md if we want to close that gap.

[kdbrooks]

@kinkard All the round 2 feedback is addressed. When you have time can you take another look and merge if it's ready? I can update the README to add S3 docs to get parity with Azure in this PR or a follow up if you want that.

[kinkard]

@kdbrooks thanks for your contribution! Unfortunately, I still have trust issues with LLMs and for the final round I need to find time and desire to sit down and review 1.5k lines of new code in that tiny project that was 1.5k lines of code in total (yeah, your PR basically doubles the size of this project). No promises here as I'm unfortunately quite heavily occupied at my daytime job

[kdbrooks]

@kdbrooks thanks for your contribution! Unfortunately, I still have trust issues with LLMs and for the final round I need to find time and desire to sit down and review 1.5k lines of new code in that tiny project that was 1.5k lines of code in total (yeah, your PR basically doubles the size of this project). No promises here as I'm unfortunately quite heavily occupied at my daytime job

I understand, some of it is refactoring existing code to be modular and optional. Let me know how I can help, if this is too big maybe I can split it up into smaller PRs.