- From: Darrel Miller via Datatracker <noreply@ietf.org>
- Date: Sun, 01 Oct 2023 12:48:29 -0700
- To: <ietf-http-wg@w3.org>
- Cc: draft-ietf-wish-whip.all@ietf.org, last-call@ietf.org, wish@ietf.org
Reviewer: Darrel Miller Review result: Almost Ready I reviewed this document as part of the HTTP Directorate's ongoing effort to review all IETF documents being processed by the IESG. These comments were written primarily for the benefit of the HTTP Area Directors. Document authors, document editors, and WG chairs should treat these comments just like any other IETF Last Call comments. Summary: Has Issues Major Concerns Section 2: Terminology Considering this is a protocol layered on top of HTTP it would be useful to be consistent with HTTP terminology. "Endpoint" is not a term defined in HTTP, but "resource" is. Calling two distinct protocol concepts "WHIP Endpoint" and "WHIP Resource" is confusing from an HTTP perspective as they are both HTTP resources. Based on my reading, and limited understanding of the domain, terms such as "WHIP Session Factory" and "WHIP Session" would be more appropriate for these resources. Section 4 : Protocol Operation The second paragraph describes how to create a session resource using HTTP POST. This is standard HTTP behaviour and should not be respecified here. For example, simply saying, "HTTP POST request is used with application/sdp content to create a WHIP session resource" should be sufficient. It is not clear to me if the response returned is a "status of the request" as per RFC9110 or a representation of the "WHIP Resource" pointed to by the Location header. If it is the latter, then including a Content-Location header would be useful to clarify this. 4.1. ICE and NAT support > If the WHIP resource supports either Trickle ICE or ICE restarts, but not both, > it MUST return a "405 Not Implemented" response for the HTTP PATCH requests that are not supported. "405 Not Implemented" appears to be a mistake. I presume this should say "405 Not Allowed" However, this does not seem like the appropriate status code. Per the text, PATCH is definitely supported. However, the specific request being made with PATCH is not allowed. This seems more like a 400 Bad Request or possibly a 409 conflict. The second case when PATCH is not supported for any purpose seems more suited to returning "405 Not Allowed". 501 Not Implemented is recommended for use when no resource on the origin server supports the method. That does not seem to be the case here. The paragraph that describes using conditional requests, should state that PATCHing a WHIP Resource MUST use conditional requests using a strong eTag and not attempt to respecify how conditional requests work. Also, the "WHIP Resource" should return a 428 to indicate a missing eTag header. The latter part of the paragraph seems to contradict the earlier part of the paragraph by stating that conditional requests are only sometimes required. Perhaps what this paragraph is trying to say is "if the WHIP Endpoint returns an eTag when creating the session, then a client MUST PATCH the session using that eTag in a conditional request header". In general, using status codes 200 and 204 to communicate different application success semantics seems like a stretch of HTTP semantics. e.g. 200 means successful ICE restart but 204 means a successful or partially successful addition of candidates without a restart. PATCH is designed to be used with "PATCH aware" media types and application/trickle-ice-sdpfrag does explicitly call out how it behaves with the PATCH method. One alternative would be to create an "envelope type" PATCH aware media type that contains the application/trickle-ice-sdpfrag content but also has place to communicate the kind of application semantics that are currently being tunneled via HTTP status codes. For example, what would happen if it became desirable to communicate to a client which candidates were not able to resolve a connection address? The current choice to use 204 means it is not possible to return information in the response body to communicate this failure. Section 4.2 "406 Not Acceptable" error response should not be used to indicate bad request content. A 400 status code is more appropriate. Section 4.3 > WHIP clients SHALL support HTTP redirection via the "307 Temporary Redirect" > In case of high load, the WHIP endpoints MAY return a "503 Service Unavailable" response indicating that the server is currently unable to handle the request due to a temporary overload or scheduled maintenance, which will likely be alleviated after some delay. The WHIP endpoint might send a Retry-After header field indicating the minimum time that the user agent ought to wait before making a follow-up request. This content seems appropriate for a best practices or implementers guide and not normative text. What is being described is standard HTTP behaviour and does not need repeating. The danger of repeating subsets of HTTP language in normative text is that it raises questions such as "Can I return 301 instead of 307?" "Should the client expect to get a 502 instead of 503?" This document should limit itself to including text that further constraints the use of HTTP. Section 4.5 This section said that WHIP clients MUST implement HTTP Auth using a bearer token, but prescribes no constraints on what kind of bearer token. Considering this open ended security definition, what is the benefit of preventing the use of other HTTP Auth schemes such as the ones registered here https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml ? Nits: Section 2: Terminology It would be good to add definitions of ICE, SDP, DTLS to the terminology section. SDP is defined in the introduction but ICE and DTLS isn't . The definition of WHIP Endpoint and WHIP Endpoint URL seems redundant. Same for WHIP Resource. All HTTP resources have URLs. There isn't usually a need to describe the URL independently. Section 4.1: ICE and NAT Support A reference for ICE ufrag/pwd pair would be helpful. References to RFC 9110 section 3.1 should be to section 13.1 Section 4.7 I believe "and the URI for the HTTP resource" should be "and the URL for the HTTP resource".
Received on Sunday, 1 October 2023 19:48:36 UTC