Repository navigation
fix(auth): refuse weak HMAC consumer secrets and warn when OpenRegister is too old - #2618
Merged
Merged
Conversation
…er is too old Two findings from Robert's re-review of the beta promotion (#2154). Credential-validation regression: the web-token verifier this app used before gate 23 refused an HMAC secret shorter than the hash output ("Invalid key length."); OpenRegister's hash_hmac() accepts any secret, the empty one included, so an HS* consumer with a short or empty secret accepted tokens integriq used to refuse. The bridge now reads the issuer from the unverified payload only to look up its stored configuration, and refuses an HS256/HS384/HS512 issuer whose secret is below 32/48/64 bytes (RFC 7518 §3.2) before OpenRegister sees the token. OpenRegister gets the same refusal in its own validator; this guard keeps integriq safe on every OpenRegister in the field. Release ordering: on an OpenRegister without the public credential checks (stable 2.1.0 and older) the bridge refuses every credentialed inbound call with 401, and info.xml cannot say so. A new setup check, OpenRegisterEntryPointsCheck, shares the bridge's probe and reports an error naming OpenRegister 2.1.35 as the minimum; the changelog carries the requirement under Changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
WilcoLouwerse
requested review from
Rem-Dam,
bbrands02,
rjzondervan and
rubenvdlinde
as code owners
October 8, 2026 13:03
…heck, as the sibling checks do SetupResult's named constructors are the only way Nextcloud offers to build one, and the bridge's probe is static so the check and the bridge share one verdict; phpmd's StaticAccess rule flags both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Robert's re-review of the beta promotion #2154: a credential-validation regression in the OpenRegister delegation (blocker) and a release-ordering concern (integriq beta on stable OpenRegister refuses every credentialed call).
What
The regression. The web-token verifier this app used before gate 23 refused an HMAC key shorter than the hash output (
Invalid key length.,HS256.php:30). OpenRegister'sJwtValidator::verifySignature()useshash_hmac(), which accepts any secret, the empty one included. So an HS* consumer with a short or empty secret accepted tokens integriq used to refuse, and with an empty secret anyone can mint one. The bridge now readsissfrom the unverified payload only to look up the issuer's stored configuration (as OpenRegister itself does), and refuses an HS256/HS384/HS512 issuer whose secret is below 32/48/64 bytes (RFC 7518 §3.2) before OpenRegister sees the token, with a reason that names the cause. An unknown issuer is still left to OpenRegister. OpenRegister gets the same refusal in its own validator (ConductionNL/openregisterfix/woo589-hmac-secret-length); this guard keeps integriq safe on every OpenRegister in the field.Release ordering. A new setup check
OpenRegisterEntryPointsCheckshares the bridge's probe (OpenRegisterCredentialBridge::entryPointsAvailable()) and reports an error in the administration overview naming OpenRegister 2.1.35 as the minimum when the installed one lacks the public credential checks, so an admin sees it before the first 401. The class is named as a string so the check loads without OpenRegister; an unresolvable service is a warning.CHANGELOG.mdcarries the requirement under Changed and the secret rule under Security, so the release notes say it too.Verification
tests/Unit/Service/Consumer/OpenRegisterCredentialBridgeHmacSecretTest.php: the empty secret, 31 bytes for HS256, 32 for HS384 and 63 for HS512 are refused before anybody acts as the consumer's user; a 32-byte HS256 secret goes through to OpenRegister and is accepted; an unknown issuer is still refused by OpenRegister.tests/Unit/SetupCheck/OpenRegisterEntryPointsCheckTest.php: current OpenRegister passes, an object with the check protected is an error naming the version and the 401, an unresolvable service is a warning.Part of WOO-589.
🤖 Generated with Claude Code