Q/Valid, Q/Utils: fail closed when Q/internal/secret is not configured

[zattak1]

Q/Valid, Q/Utils: fail closed when Q/internal/secret is not configured

Internal request signing is fail-open when "Q"/"internal"/"secret" is unset,
in two different ways.

1. Q_Valid::signature() returns TRUE for every payload:

   if (!isset($secret)) {
       return true;
   }
   

   It is a validator, and it reads "no key" as "valid". Everything built on it
   is an internal gate -- handlers/Q/config/validate.php, which writes config
   onto the machine; Streams::invites(); Media/callCenter -- so an install that
   has not set the secret accepts forged internal requests while looking
   healthy. All three of those call sites pass $throwIfInvalid = true, so the
   rejection surfaces as the existing Q_Response::code(403), with
   Q_Exception_MissingConfig naming the key at fault rather than blaming the
   caller for a local misconfiguration.

2. Q_Utils::signature() and Q_Utils::sign() fall back to
   generateLocalSecret(), a sha256 of gethostname(), PHP_OS, APP_DIR and
   /etc/machine-id (or the Windows MachineGuid). Every component is public or
   identical across hosts deployed from the same image, so it is a secret in
   name only, and callers cannot tell they were handed one instead of the
   configured key. It also differs between the PHP host and the Node host, so
   PHP <-> Node internal signing silently never verifies.

Rejecting on the verifying side is only safe together with failing loudly on
the signing side -- otherwise a silent hole is traded for a silent lockout:
sign, hand out something no peer can verify, and see no error anywhere. So both
halves move together, and generateLocalSecret() is deleted rather than left
unused.

Q_Utils::requireInternalSecret() also treats the empty string and the
"TODO: CHANGE TO SOME RANDOM STRING, ..." literal that local.sample/app.json
ships as unconfigured. That literal is a fixed value published in every copy of
this repository, so an install that keeps it is not weakly configured -- it
holds a secret every attacker already has, and can therefore sign.

platform/classes/Q/Utils.js carries the identical fallback in signature(),
sign() and validate(), whose docblock advertised "Returns true if secret is
empty". Fixed the same way so the two runtimes agree.

Behaviour with a secret configured is unchanged: every path that passes an
explicit $secret, and every path reading a real configured secret, is untouched.

[EGreg]

The Q_Valid::signature() half of this is right and I want it. A validator that returns true because it has no key is the same defect as #47, and this is the worse instance of it: handlers/Q/config/validate.php is the gate on Q_Config_post(), which dispatches to Q_Config::setOnServer() and clearOnServer() with a client-supplied filename and data. On an install with no Q/internal/secret, that's an unsigned POST writing config files onto the machine. Thanks for finding it.

The placeholder detection is right too. A TODO: string that ships in every copy of the repository is worse than no secret at all — it's a key everyone has rather than a key nobody has. Good call treating it as unconfigured.

What I'd like changed is removing generateLocalSecret() and making Q_Utils::signature() itself throw.

Q/Session.php calls Q_Utils::signature() nine times now, on every request, inside isValidId() — which runs from Bootstrap::validateEarly(), before there's any sensible error path. So on an install missing the secret this turns a degraded mode into a 500 on every request, and there's no install-time config check anywhere in the tree to catch it earlier. The throw becomes the discovery mechanism, and the first visitor after a deploy is the worst possible discoverer.

The distinction I'd draw is between producing a signature and accepting one. Accepting an unsigned request must fail closed, always — that's this PR's real finding. Producing a signature, or verifying an artifact we issued ourselves, can degrade to a machine-local key without letting anyone in, and that's what keeps a misconfigured install serving instead of hard-erroring at the door.

Concretely, what I'd merge:

• Q_Valid::signature() fails closed. Keep it exactly as you have it.
• Q_Utils::sign() throws, since that's an outbound credential and signing with a guessable key is worse than not signing.
• Q_Utils::signature() keeps generateLocalSecret() as its fallback.
• Add the check to Q/install.php so a missing or placeholder secret fails at install time, loudly, with the config path in the message. That's where an operator can actually act on it.
• Same split on the Node side in Utils.js.

I'd also take a static memo inside generateLocalSecret() if you're in there anyway. It's uncached today, so it re-reads /etc/machine-id on every call and spawns reg query through shell_exec on Windows — which barely mattered before and matters more now that session validation calls it several times per request on an unconfigured install. The inputs are fixed for the life of the process, so memoizing changes no output.

Happy to take this as a rewrite whenever you have time, or split it: the Q_Valid change alone would be an easy merge today, and the Q_Utils policy can land separately.

[zattak1]

Split as suggested:

• Q_Valid::signature(), Utils.validate(): fail closed when Q/internal/secret is not configured #54 — the accepting side only: Q_Valid::signature() fails closed, Utils.validate() returns false, plus the placeholder detection in a new additive Q_Utils::requireInternalSecret(). Nothing about producing signatures changes there.
• Q_Utils::sign() throws without Q/internal/secret; signature() keeps the local fallback; install.php checks the secret #55 — based on Q_Valid::signature(), Utils.validate(): fail closed when Q/internal/secret is not configured #54: Q_Utils::sign() throws, Q_Utils::signature() keeps generateLocalSecret() (with the comment saying why it is allowed to degrade), a static memo in generateLocalSecret(), the check in Q_Plugin::installApp() with the config path in the message, and the same split in Utils.js.

On the distinction you drew — producing vs. accepting — agreed, and it is a better line than the one this PR drew. The runtime throw made sense against a Session.php that called signature() roughly once; against cf79eaa it doesn't, and an install-time check is where an operator can actually act. Closing this in favour of the two.