Exempt GET /metrics from admin API auth - #30
Open
bartzon wants to merge 2 commits into
Open
Conversation
The admin listener serves both the admin API and Prometheus metrics. Annotation-based scraping cannot attach credentials, so guarding /metrics behind LIBSQL_ADMIN_AUTH_KEY would blind the fleet. Keep GET /metrics open; all other admin routes remain guarded.
The Extensions Tests job fresh-resolves rust_suite deps on every run; icu 2.3.0 (via idna_adapter 1.2) now requires rustc 1.88 while the repo pins 1.85.0, failing CI on any branch of this tag. Pinning idna_adapter to 1.1.0 (unicode-rs backend) drops icu from the graph.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of https://github.com/shop/issues-retail/issues/34482 (bugbounty 3990068 hardening, parent https://github.com/shop/issues-retail/issues/34436).
Why
We want to set
LIBSQL_ADMIN_AUTH_KEYin production (the admin port serves AdminShell — arbitrary SQL on any namespace — andPOST /v1/namespaces/:ns/configwithjwt_key, i.e. install-your-own-signing-key). Today the key is unset, soauth_middlewareinhttp/admin/mod.rsis a no-op.Blocker: the middleware is layered onto the merged admin router, which includes
GET /metrics, and our chart scrapes metrics on the admin port viaprometheus.io/*annotations that cannot attach credentials. Setting the key as-is would 401 every scrape and blind the fleet.What
GET /metricsbypasses admin auth; every other admin route stays guarded.Base / release mechanics
v0.9.30-shopify-patches= taglibsql-rs-v0.9.30+connection-initialization-patch(upstream 0.9.30 + FULLMUTEX patch), matching whatlibsql-infrastructure's Dockerfile builds today.libsql-rs-v0.9.30+connection-initialization-patch+admin-metrics) and bump the clone ref inShopify/libsql-infrastructure/libsql-server/Dockerfile+ VERSION to cut a new image.cargo check -p libsql-serverpasses.Order in the #34482 sequence
Must land before the
LIBSQL_ADMIN_AUTH_KEYenablement PR in Shopify/infrastructure.CI notes
rust_suitecrate has no committed lockfile, so each run fresh-resolvedicu_* 2.3.0(requires rustc 1.88) against the repo-pinned 1.85 toolchain. Fixed here by committinglibsql-sqlite3/test/rust_suite/Cargo.lockwithidna_adapterpinned to 1.1.0 (unicode-rs backend, drops icu from the graph entirely). Future PRs on this patch line inherit the fix.connection_core::test::test_many_concurrent(Run Tests) is flaky on shared runners — it's upstream's lock-stealing stress test and its own doc comment says "If this test becomes flaky check out the lock stealing code." It fails/passes independent of this change (observed both across identical commits); retry on failure.