Re: Review of WebRC PC Spec

Thanks for the review!

On 30/05/17 21:26, Eric Rescorla wrote:
> OVERALL
> I have not studied all the algorithms for how to handle asynchronous
> task queuing, but we seem kind of inconsistent on exactly when:
>
> - We report errors synchronously
> - When the task becomes due, if conditions X apply, don't run
>   it
> - When the task comes due, run the algorithm and then check
>   to see if condition X applies and then don't report.
>
> More generally, there is an enormous amount of boilerplate for
> handling deferred operations of various kinds. It would be
> far better if there were a single generic algorithm for handling
> all the deferred/queued operations and then we just defined
> the inner loop of those.
>
> Generally, I tend to think that more things should
> be async... That would make reasoning about this a lot
> easier.
>
>
> There seems to be inconsistency about whether these tables
> of enums are normative or non-normative. Compare S 4.2.6
> to S 4.2.5. Both of these seem to be defined in JSEP.
>
>
> This would all be a lot easier to read if the state definitions
> came before the enormously long block describing how to process
> every method.
>
>
> A similar comment about pulling stuff out is with the boilerplate
> about events not bubbling or being cancelable.
>
>
>
> TECHNICAL
> S 4.2.2.
> Generally, we try to avoid sharing the same initial keying material
> with two hashes, which seems to be explicitly what you propose
> here. With that said:
>
> - HMAC already has its own shortening algorithm (you hash)
> - You should use HKDF to generate fresh subkeys for each
>   hash algorithm.
>
>
>    Note that the OAuth response "key" parameter is a JSON Web Key (JWK)
>    or a JWK encrypted with a JWE format. Also note that this is the only
>    OAuth parameter whose value is not used directly, but must be
>    extracted from the "k" parameter value from the JWK, which contains
>    the needed base64-encoded "mac_key".
>
> It's been a while since I looked at this piece of OAuth, but what
> key is this encrypted with?
>
> Why are you requiring that the access token be self-encrypted rather
> than (say) a table lookup. This is a reasonable procedure, but it's
> not the only one, and not really the standard's business.
>
>
> S 4.2.6.
>    Gather ICE candidates for each media type in use (audio, video, and
>    data). If the remote endpoint is not bundle-aware, negotiate only
>    one audio and video track on separate transports.
>
> The second sentence here (and below) are just information, they're
> not instructions to the implementation, so they should be removed
> or rewritten.
>
>
> S 4.3.1
> (operations queue)
> This algorith seems to imply that operation is run synchronously
> if the queue depth is one (step 6) whereas if it's not, it's run
> asynchronously. This definitely isn't invisible in the callback
> version (reentrancy) and isn't actually invisible in the promises
> version because these operations have side effects. Is it really
> the expectation that we should (for instance) start sending
> media before the promise resolves? That seems odd.
>
>
> (setDescription)
> The says:
>     To set an RTCSessionDescription description on an RTCPeerConnection
>     object connection, enqueue the following steps:
>
>     1. Let p be a new promise.
>
> I may be confused, but I don't think that the promise creation and
> resolution is in fact queued.
>
>     2. If elements of the SDP were modified, then reject p with a newly
> created
>     InvalidModificationError and abort these steps.
>
> This only applies to local decriptions.
>
>
> This algorithm is extremely prescriptive about which error checks
> you need to perform and in which order. This is externally observable
> because I throw different errors. For instance, why is it the
> spec's business whether I check for being in the right state before
> or after I check for the SDP being modified.
>
>     5. If the content of description is invalid, then reject p with a
>     newly created InvalidAccessError and abort these steps.
>
> What would invalid be here? Anything else?
>
>
>     2. If description is applied successfully, the user agent must
>     queue a task that ru ns the following steps:
>
> Didn't we already queue above?
>
>
> S 4.2.3 (createOffer)
>
>    4. Return the result of enqueuing the following steps:
>       1. Let p be a new promise.
>
> This seems like another situation where a generic algorithm would
> help.
>
> (createAnswer)
>    setLocalDescription without causing an error as long as
>    setLocalDescription is called reasonably soon.
>
> I have no idea what "reasonably soon" means
>
>
> (setLocalDescription)
> This algorithm leaves the state of the PC changed in case of
> bogus inputs. However, JSEP requires that the state be unchanged
> (see JSEP S 5.8)
>
>
> (addIceCandidate)
>
>    3. If both sdpMid and sdpMLineIndex are null, return a promise
>    rejected with a newly created TypeError.
>
> See above about consistency. Why is the promise rejected here
> immediately but otherwise these are enqueued? How does that help?
>
>
> (setConfiguration)
>
>     3. For each url in server.urls parse url and obtain scheme
>     name. If the scheme name is not implemented by the browser, or if
>     parsing based on the syntax defined in [ RFC7064] and [RFC7065]
>     fails, throw a SyntaxError.
>
> This seems unwise as it makes it very hard to introduce new schemes.
> Why aren't they just ignored?
>
>
> S 4.3.3.
>     Supporting the methods in this section is optional. However, if
>     these methods are supported it is mandatory to implement according
>     to what is specified here.
>
> This doesn't match my memory of what we agreed when we added promises.
> When was this decision made?
>
>
> S 4.8.1.
> Seems like we are missing:
>
> - component-id
> - extension-att-name
> - extension-att-value
>
> Arguably, we don't need the last two but we do need component-id
>
> Also, you need to cite the relevant parts of 5245 here.
>
> S 4.8.3.
>
>     If no host candidate can reach the server, errorCode will be set
>     to the value 701 which is out- side the STUN error code
>     range. This error is only fired once per server URL while in the
>     RTCIceGatheringState of "gathering".
>
> This is kinda buried..
>
>
>
> S 5.1.
>
>    RID values passed into init.sendEncodings must be composed only of
>    case-sensitive al- phanumeric characters (a-z, A-Z, 0-9) up to a
>    maximum of 16 characters.
>
> Characters aren't case-sensitive. I assume you mean that rids that
> differ only in case are distinct.
>
>
> S 5.2.
> The advice here (center, scale, then crop) differs from a MUST
> in JSEP S 3.6.
>
>    If the original resolution exceeds the size limits in the attribute,
>    the sender SHOULD apply downscaling to the output of the
>    MediaStreamTrack in order to satisfy the limits.  Downscaling MUST
>    NOT change the track aspect ratio.
>
> S 5.2.
>    An unique identifier for the last set of parameters
>    applied. Ensures that setParameters can only be called based on a
>    previous getParameters, and that there are no intervening changes.
>
> Are there any restrictions on how this is generated.
>
>
> Shouldn't the definition RTCRtpEncodingParameters mark a bunch of stuff
> readonly?
>
>
> S 5.3.
>     Initialize track.label to the result of concatenating the string
> "remote " with kind.
>
> Does this mean I will have a lot of tracks labelled "remote video"? That
> seems
> unfortunate.
>
>
> S 5.5.1
>
>     One of the the hash function algorithms defined in the 'Hash function
>     Textual Names' registry, initially specified in [ RFC4572] Section
>     8. As noted in [JSEP] Section 5.2.1, the digest algorithm used for the
>     fingerprint matches that used in the certificate signature.
>
> I see that this was relaxed in 8122. I wonder if we wnt to relax it here
> and in JSEP.
>
>
> S 6.1.
>
>     6. If negotiated is false and label is longer than 65535 bytes long,
> throw a TypeError.
>     7. If negotiated is false and protocol is longer than 65535 bytes
> long, throw a TypeError.
>
> Why not just restrict this to 65k in general.
>
>
> S 6.2.
>     5. Attempt to send data on channel's underlying data transport; if
>     the data cannot be sent, e.g. because it would need to be buffered
>     but the buffer is full, the user agent must abruptly close
>     channel's underly- ing data transport with an error.
>
> This seems like a very unsafe API. Why not instead have it fail?
>
> S 6.4.
> You seem to be saying that I can't garbage collect the data channel
> as long as the SCTP connection is up even if the PC is closed? that
> seems surprising.
>
>
> S 7.2
> If I am reading this correctly, the intertone gap can be infinite,
> though tones can only be 6 s long?
>
>
> S 9.1.
> It's not quite clear to me when the IdP is ready? I guess when it has
> called register?
>
>
> S 9.3.
>    LOGINDONE
>
> I wonder if we should use WEBRTC-LOGINDONE or something
>
>
>
> S 9.4.
> It seems like some text about how to handle peerIdentity mismatches
> would be helpful here, although I recognize the normative text is
> elsewhere. I can supply some if people would like.
>
>
> S 9.6.
> Is there any valid reason to allow the protocol to be a DOMString as
> opposed to ASCII or LDH? It's about to be shoved into a URL...
>
>
> S 10.4.1.
> Do you think we should have have the target peerIdentity, not just
> isolated.
>
>
> S 11.
> Has someone tested these examples? I gave them a quick skim and saw
> some stuff that seems like it might be concerning, but maybe it's already
> been tested...
>
> If nothing else, it would be good to:
>
> - Refactor out the common code
> - Explicitly state what's different
>
> S 11.6.
> has anyone checked this diagram recently?
>
> -Ekr
>

Received on Wednesday, 31 May 2017 07:22:25 UTC