Last Call Review of draft-ietf-wish-whip-09
review-ietf-wish-whip-09-httpdir-lc-miller-2023-10-01-00

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".