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/inc/hubSpaceKeys.h src/hg/inc/hubSpaceKeys.h index c09fd75e091..5ae4eccb789 100644 --- src/hg/inc/hubSpaceKeys.h +++ src/hg/inc/hubSpaceKeys.h @@ -1,93 +1,105 @@ /* hubSpaceKeys.h was originally generated by the autoSql program, which also * generated hubSpaceKeys.c and hubSpaceKeys.sql. This header links the database and * the RAM representation of objects. */ #ifndef HUBSPACEKEYS_H #define HUBSPACEKEYS_H #define HUBSPACEKEYS_NUM_COLS 2 extern char *hubSpaceKeysCommaSepFieldNames; struct hubSpaceKeys /* Api keys for userNames to upload to hubSpace using hubtools */ { struct hubSpaceKeys *next; /* Next in singly linked list. */ char *userName; /* userName in gbMembers */ char *apiKey; /* random key */ }; void hubSpaceKeysStaticLoad(char **row, struct hubSpaceKeys *ret); /* Load a row from hubSpaceKeys table into ret. The contents of ret will * be replaced at the next call to this function. */ struct hubSpaceKeys *hubSpaceKeysLoad(char **row); /* Load a hubSpaceKeys from row fetched with select * from hubSpaceKeys * from database. Dispose of this with hubSpaceKeysFree(). */ struct hubSpaceKeys *hubSpaceKeysLoadAll(char *fileName); /* Load all hubSpaceKeys from whitespace-separated file. * Dispose of this with hubSpaceKeysFreeList(). */ struct hubSpaceKeys *hubSpaceKeysLoadAllByChar(char *fileName, char chopper); /* Load all hubSpaceKeys from chopper separated file. * Dispose of this with hubSpaceKeysFreeList(). */ #define hubSpaceKeysLoadAllByTab(a) hubSpaceKeysLoadAllByChar(a, '\t'); /* Load all hubSpaceKeys from tab separated file. * Dispose of this with hubSpaceKeysFreeList(). */ struct hubSpaceKeys *hubSpaceKeysCommaIn(char **pS, struct hubSpaceKeys *ret); /* Create a hubSpaceKeys out of a comma separated string. * This will fill in ret if non-null, otherwise will * return a new hubSpaceKeys */ void hubSpaceKeysFree(struct hubSpaceKeys **pEl); /* Free a single dynamically allocated hubSpaceKeys such as created * with hubSpaceKeysLoad(). */ void hubSpaceKeysFreeList(struct hubSpaceKeys **pList); /* Free a list of dynamically allocated hubSpaceKeys's */ void hubSpaceKeysOutput(struct hubSpaceKeys *el, FILE *f, char sep, char lastSep); /* Print out hubSpaceKeys. Separate fields with sep. Follow last field with lastSep. */ #define hubSpaceKeysTabOut(el,f) hubSpaceKeysOutput(el,f,'\t','\n'); /* Print out hubSpaceKeys as a line in a tab-separated file. */ #define hubSpaceKeysCommaOut(el,f) hubSpaceKeysOutput(el,f,',',','); /* Print out hubSpaceKeys as a comma separated list including final comma. */ /* -------------------------------- End autoSql Generated Code -------------------------------- */ #define AUTH_TABLE_DEFAULT "apiKeys" char *hubSpaceGetApiKey(char *userName); /* Grab the already created api key if it exists */ char *hubSpaceUserNameForApiKey(struct sqlConnection *conn, char *apiKey); /* Return userName associated with apiKey else NULL. If conn is NULL, will create a connection and free it. */ void hubSpaceRevokeApiKey(char *userName); /* Remove any api keys for userName. errAborts if userName is NULL. * Run in an errCatch to handle errors. */ char *hubSpaceGenerateApiKey(char *userName); /* Make a random (but not crypto-secure) api key for userName, for use of hubtools to upload * to hubspace or for bypassing cloudflare. errAborts if userName is NULL. * Run in an errCatch to handle errors. */ void hubSpaceSetApiKey(char *userName, char *apiKey); /* Set userName's api key to apiKey, replacing any existing key -- unlike hubSpaceGenerateApiKey, * this does not make up a new key. Used to adopt a key that a peer geo mirror generated, so * that a key works the same on every UCSC mirror. errAborts if userName or apiKey is NULL. */ -char *hubSpaceApiKeySyncSig(char *userName, char *apiKey); -/* Return a signature over userName and apiKey (empty string for a revoke), made with the - * login.cookieSalt shared secret that is already required to be identical across all of a - * site's geo mirrors (it is what makes the login cookie itself verifiable on every mirror). - * A peer mirror recomputes this to check that a hubSpaceSetApiKey/revoke request genuinely - * came from another UCSC mirror acting for this user, not from an outside caller. */ +#define HUB_APIKEY_SYNC_WINDOW 300 +/* How many seconds either side of now a signed api key sync is accepted for. Bounds how + * long a captured sync stays replayable; also absorbs clock skew between the mirrors. */ + +char *hubSpaceApiKeySyncSig(char *userName, char *apiKey, long timeStamp); +/* Return a signature over userName, apiKey (empty string for a revoke) and timeStamp (unix + * seconds), made with the login.cookieSalt shared secret that is already required to be + * identical across all of a site's geo mirrors (it is what makes the login cookie itself + * verifiable on every mirror). A peer mirror recomputes this to check that a + * hubSpaceSetApiKey/revoke request genuinely came from another UCSC mirror acting for this + * user, not from an outside caller. HMAC rather than a plain hash of secret+message, so + * the construction does not depend on the hash resisting length extension. */ + +boolean hubSpaceApiKeySyncSigOk(char *userName, char *apiKey, char *timeStampString, char *sig); +/* Return TRUE if sig is what this site would have signed over userName, apiKey and + * timeStampString, and that timestamp is inside HUB_APIKEY_SYNC_WINDOW of now. The window + * is what bounds replay: without it a captured sync could be resent at any time to + * reinstate a key its owner had since revoked. errAborts if login.cookieSalt is unset. */ #endif /* HUBSPACEKEYS_H */