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/hubSpaceKeys.c src/hg/lib/hubSpaceKeys.c
index a00345421a2..86932e2d76c 100644
--- src/hg/lib/hubSpaceKeys.c
+++ src/hg/lib/hubSpaceKeys.c
@@ -1,28 +1,29 @@
 /* hubSpaceKeys.c was originally generated by the autoSql program, which also 
  * generated hubSpaceKeys.h and hubSpaceKeys.sql.  This module links the database and
  * the RAM representation of objects. */
 
 #include "common.h"
 #include "linefile.h"
 #include "dystring.h"
 #include "jksql.h"
 #include "hubSpaceKeys.h"
 #include "hdb.h"
 #include "hgConfig.h"
 #include "htmshell.h"
 #include "md5.h"
+#include "hmac.h"
 
 
 char *hubSpaceKeysCommaSepFieldNames = "userName,apiKey";
 
 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. */
 {
 
 ret->userName = row[0];
 ret->apiKey = row[1];
 }
 
 struct hubSpaceKeys *hubSpaceKeysLoad(char **row)
 /* Load a hubSpaceKeys from row fetched with select * from hubSpaceKeys
@@ -192,29 +193,92 @@
 char *apiKey = makeRandomKey(256); // just needs some arbitrary length
 hubSpaceSaveApiKey(userName, apiKey);
 return apiKey;
 }
 
 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. */
 {
 if (!userName || !apiKey)
     errAbort("hubSpaceSetApiKey: need both a userName and an apiKey");
 hubSpaceSaveApiKey(userName, apiKey);
 }
 
-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. */
+static boolean hasNewLine(char *s)
+/* Return TRUE if s is non-NULL and holds a carriage return or a line feed. */
+{
+return (s != NULL && (strchr(s, '\n') != NULL || strchr(s, '\r') != NULL));
+}
+
+static char *apiKeySyncSalt()
+/* The shared secret the mirrors sign api key syncs with.  errAborts if it is not set. */
 {
 char *salt = cfgOption("login.cookieSalt");
 if (isEmpty(salt))
     errAbort("hubSpaceApiKeySyncSig: login.cookieSalt must be set to sync api keys across mirrors");
+return salt;
+}
+
+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. */
+{
+// The fields are joined with a newline, so no field may contain one: otherwise ("a", "b\nc")
+// and ("a\nb", "c") join to the same string and one signature covers both, and a peer that
+// checks the signature would still write the wrong userName's row.  A real userName is an
+// email address and a real apiKey is hex from makeRandomKey(), so this costs nothing -- but
+// the receiving side takes both from the request, so it has to be enforced, not assumed.
+if (hasNewLine(userName) || hasNewLine(apiKey))
+    errAbort("hubSpaceApiKeySyncSig: a userName or apiKey may not contain a newline");
 char buf[1024];
-safef(buf, sizeof buf, "%s-%s-%s", salt, userName, apiKey ? apiKey : "");
-return md5HexForString(buf);
+safef(buf, sizeof buf, "hgHubSyncApiKey\n%s\n%s\n%ld", userName, apiKey ? apiKey : "",
+        timeStamp);
+return hmacSha256(apiKeySyncSalt(), buf);
+}
+
+static boolean constantTimeSameString(char *a, char *b)
+/* Return TRUE if a and b are equal, in time that does not depend on how far along they
+ * first differ, so comparing a signature does not leak it one byte at a time. */
+{
+if (a == NULL || b == NULL)
+    return (a == b);
+size_t aLen = strlen(a), bLen = strlen(b);
+// The lengths of our signatures are not secret, only their contents
+if (aLen != bLen)
+    return FALSE;
+unsigned char diff = 0;
+size_t i;
+for (i = 0;  i < aLen;  i++)
+    diff |= (unsigned char)(a[i] ^ b[i]);
+return (diff == 0);
+}
+
+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. */
+{
+if (isEmpty(userName) || apiKey == NULL || isEmpty(timeStampString) || isEmpty(sig))
+    return FALSE;
+// refused here rather than left to errAbort in hubSpaceApiKeySyncSig, so that a peer asking
+// about a nonsense userName gets the same plain "no" as one with a bad signature
+if (hasNewLine(userName) || hasNewLine(apiKey))
+    return FALSE;
+char *end = NULL;
+long timeStamp = strtol(timeStampString, &end, 10);
+if (end == timeStampString || (end != NULL && *end != '\0'))
+    return FALSE;
+// Signed in the future as well as in the past: mirror clocks are not identical, and a peer
+// a few seconds ahead of us must still be able to talk to us.
+long age = (long)time(NULL) - timeStamp;
+if (age < -HUB_APIKEY_SYNC_WINDOW || age > HUB_APIKEY_SYNC_WINDOW)
+    return FALSE;
+return constantTimeSameString(sig, hubSpaceApiKeySyncSig(userName, apiKey, timeStamp));
 }