Re: [Wish] Httpdir last call review of draft-ietf-wish-whip-09

Hi Darrel,

Thank you very much for your review and comments. I will try to answer some
of them inline, but probably will have to start a new thread with some of
them as I expect quite a bit of discussion before getting to an agreement.


On Sun, Oct 1, 2023 at 9:48 PM Darrel Miller via Datatracker <
noreply@ietf.org> wrote:

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

While the "WHIP Endpoint" is acting as a WHIP session/resource factory, the
term Endpoint is quite established  in rest APIs and I would prefer to keep
it.

I understand the concern about the WHIP resource, and I wouldn't mind
changing it to WHIP session to avoid the naming collision with HTTP
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.
>
>
I have received comments in the opposite direction in the past about not
being described enough with how the protocol operates. Also some of these
texts came from early implementations doing crazy things for the HTTP POST

Given that there is no normative language in the paragraph would you be ok
to keep it but adding a reference to the proper specification of how to
perform an HTTP POST correctly?



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

I have not found the definition of the "status of the request" anywhere,
but the SDP answer is definitely not a representation of the WHIP
Resource/Session, so I would say it is the former.



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

Changing the 501 by a 405 makes sense.  In case of the server supports
PATCH for certain ICE operations, but not for others, sending a 400 bad
request doesn't seem appropriate:

> The 400 (Bad Request) status code indicates that the server cannot or
will not process the request due to something that is perceived to be a
client error (e.g., malformed request syntax, invalid request message
framing, or deceptive request routing).

Neither 409, as the client can't really solve the conflict (as there is no
conflict). Could it be possible to use 422 Unprocessable Content instead?


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


I am concerned that we won't have a compliant implementation doing etags
correctly if we don't describe the process in detail here.
Which parts of the paragraphs are you more concerned about, and is any of
the mandatory text going against the conditional request spec?


>   Also, the "WHIP
> Resource" should return a 428 to indicate a missing eTag header.


Will add it, thank you!


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

Yes, but also mandate that the whip server must use conditional requests if
it supports ice restarts.


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

Not sure if I understand this in full. But let me add some clarifications:
- There is no possible failure when adding remote candidates as it just
means that the ICE agent will start performing a connectivity check but
does not change the status of the ICE agent. It is perfectly fine for those
ice candidates checks to fail. Moreover, an initial check doesn't preclude
a failure later on.
- And ICE restart changes the state of the ICE agent, and a new
username/frag must be communicated back to the client

Due to that we use 200 ok for ice restarts (with body) and 204 for trickle
ice (without body).


>
> Section 4.2
>
> "406 Not Acceptable" error response should not be used to indicate bad
> request
> content. A 400 status code is more appropriate.
>

I would prefer to keep 400 for sdp parsing errors, could we use 422
Unprocessable Content instead? The SDP is valid, but the semantics aren't.


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

Would it be fine to remove the normative language on the second part and
maybe just state that the "WHIP clients MUST support HTTP redirection" and
referencing the http specs for the proper error codes?


> 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 ?
>

Interoperability. The implementators of the WHIP clients are not going to
be the users of them, and they should be able to interoperate with any WHIP
service., so I need to specify at least a common authentication method.
Nothing precludes WHIP clients to add more authentication methods if they
want.


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

This was raised by other reviews too, I have a tentative change here:
https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/130/files

Would this be better to you?


> 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
>
> Will add them


> Section 4.7
> I believe "and the URI for the HTTP resource" should be "and the URL for
> the
> HTTP resource".
>

Will change it.

Best regards
Sergio