w3c
Invalid TURN credentials: What function should fail?
I had an issue come up with TURN credentials recently.
Sometimes, it's possible to tell that a TURN credential is invalid just from looking at them. For instance, username fields that violate the length restriction (509) in RFC 8489.
But I couldn't figure out from our spec what function should fail. Alternatives:
- RTCPeerConnection constructor or setConfiguration, where the parameters are passed in
- createOffer/addIceCandidate, where the candidates are used
- nowhere - just act as if the relevant TURN candidate was never generated
The last one is easiest to implement. Is it the right solution?
For a too long TURN username, pretty sure setConfig should fail, same as if some other invalid IceServer attribute was supplied.
I agree with @juberti that especially in a case where the error can be detected it should be raised as early as possible, thus in the constructor or setConfig. Which is also how Firefox rejects too long TURN usernames already.
"Nowhere" appears to be the worst option, as that could result in nobody noticing anything wrong until it is too late, e.g. multiple urls provided, but one is invalid but goes unnoticed.
Should be handled together with all the other failures of ICE server parsing.
4.4.1.1 says that UnknownError should be returned when "something else" fails.
section 4.4.6.6 (apply configuration) step 10.5 is where that validation should happen.
As @annevk mentioned, fetch and web socket handle these cases as a network error. To be consistent, we could surface these errors using icecandidateerror.
We already differ from fetch in some ways (e.g., failing immediately in setConfig for an unknown scheme), so I'd prefer a more explicit rationale rather than just 'consistency'.
There's multiple reasons to fail in the network layer rather than at the API:
- A generic network error used for various reasons gives less information to an attacker. Browsers can still expose more granular information in the developer console.
- A network error usually degrades more gracefully than an API starting to throw an exception it previously did not throw. There's less chance for the application to fall apart.
- Architecturally, using the network layer for security checks reduces the risk of new APIs forgetting about some of them.
Unknown schemes seem somewhat different though and it might well make sense to fail at the API boundary there to allow for feature testing.
I could get behind a delineation where missing, unknown, or unparsable data results in an exception (i.e., the current behaviors defined in https://w3c.github.io/webrtc-pc/#set-the-configuration) and values that just exceed some protocol limit result in an async error, the same as if the server had enforced the check.
That sounds reasonable to me, for what it's worth. (Fetch and XMLHttpRequest will also throw exceptions on the API side for similar reasons.) The main thing in my mind is to centralize on a "network error" in the "network layer" (which you can implement in all kinds of ways of course) for what might change over time and is not really the concern of the API itself, such as ports, mixed content blocking, CSP, etc.
Looks ok to me as well. WebSocket is throwing on bad schemes AFAIK, which is somehow a kind of type matching close to WebIDL, hence throwing is fine.
Other things like blocking ports or sanitisation checks for instance may actually happen out of process or done by control blockers, hence done asynchronously.
I'm guessing we'd want to use onicecandidateerror with error code 701 for this (which seems to be our current generic-error code). https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnectionIceErrorEvent
Proposal:
- If it's a parse error, just throw immediately (constructor/setConfiguration), it seems to be easiest to implement.
- If it's an error that happens asynchronously, including unable to reach the host, onicecandidateerror 701 makes sense.
Right. But that basically means all non-parse-errors necessarily need to be async.
As I understand, the proposal was accepted at the virtual interim, however when @aboba was filling in some missing details on the minutes he added this:
The IANA registry of existing STUN/TURN errorCode values is here: http://www.iana.org/assignments/stun-parameters/stun-parameters.xhtml
Example: RFC 8656 (TURN) defines error code 441 "Wrong Credentials".
So I think this changes the PR from "fire onicecandidateerror with errorCode 701" to "fire onicecandidateerror with errorCode 441 (for wrong credentials) or 701 (if no other error code applies".
If there already exists an errorCode the PR may simply be an editorial clarification.
I think you have to carefully consider whether to expose the exact failure to sites. (See also https://github.com/w3c/webrtc-extensions/issues/52#issuecomment-728811972.) E.g., "wrong credentials" sounds like the kind of code someone trying to brute-force credentials might like.
If we put the request on the wire and the server returns an error code, we should just use whatever the server sends back (although @annevk 's comment is worth considering as a separate concern). If we decide to not send the request because of our own enforcement, I think we should use 701. This way, any 4xx/5xx codes can be unambiguously attributed to the server, which I think is useful.
@annevk Would it be OK to land clarifying text to close this issue, and then add a follow-up issue about what to do to mitigate exploiting error codes? As-is, I don't think the proposed PR is changing any behavior, it's just clarifying what is implied we should already do.
How to mitigate brute-force attacks sounds like a bigger issue than exposing errorCodes. For example, even if we emit an error code that looks the same as "host not found", it would be a bit suspicious if onicecandidateerror fired faster or slower depending on if the host was actually not found.
Cool, ready for PR it is then. I filed https://github.com/w3c/webrtc-extensions/issues/70 specifically for your concerns.