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

[Darrel Miller <[e-mail]>]

Hi Sergio,

> the term Endpoint is quite established  in rest APIs and I would prefer to keep it.

I agree that is used extensively. In my experience I have seen it inconsistently. This article tries to explain what it is https://www.cloudflare.com/en-ca/learning/security/api/what-is-api-endpoint/ but what it seems to be describing is a resource.  As I don't have a significantly better suggestion, I withdraw my objection.

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

That seems like a reasonable compromise. I would ask that you emphasise where this document defines further constraints on what RFC 9110 specifies.

>  Could it be possible to use 422 Unprocessable Content instead?​

If you believe that returning a 422 will allow clients to do something different than they would if they received a 400, then I have no objection to using 422.  400 is a generic catch all for any 4XX class error, so there is nothing semantically incorrect with using 400, it just isn't very specific about the problem. If you want to attach some specific semantics to 422 within the WHIP protocol, that can be used by the client, then that is reasonable.
​
>  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?

I'm concerned that I can't tell whether I just need to use an Etag to make a conditional request, as I would with any other HTTP API.  I have to go through the effort of reconciling the words in this paragraph with those in RFC9110 to know if this protocol layers on new constraints.  This is made more difficult by the fact that it has four MUSTs in the paragraph and finishes with a statement that invalidates the MUSTs.

I have attempted to reword the paragraph with this:
DM>  WHIP sessions that support ICE restarts MUST return a strong ETag as per [RFC9110] Section 8.8.3 in a successful response to a POST to the WHIP Endpoint.
DM> Successful PATCH requests to WHIP sessions that trigger a restart MUST return an updated ETag.

This rewording made me ask the question, what happens when a PATCH doesn't trigger a restart. Then I read your response to my last concern and I realized that the protocol is suggesting that a PATCH that doesn't trigger a restart returns a 204 and does not include a Etag.  I had not realized this on first read but it is directly related to my comment that layering addition semantics onto 200 vs 204 is not advisable.

I would recommend clarifying the unusual handling of Etags with an additional sentence like :
DM> Successful PATCH requests to WHIP sessions that do not trigger a restart MUST not return an updated ETag.

Regarding the use of 200 vs 204 to convey two distinct server behaviors, I personally do not like it and I think people will find it confusing.  However, I cannot point you to a specification that says you must not do this.  I would highly recommend emphasizing these two distinct interactions. Maybe you could create two headings, one for "WHIP Session update with restart" and one for "WHIP Session update without restart".
I don't think I really understand the scenario well enough to suggest solutions, but I would ask why not use a weak validator that conceptually corresponds to the session identifier and therefore could be returned unchanged when doing a PATCH that doesn't do a restart. That way a PATCH could always return a 200. Or return whatever the client asks for using the prefer header.

> 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.
Ok.  But I would suggest you consider what the client is going to do differently when it processes SDP parsing errors vs SDP semantic errors. Are there actually two code paths?

> 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?
That seems like a reasonable compromise.


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.

There is really nothing interoperable about requiring the bearer authentication scheme, other than the keyword in the auth header.  The text states that the auth scheme MUST be "bearer". That does preclude clients adding more authentication methods.


Regards,

Darrel

________________________________
From: Sergio Garcia Murillo <[e-mail]>
Sent: Tuesday, October 3, 2023 6:21 AM
To: Darrel Miller <[e-mail]>
Cc: [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Subject: 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 <[e-mail]<mailto:[e-mail]>> 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