github-such-software/smirk-backend-core
1
0
Fork 0
mirror of https://github.com/Such-Software/smirk-backend-core.git synced 2026-10-28 11:48:38 +01:00
Code Issues Packages Projects Releases Wiki Activity

fix: pre-ship audit remediation for auth identity, abuse control and retention

Browse Source 
Fixes every confirmed backend finding from the v0.3.0 pre-ship review.

Identity (the two that corrupt accounts):
- nostr_register creates a row with pubkey_hash NULL, so that wallet's later
  /auth/extension call matched nothing, fell through the rotation path (which no
  client ever satisfies, since nothing sends rotation_signature) and minted a
  SECOND account. A fingerprint match on a NULL-pubkey_hash row now adopts the
  submitted pubkey_hash onto that row, but only when the row's on-file BTC key
  equals the submitted one, so a bare fingerprint (which check_restore discloses,
  and is therefore not a secret) still cannot claim an account.
- The fingerprint-ownership check no longer short-circuits on returning_by_pubkey.
  It was skipped for returning users, so a fingerprint owned by another row got
  backfilled, violated the UNIQUE index, and 500'd that account on every
  subsequent login.
- The rotation proof is verified over a distinct, instance- and target-bound
  message. It shared the login message, so a captured login signature was a
  replayable rotation proof against another instance.

Abuse control:
- The rate limiters key on the real client IP, honouring X-Forwarded-For only
  from a trusted proxy. Behind the TLS terminator every request looked like
  127.0.0.1, so the whole internet shared one bucket and ~3 req/s 429'd every
  wallet unlock globally.
- A PoW solution is single-use instead of replayable for the challenge TTL.
- Delegated /verify is opt-in and rate-limited; it was an unauthenticated
  amplifier into Electrum, LWS and a full-genesis grin scan.
- A grin scan with no start_height gets the same depth policy and PoW price as
  one that passes a height, instead of an unbounded free scan.

Retention and logging:
- The retention sweep is implemented AND scheduled. RETENTION_* was advertised
  and inert, which made the privacy policy's retention promise untrue. Zero
  means keep forever, and each statement is batched so a backlog drains over
  ticks rather than holding row locks.
- login_events.ip_hash is no longer written, and legacy values are scrubbed:
  nothing ever read it, so it was retention without a purpose.
- Tracing spans log user_id only, not the raw npub and username.
- secret-scan.sh matches the coverage its comment claims.

Also fixes the test suite, which did not compile on this commit: NewSession
gained an id in c64cd9b and two call sites were never updated. mint_session now
binds one session_id for both the token and the row, since a different id there
would silently recreate the bug c64cd9b fixed.
This commit is contained in:
jw
2026-08-13 10:31:02 -04:00
parent 23659dfd5a
commit 81a3e4cf58
15 changed files with 989 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/api/auth.rs
+137 -25
View File
@@ -9,7 +9,9 @@
//! keyed by `pubkey_hash`. A derivation-scheme rotation (a known
//! `seed_fingerprint` at a new `pubkey_hash`) re-points an EXISTING user row
//! ONLY when the request additionally proves control of the BTC key already on
//! file for that user — never on a bare (unauthenticated) fingerprint.
//! file for that user — never on a bare (unauthenticated) fingerprint. The same
//! rule adopts a `pubkey_hash` onto a row that has none yet (an npub-native
//! registration): the BTC key on file must be the submitted one.
//! * [`nostr_login`] — NIP-98 (login grade) over the `Authorization` header.
//! Resolves an already-linked npub to its user; it NEVER creates a user.
//! * [`refresh_token`] — rotates a valid, still-active refresh token.
@@ -192,6 +194,31 @@ fn nip98_url(state: &AppState, path: &str) -> Result<String, AppError> {
Ok(format!("{}{}", base.trim_end_matches('/'), path))
}
/// The message a derivation-rotation proof must sign.
///
/// Deliberately DISTINCT from the ordinary login message (`smirk-auth-{ts}`), so
/// a captured login signature is useless as a rotation proof. It also commits to
/// the NEW `pubkey_hash` (a proof cannot be re-aimed at a different key) and to
/// this instance's canonical public API URL (a proof captured by a hostile
/// self-hosted instance cannot be replayed against another one).
///
/// `None` when the instance has no canonical URL configured: the proof cannot be
/// instance-bound, so we decline to rotate rather than accept a replayable proof.
fn rotation_message(
state: &AppState,
new_pubkey_hash: &str,
signed_timestamp: i64,
) -> Option<String> {
let cfg = state.cfg();
let instance = cfg.identity.public_api_url.as_deref()?;
Some(format!(
"smirk-rotate-v1|{}|{}|{}",
instance.trim_end_matches('/'),
new_pubkey_hash,
signed_timestamp
))
}
/// Build [`UserInfo`] from a DB user.
fn user_info(user: &crate::models::db::User) -> UserInfo {
UserInfo {
@@ -296,7 +323,9 @@ pub struct ExtensionRegisterRequest {
pub wallet_birthday: Option<i64>,
/// Seed fingerprint `hex(SHA256(SHA256(seed))[..])`. Used for restore and to
/// LOCATE a candidate user row for the derivation-rotation path. By itself it
/// is NOT authority: a rotation also requires `rotation_signature` below.
/// is NOT authority: re-pointing an existing row additionally requires either
/// `rotation_signature` below or that the row's on-file BTC key is exactly the
/// one submitted in `keys` (whose control this request already proves).
pub seed_fingerprint: Option<String>,
pub xmr_start_height: Option<i64>,
pub wow_start_height: Option<i64>,
@@ -305,12 +334,18 @@ pub struct ExtensionRegisterRequest {
/// BIP-137 base64 signature of `smirk-auth-{signed_timestamp}` under the
/// SUBMITTED (new) BTC key. Proves control of the key in `keys`.
pub signature: String,
/// Derivation-rotation proof: BIP-137 base64 signature of the SAME
/// `smirk-auth-{signed_timestamp}` message under the BTC key ALREADY ON FILE
/// for the user identified by `seed_fingerprint`. Required to re-point an
/// existing user row; without it (or if it does not verify against the stored
/// key) a fingerprint match is treated as a brand-new identity and the
/// existing row is never touched. See [`extension_register`].
/// Derivation-rotation proof: BIP-137 base64 signature under the BTC key
/// ALREADY ON FILE for the user identified by `seed_fingerprint`, over the
/// DEDICATED rotation message
/// `smirk-rotate-v1|{public_api_url}|{pubkey_hash}|{signed_timestamp}`, where
/// `public_api_url` is this instance's canonical API base with any trailing
/// `/` removed and `pubkey_hash` is `hex(SHA256(submitted btc public_key))`.
/// It is deliberately NOT the login message: a captured login signature must
/// be useless as a rotation proof, and a proof minted for one instance must
/// not verify on another. Required to re-point an existing user row; without
/// it (or if it does not verify against the stored key) a fingerprint match is
/// treated as a brand-new identity and the existing row is never touched. See
/// [`extension_register`].
#[serde(default)]
pub rotation_signature: Option<String>,
/// Optional proof-of-work solution. Required when the PoW gate applies to
@@ -346,10 +381,24 @@ pub struct ExtensionRegisterRequest {
/// request also carries a `rotation_signature` that verifies against the BTC key
/// already on file for that user (proving control of the seed-derived key, not
/// merely knowledge of the fingerprint, which `check_restore` discloses and is
/// not secret). A bare fingerprint match WITHOUT a valid rotation proof is
/// treated as a brand-new identity: a fresh user row is created on the new
/// `pubkey_hash` and the matched victim row is never modified. The rotation path
/// is gated by PoW exactly like any other new-pubkey registration.
/// not secret). That proof signs a dedicated message, NOT the login message: see
/// [`rotation_message`], which binds a rotation-specific prefix, the new
/// `pubkey_hash`, the timestamp and this instance's canonical public API URL, so
/// a captured login signature is worthless here and a proof minted for one
/// instance does not verify on another. A bare fingerprint match WITHOUT a valid
/// rotation proof is treated as a brand-new identity: a fresh user row is created
/// on the new `pubkey_hash` and the matched victim row is never modified. The
/// rotation path is gated by PoW exactly like any other new-pubkey registration.
///
/// ## Adopting a `pubkey_hash` onto a keyless row
///
/// A row minted by [`nostr_register`] carries no `pubkey_hash`, so that wallet's
/// first call here matches nothing by pubkey. When the fingerprint matches such a
/// row AND the BTC key on file for it is exactly the key submitted (whose control
/// this request already proved), it is the same wallet: the submitted
/// `pubkey_hash` is adopted onto that row (`is_new=false`) instead of minting a
/// second identity that would strand the handle, tips and premium state. If the
/// row already has a `pubkey_hash`, or the BTC keys differ, nothing is adopted.
#[utoipa::path(
post,
path = "/auth/extension",
@@ -395,12 +444,11 @@ pub async fn extension_register(
// literal-messaged, so this is not an oracle.
verify_bitcoin_signature(&message, &req.signature, &btc_key.public_key)?;
// Is the exact pubkey_hash already known? (Plain returning user.)
let returning_by_pubkey = state
.db
.get_user_by_pubkey_hash(&pubkey_hash)
.await?
.is_some();
// Is the exact pubkey_hash already known? (Plain returning user.) Keep the
// ROW, not just the boolean: the fingerprint-ownership check below needs its
// id to tell this user's own fingerprint from someone else's.
let existing_by_pubkey = state.db.get_user_by_pubkey_hash(&pubkey_hash).await?;
let returning_by_pubkey = existing_by_pubkey.is_some();
let wallet_birthday = req
.wallet_birthday
@@ -418,12 +466,66 @@ pub async fn extension_register(
// we cannot authenticate a rotation -> fall through to new-identity.
let stored_btc = state.db.get_user_key(target.id, AssetType::Btc).await?;
let rotation_proven = match (&req.rotation_signature, &stored_btc) {
(Some(sig), Some(stored)) => {
// Adopt the submitted pubkey_hash onto a row that has none yet.
// `nostr_register` mints its row with pubkey_hash NULL, so the
// same wallet's first /auth/extension matches nothing by pubkey
// and would otherwise mint a SECOND row, stranding the handle,
// tips and premium state on the first. Same seed, same BTC key:
// it is one wallet, so adopt rather than split the identity.
//
// The submitted `signature` was already verified against the
// submitted BTC key above, so requiring the on-file key to EQUAL
// it means the caller proved control of the key this row already
// carries. A bare fingerprint (which check_restore discloses, so
// it is not a secret) can never claim an account this way.
let adoptable = target.pubkey_hash.is_none()
&& stored_btc
.as_ref()
.is_some_and(|stored| stored.public_key == btc_key.public_key);
if adoptable {
// A move, not a mint (`is_new=false`): this row was already
// gated when it was created, so the invite / pay-to-register
// gates do not re-run here, exactly as on the rotation branch
// below. PoW still applies to the new pubkey.
enforce_pow(&state, &pubkey_hash_lc, false, req.altcha_solution.as_ref())?;
info!(user_id = %target.id, "adopting pubkey_hash onto keyless row");
state.db.update_pubkey_hash(target.id, &pubkey_hash).await?;
upsert_all_keys(&state, target.id, &req.keys).await?;
let pair =
issue_session(&state, target.id, Platform::Extension, "Browser Extension")
.await?;
let _ = state
.db
.record_login_event(
Some(target.id),
"btc",
Platform::Extension.as_str(),
None,
Some(&ip.to_string()),
)
.await;
return Ok(Json(AuthResponse {
access_token: pair.access_token,
refresh_token: pair.refresh_token,
expires_in: pair.expires_in,
user: user_info(&target),
is_new: false,
}));
}
// The rotation proof signs a DEDICATED message (see
// [`rotation_message`]), never the login message: a captured
// login signature must be useless as a rotation proof, and a
// proof minted for one instance must not verify on another.
let rotation_msg = rotation_message(&state, &pubkey_hash, req.signed_timestamp);
let rotation_proven = match (&req.rotation_signature, &stored_btc, &rotation_msg) {
(Some(sig), Some(stored), Some(msg)) => {
// Control of the on-file (seed-derived) key proves seed
// ownership. A bad signature is rejected (not an oracle:
// we simply decline to rotate and create a new identity).
verify_bitcoin_signature(&message, sig, &stored.public_key).is_ok()
verify_bitcoin_signature(msg, sig, &stored.public_key).is_ok()
}
_ => false,
};
@@ -489,13 +591,23 @@ pub async fn extension_register(
req.altcha_solution.as_ref(),
)?;
// Never attach a seed_fingerprint that already belongs to ANOTHER user (it
// would collide on the UNIQUE or hijack the lookup); a returning user only
// backfills NULLs anyway. Used as the new row's fingerprint below.
// would collide on the UNIQUE or hijack the lookup). This runs on EVERY path,
// returning users included: backfilling a fingerprint owned by another row
// violates that UNIQUE and then 500s every later login for this account. A
// fingerprint already on THIS user's row is its own, so it is kept.
let fingerprint_for_row = match &req.seed_fingerprint {
Some(fp) => {
let belongs_to_other =
!returning_by_pubkey && state.db.get_user_by_seed_fingerprint(fp).await?.is_some();
let owner = state.db.get_user_by_seed_fingerprint(fp).await?;
let belongs_to_other = match (&owner, &existing_by_pubkey) {
(Some(owner), Some(me)) => owner.id != me.id,
(Some(_), None) => true,
(None, _) => false,
};
if belongs_to_other {
warn!(
"extension_register: submitted seed_fingerprint belongs to another row; \
not attaching it"
);
None
} else {
Some(fp.clone())
src/api/verify.rs
+24 -1
View File
@@ -12,6 +12,13 @@
//! at deploy time by an nginx allow-list to moon.vote's mesh IP; there is no code
//! auth here on purpose (adding one would reject the fixed peer client).
//!
//! OFF BY DEFAULT (see [`routes`]): unauthenticated plus resource-expensive (it
//! drives Electrum, the LWS backends and a full grin-wallet rewind scan on
//! caller-supplied input) makes this an amplifier pointed at the operator's own
//! infrastructure, so an instance that has not negotiated the moon.vote peering
//! does not mount the route at all. When it IS enabled it sits on the strict
//! rate-limit tier; the nginx allow-list remains the primary containment.
//!
//! Response discipline: EVERY business outcome — including a failed check — is a
//! `200 OK` carrying `{verified, sender, amount, reason}`, where `reason` is drawn
//! from the fixed vocabulary in [`reason`] that the moon.vote client maps to typed
@@ -424,8 +431,24 @@ pub async fn verify_handler(
// ── router ────────────────────────────────────────────────────────────────────
/// The verify route, RELATIVE to the `/api/v1` mount point.
/// Whether the operator has turned the delegated-verify endpoint on. Default OFF:
/// the handler is unauthenticated and spends the operator's own Electrum / LWS /
/// grin-wallet capacity on caller-supplied addresses and view credentials, so an
/// instance that has not asked for the moon.vote peering must not expose it.
/// Read through the one flag parser every feature flag uses, so "on" means the
/// same thing here as everywhere else; an unset or unparseable value is off.
pub(crate) fn delegated_verify_enabled() -> bool {
crate::api::capabilities::env_flag_enabled("FEATURE_DELEGATED_VERIFY")
}
/// The verify route, RELATIVE to the `/api/v1` mount point. Returns an EMPTY
/// router unless the operator enabled the feature, so a request to `/verify`
/// then 404s (the route does not exist) rather than reaching the handler. The
/// caller mounts this on the strict rate-limit tier.
pub fn routes() -> Router<Arc<AppState>> {
if !delegated_verify_enabled() {
return Router::new();
}
Router::new().route("/verify", post(verify_handler))
}
src/api/wallet/grin.rs
+28 -14
View File
@@ -52,6 +52,12 @@ fn grin_client(state: &AppState) -> Result<&GrinClient, AppError> {
pub struct GrinScanRequest {
/// The wallet's `rewind_hash` (64 hex). Forwarded to grin-wallet; not stored.
pub rewind_hash: String,
// Omitting this is NOT a free full-chain scan: `scan` resolves it to the
// deepest height this instance permits and gates/prices THAT (see the handler).
// A client that must solve the restore PoW should send an explicit height,
// because the nonce binds to the height actually scanned. Kept out of the doc
// comment so the committed `openapi.json` (drift-gated in CI) stays byte-equal;
// regenerate the spec if this is ever promoted to `///`.
/// Scan from this block height (wallet birthday / last scanned). Omit for full.
pub start_height: Option<u64>,
/// Restore proof-of-work nonce. Required when the instance prices the
@@ -200,23 +206,30 @@ pub async fn scan(
// Restore-depth + PoW gate runs ONCE, before path selection, and is TERMINAL:
// a policy rejection returns 400 directly. It must never be masked by the
// grin-wallet fallback, so it is enforced before either scan path is chosen.
if let Some(h) = req.start_height {
let tip = grin_tip(&state).await?;
state.cfg().restore.enforce("grin", h, tip)?;
state.cfg().restore.enforce_restore_pow(
"grin",
&req.rewind_hash,
h,
tip,
req.restore_pow_nonce,
)?;
}
//
// An OMITTED start_height used to skip this block entirely, so leaving the
// field out bought an unbounded full-chain scan for free while an honest
// caller paid for depth. Resolve the omission to the deepest height this
// instance permits and gate/price THAT, then scan from it: both callers now
// pay the same for the same work.
let tip = grin_tip(&state).await?;
let start_height = req
.start_height
.unwrap_or_else(|| state.cfg().restore.min_start_height("grin", tip));
state.cfg().restore.enforce("grin", start_height, tip)?;
state.cfg().restore.enforce_restore_pow(
"grin",
&req.rewind_hash,
start_height,
tip,
req.restore_pow_nonce,
)?;
// Default to grin-lws when configured. It is trusted ONLY when provably synced
// to the tip; otherwise (and on any transport error) the scan falls back to
// the authoritative grin-wallet scan below.
if let Some(lws) = state.chains.grin_lws.as_ref() {
match scan_via_grin_lws(lws, &req).await {
match scan_via_grin_lws(lws, &req, start_height).await {
Ok(Some(resp)) => return Ok(Json(resp)),
Ok(None) => {} // still backfilling — fall through to grin-wallet
Err(e) => {
@@ -229,7 +242,7 @@ pub async fn scan(
// synthesize an empty/zero success.
let client = grin_client(&state)?;
let view = client
.scan_rewind_hash(&req.rewind_hash, req.start_height)
.scan_rewind_hash(&req.rewind_hash, Some(start_height))
.await?;
Ok(Json(GrinScanResponse {
outputs: view.output_result.into_iter().map(output_dto).collect(),
@@ -265,8 +278,9 @@ async fn grin_tip(state: &AppState) -> Result<u64, AppError> {
async fn scan_via_grin_lws(
lws: &GrinLwsClient,
req: &GrinScanRequest,
start_height: u64,
) -> Result<Option<GrinScanResponse>, AppError> {
lws.register(&req.rewind_hash, req.start_height).await?;
lws.register(&req.rewind_hash, Some(start_height)).await?;
let bal = lws.get_balance(&req.rewind_hash).await?;
// Trust grin-lws only when its scan has effectively reached the tip.
// `saturating_add` so a hostile/garbled `scanned_height` can never overflow