feat(signalling): add an optional shared token on the player port - #979
Merged
Merged
Conversation
Adds --player_token / --player_token_file, checked with the
playerWsOptions.verifyClient seam the security guidelines already
recommend. A player that cannot present it is refused during the HTTP
upgrade with 401, so it never becomes a connection and is never sent the
config message - which is where the peer options, and any TURN
credential in them, would otherwise be handed over.
The token is accepted as a ?token= query parameter or an Authorization:
Bearer header, either of them, and compared over SHA-256 digests so that
neither its length nor the position of the first difference is
observable.
Four things the mechanism has to get right, each covered by tests:
- A request target that is not a parseable URL fails closed. Node's
HTTP parser accepts targets the URL constructor rejects, and a throw
inside verifyClient escapes through ws's upgrade handler and ends the
process - so "//" would be an unauthenticated remote kill.
- The reader and the remover agree on what the token parameter is, in
both directions. The token is spliced out of an accepted request's
target because the signalling server logs it and that log is rotated
and backed up; if the two disagreed, an accepted request would keep
its credential in the line that gets written. Hence reading every
token parameter rather than the first, and matching the URL parser's
treatment of encoded and control characters in a key. Splicing rather
than rebuilding the target leaves every other parameter exactly as
the client wrote it, since the request is handed to consumer code.
- A misconfigured token fails closed and says so. A config file is
written by hand, so a value that cannot be a token refuses to start
rather than leaving the port open, and one that can be coerced into
a token says which text it became.
- Refusals are logged with their source address. The player port is
not covered by the HTTP rate limiter, which only sees request
events, so guessing is cheap and an operator needs to be able to see
it happening. A token short enough to matter is warned about at
startup.
Streamer and SFU connections are unaffected, and no player connection is
refused when no token is supplied. Two things do change for a deployment
that uses --turn_secret without a token: it now warns at startup that
TURN credentials are issued to anyone, and the interactive config dump
redacts turn_secret as the startup dump already did.
🦋 Changeset detectedLatest commit: b4b134f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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
2 participants
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.
Relevant components:
Problem statement:
Docs/Security-Guidelines.mdtells a deployment to authenticate at the WebSocket upgrade with averifyClientonplayerWsOptions, and that is the right place — it runs before the config message, so a refused peer is never sentpeerConnectionOptions. But the signalling server gives you no way to reach that seam, so even the simplest version of it means forking the reference server.--turn_secretmade the gap more obvious. A time limited credential decides how long a leak stays usable, not who is issued one, and anything that can open a player socket is issued one.Solution
--player_token(or--player_token_file) requires a player to present a token, as a?token=query parameter or anAuthorization: Bearerheader. One that cannot is refused during the upgrade with401, so it never becomes a connection and never receives the config message. Streamer and SFU connections are untouched, and with no token configuredplayerWsOptionsis not set at all.It is deliberately the least ambitious useful thing: one shared token, the same for everyone, with no identity and no expiry. Enough for a kiosk or an internal demo, and the documentation says plainly that anything needing sessions or identity should supply its own
verifyClientinstead.Two implementation notes, since both are easy to get wrong in a security feature. A request target that will not parse is refused rather than allowed to throw — Node's HTTP parser accepts targets
new URL()rejects, and a throw insideverifyClientescapes through ws's upgrade handler and ends the process. And the token is removed from an accepted request beforeSignallingServerlogs the URL it connected with, because that log is rotated and backed up.Two things I would rather raise than have found:
This is authentication, and the guidelines said the project ships none. I have amended that sentence rather than leave it contradicting the new section below it. I think it belongs here for the same reason
--turn_secretdoes — the library keeps the policy-free hook, the application implements the common case — but if you would rather the reference server stayed clear of it, I would genuinely rather hear that than have it merged reluctantly.The bundled frontend cannot use it unaided. It builds its signalling URL from the page's protocol, host and port only, so a
?token=on the page address never reaches the socket. The docs give the?ss=form that works today. TeachingConfig.tsto carry the parameter through would make the obvious thing work, but it couples the frontend library to a flag name in the application, so I left it out — happy to add it if you would prefer.Documentation
A subsection under the upgrade hook in
Docs/Security-Guidelines.md: what the flag is, what it is not, and the caveats worth knowing — it gates the player port only, nothing rate limits guessing, and a query string is visible to anything that logs URLs. It also notes that--rest_api'sGET /api/configalready discloses peer options without a token.SignallingWebServer/README.mdlists both options.Test Plan and Compatibility
39 unit tests: bypass attempts, both transports, malformed targets, and the URL rewriting. Also run against a live server — no token, a wrong one, a prefix and an empty one are each refused; both transports connect; every malformed target is refused with the server still serving afterwards; and the connection log shows the token absent. With no token configured, all of the same requests connect exactly as they do on
master.npm run build,npm run lintandnpm run testpass forSignallingWebServer.