33eb806d3707661d8a4d9af1e0673f90578d86bc max Mon Sep 21 06:12:21 2026 -0700 hgHubConnect: the api key sync has to answer before the cart, and its signature needed a timestamp, refs #38323 Six things, all in the syncHubApiKeys path, which is still off everywhere. The sync never reached its handler on a real mirror. It was registered as a cartJson command, so building the cart ran forceUserIdOrCaptcha() first; a peer's request carries no hguid cookie and no apiKey= variable, so any site with cloudFlareSiteKey set -- which is every site that needs api keys in the first place -- answered it with a captcha page. Only hgwdev, where the captcha is commented out, ever ran the handler, which is why this was not caught earlier. doApiKeySyncIfRequested() now answers the request in main() before any cart exists. That also stops each sync leaving a junk userDb and sessionDb row behind: measured, 15 syncs now create none. The signature is HMAC-SHA256 over the salt instead of md5(salt-user-key), so it does not rest on md5 resisting length extension, and it is compared in constant time. It now covers a timestamp too, and a peer refuses anything signed more than HUB_APIKEY_SYNC_WINDOW (300s) either side of now, so a captured sync cannot be replayed later to reinstate a key its owner has since revoked. The fields are joined with newlines and a newline in a userName or apiKey is refused, so one signature cannot cover two different splits of the same string. syncApiKeyToOtherNodes() is called inside an errCatch of its own now. It runs after the local key is already written and in the response, so an errAbort there (login.cookieSalt unset) used to reach cartJsonExecute's outer catch, which threw the response away -- the user saw a failure over a key that was sitting in the database. geoMirrorNotifyOtherNodes() returns each peer's response instead of discarding it, and uses netUrlMustOpenPastHeader, which errAborts on anything but a 200 and hands back the body alone. A peer answering 'bad signature' or 'not enabled on this site' is no longer indistinguishable from success; it is logged to error_log, not warn()ed at the person who clicked the button. Verified against the built CGI run as Apache runs it, with the captcha turned on: a signed sync is accepted and writes the row, a signed revoke removes it, and a replay, a swapped userName, a forged signature and malformed json are all refused. hubSpaceKeysTester covers the signature rules; it still cannot run its database half until hgcentralregress gets the UPDATE and DELETE grant. diff --git src/hg/lib/geoMirror.c src/hg/lib/geoMirror.c index 2da6a2eb62f..6fe482b9e85 100644 --- src/hg/lib/geoMirror.c +++ src/hg/lib/geoMirror.c @@ -332,64 +332,82 @@ struct slPair *geoMirrorThisNode() /* Return this node (browser.node) as a single pair of name=shortLabel, val=domain, or NULL when * geo mirroring is off or gbNode has no row for it. slPairFreeValsAndList when done. */ { return geoMirrorNodeList(TRUE); } struct slPair *geoMirrorOtherNodes() /* Return the other geo mirror nodes, as pairs of name=shortLabel, val=domain, ordered by node. * The node this CGI is running on (browser.node) is left out. Returns NULL when geo mirroring * is off or this is the only node. slPairFreeValsAndList when done. */ { return geoMirrorNodeList(FALSE); } -void geoMirrorNotifyOtherNodes(char *cgiName, struct slPair *cgiVars) +struct slPair *geoMirrorNotifyOtherNodes(char *cgiName, struct slPair *cgiVars) /* Best-effort: fire cgiVars (name=value) as a GET request at cgiName on every other geo mirror - * node (per geoMirrorOtherNodes()). No-ops if geo mirroring is off or this is the only node. + * node (per geoMirrorOtherNodes()). Returns one pair per node attempted, name=node domain and + * val=the response body, or val=NULL for a node that could not be reached or answered anything + * but a 200 -- the caller is expected to look at what came back, since a peer that refuses the + * request answers with a body, not with a connection failure. Returns NULL when geo mirroring + * is off or this is the only node. slPairFreeValsAndList when done. * Adds no authentication of its own -- callers must put their own signed proof into cgiVars, * since the receiving CGI runs with no session/cart tying the request to a user. A slow or - * unreachable peer is logged with warn() and skipped; the caller's own action must already be + * unreachable peer is logged to stderr and skipped; the caller's own action must already be * complete locally before this is called, since a peer being down must never fail the local * action. */ { struct slPair *nodes = geoMirrorOtherNodes(); -struct slPair *node; +struct slPair *node, *results = NULL; for (node = nodes; node != NULL; node = node->next) { - // https, not http: the mirrors redirect http to https and netSlurpUrl does not follow - // redirects, so an http request never reaches the CGI at all - struct dyString *url = dyStringCreate("https://%s/cgi-bin/%s?", (char *)node->val, cgiName); + char *domain = (char *)node->val; + // https, not http: the mirrors redirect http to https, and sending a secret in the clear + // to Germany and Japan would leave it in each peer's access log besides + struct dyString *url = dyStringCreate("https://%s/cgi-bin/%s?", domain, cgiName); struct slPair *var; for (var = cgiVars; var != NULL; var = var->next) dyStringPrintf(url, "%s%s=%s", (var == cgiVars) ? "" : "&", var->name, cgiEncodeFull((char *)var->val)); + char *body = NULL; struct errCatch *errCatch = errCatchNew(); if (errCatchStart(errCatch)) { - struct dyString *response = netSlurpUrl(url->string); - dyStringFree(&response); + // MustOpenPastHeader, not netSlurpUrl: it errAborts on anything but a 200 and hands + // back the body alone, so the caller does not have to pick it out of the headers + int sd = netUrlMustOpenPastHeader(url->string); + struct dyString *response = netSlurpFile(sd); + close(sd); + body = dyStringCannibalize(&response); } errCatchEnd(errCatch); if (errCatch->gotError) - warn("geoMirrorNotifyOtherNodes: failed to reach %s (%s): %s", - node->name, (char *)node->val, errCatch->message->string); + { + // stderr, not warn(): this is between two servers, and the person who clicked the + // button in the browser can do nothing about a peer being down + fprintf(stderr, "geoMirrorNotifyOtherNodes: failed to reach %s (%s): %s\n", + node->name, domain, errCatch->message->string); + freez(&body); + } errCatchFree(&errCatch); + slPairAdd(&results, domain, body); dyStringFree(&url); } slPairFreeValsAndList(&nodes); +slReverse(&results); +return results; } char *geoMirrorMenu() /* Create customized geoMirror menu string for substitution of into * in htdocs/inc/globalNavBar.inc * Reads hgcentral geo tables and hg.conf settings. * Free the returned string when done. */ { struct dyString *dy = dyStringNew(0); // by default replacment is just an empty string. if (geoMirrorEnabled()) { dyStringAppend(dy, "
    • \n"); // Geo mirror functionality (e.g. in nav bar) char *myNode = geoMirrorNode(); /* hgcentral.gbNode table so UI can share w/ browser GEO mirror redirect code */