Subject: [Wish] Use entity-tags for solving out of order PATCH requests

From: Sergio Garcia Murillo <[email-masked]>
Date: Wed, 2 Mar 2022 17:58:11 +0100

Hi all,

I have prepared a PR for the draft to address the issue of how to protect
WHIP against out of order PATCH requests based on the entity-tags as
discussed at the latest IETF meeting.

You can find the changes here:

https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/24/files?short_path=6ca9ae6#diff-6ca9ae63875a605dbaf6d8e45c08b5945951d62135ae0661065b7fe9ebb04d84

Please, provide any feedback in the github PR so we can merge it and
prepare a new draft before the next meeting.

I had reviewed my notes and minutes from the last meeting and I think there
are no other pending topics as there was some pushback for
the proposed changes, so I will just leave the draft without changes on
those areas.

Anything else that I am missing?

Best regards
Sergio

---

From: Lorenzo Miniero <[email-masked]>
Date: Wed, 2 Mar 2022 18:24:43 +0100

On Wed, 2 Mar 2022 17:58:11 +0100
Sergio Garcia Murillo <[email-masked]> wrote:

> Hi all,
> 
> I have prepared a PR for the draft to address the issue of how to
> protect WHIP against out of order PATCH requests based on the
> entity-tags as discussed at the latest IETF meeting.
> 
> I had reviewed my notes and minutes from the last meeting and I think
> there are no other pending topics as there was some pushback for
> the proposed changes, so I will just leave the draft without changes
> on those areas.
> 

Hi Sergio,

I think the changes look fine, I just have a couple of comments:

	* What error should be returned in case a PATCH to trigger an
	  ICE restart does NOT contain the "*" If-Match value? The
	  draft says it's a MUST, but doesn't elaborate on the
	  consequences if that doesn't happen. Intuitively a 412 could
	  be returned as well, but the PATCH for ICE restart could
	  actually contains the last valid ETag, and so in that case a
	  412 sounds "wrong".

	* It's also not clear to me if ETags apply to other requests
	  addressed to a WHIP resource. Thinking of the DELETE to tear
	  down the PeerConnection, for instance, would that need the
	  right ETag to be provided as well? Or would ETags be limited
	  exclusively to PATCH? If so, it might be better to clarify it
	  in the doc to limit the scope.

Thanks,
Lorenzo

-- 
I'm getting older but, unlike whisky, I'm not getting any better
https://twitter.com/elminiero

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 02 Mar 2022 21:37:55 +0100

>> entity-tags as discussed at the latest IETF meeting.

Does that mean that we're now serialising trickle?

Suppose the exchange goes:

  C->S: POST Offer
  S->C: 202 Answer
  C->S: PATCH candidate
  
Now suppose that the client generates a new ICE candidate before it
receives the answer to its latest PATCH request.  Since it doesn't know
the reply's ETag yet, it must buffer the new candidate until it receives
the response, right?

Hopefully I'm misunderstanding something, since serialising trickle
needlessly complicates the implementation of clients.  If I'm not, then
I have no strong objection to requiring this serialisation, but if that's
what you're requiring, it must be made explicit in the draft -- you must
say explicitly that it's not allowed to have multiple candidates in flight.

-- Juliusz

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Thu, 3 Mar 2022 06:45:48 +0100

> say explicitly that it's not allowed to have multiple candidates in flight.

Hi Juliusz,

entity-tags only change on ice restarts, which can only be triggered by the
client. So it is perfectly possible to have multiple ice trickle requests
in flight.

Best regards
Sergio

---

From: Lorenzo Miniero <[email-masked]>
Date: Thu, 3 Mar 2022 10:04:39 +0100

On Thu, 3 Mar 2022 06:45:48 +0100
Sergio Garcia Murillo <[email-masked]> wrote:

> > receives the answer to its latest PATCH request.  Since it doesn't
> > know the reply's ETag yet, it must buffer the new candidate until
> > it receives the response, right?
> >
> > needlessly complicates the implementation of clients.  If I'm not,
> > then I have no strong objection to requiring this serialisation,
> > but if that's what you're requiring, it must be made explicit in
> > the draft -- you must say explicitly that it's not allowed to have
> > multiple candidates in flight. 
> 
> Hi Juliusz,
> 
> entity-tags only change on ice restarts, which can only be triggered
> by the client. So it is perfectly possible to have multiple ice
> trickle requests in flight.
> 

Yes, that's how I understood the feature as well from the document. For
those that may be interested, I wrote a quick implementation of the
feature in both my WHIP client and WHIP server, that might be helpful
for testing purposes. Juliusz, please let me know if you have trouble
with those.

Server: https://github.com/meetecho/simple-whip-server/pull/4
Client: https://github.com/meetecho/simple-whip-client/pull/9

Lorenzo

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 03 Mar 2022 10:13:47 +0100

> client.

Ah, ok, perfect.  Sorry for the confusion.

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Thu, 3 Mar 2022 10:19:40 +0100

Hi Lorenzo

>           actually contains the last valid ETag, and so in that case a

Not sure if ETags are mandated on the client side for non-GET requests, so
while we mandate the If-Match="*" on the whip spec, i think whip
implementations should just accept request without it for ice restarts and
do not return an error.

>           in the doc to limit the scope.

I have the same doubt as before, I don't think it is mandatory from the
HTTP specs to send the etags on the DELETE request, so we should just state
that it is only required for PATCH request.

Best regards
Sergio

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 03 Mar 2022 12:13:51 +0100

>               right ETag to be provided as well?

It seems to me that when a client goes away, it will want to quickly tear
down all streams, with no regard to their ICE generation.

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 03 Mar 2022 12:27:42 +0100

Thanks a lot for your work, Sergio.  I think I got it now.

1. Would you consider adding guidance for server implementers about how to
generate the ETag ?  For example, something like

"The ETag could, for example, be generated by appending the ICE ufrag with
 the ice password, or by using the timestamp of the last ICE restart."

(I'm not very familiar with HTTP, but it is my understanding that a strong
ETag must be unique over the lifetime of a resource.  I'm not quite sure
what a resource is, and I'm not sure what data can be used to guarantee
the properties of the ETag that are required by HTTP.  I'm sure I'm not
the only person who would find such guidance useful.)

2. Could you please spell out explicitly how the server distinguishes
between restarts and candidates?  My understanding is that one says
"If-Match: "*"" while the other carries a strong ETag, but I'd like to see
it spelled out explicitly.

3. What should a server do if it cannot parse a candidate (say, the client
sends an mDNS candidate, and the server doesn't implement mDNS, or the
server is IPv6-only and the client sends an IPv4 candidate).  Should it
return success and ignore the candidate, or should it return an error?  If
so, which one?

4. What should the server do if restarting ICE fails?  Should it ignore
the restart and return the same ETag as before, should it return an error
to the client and continue with the previous ICE generation, or should it
return the error and close the RTP stream (with RTCP BYE)?

-- Juliusz

---

From: Lorenzo Miniero <[email-masked]>
Date: Thu, 3 Mar 2022 12:39:02 +0100

On Thu, 03 Mar 2022 12:27:42 +0100

Hi Juliusz,

chiming in with some responses (from my understanding of the document)
and comments, waiting for Sergio to answer.

> 1. Would you consider adding guidance for server implementers about
> how to generate the ETag ?  For example, something like
> 
> "The ETag could, for example, be generated by appending the ICE ufrag
> with the ice password, or by using the timestamp of the last ICE
> restart."
> 
> (I'm not very familiar with HTTP, but it is my understanding that a
> strong ETag must be unique over the lifetime of a resource.  I'm not
> quite sure what a resource is, and I'm not sure what data can be used
> to guarantee the properties of the ETag that are required by HTTP.
> I'm sure I'm not the only person who would find such guidance useful.)
> 

It's my understanding that any random string would work, so if
concatenating ICE ufrag and pwd (which is what Sergio does in the
draft) works for you, then it's ok. Even if the ETag is supposed to be
unique over the lifetime of a resource, as you say, a WHIP resource has
a short life, since once the PeerConnection is closed it goes away
(further attempts to publish to the same WHIP endpoint will in practice
create a new resource, even if the URI is the same). As such, it should
be easy to enforce those unique IDs in that limited scope.

> "If-Match: "*"" while the other carries a strong ETag, but I'd like
> to see it spelled out explicitly.
> 

The right way to do that would be looking at the payload, I suppose: if
the provided ICE credentials differ from the ones you knew, it's a
restart. In my prototype, for instance, I'm only allowing "*" as a
value if the payload _actually_ contains a restart: not sure if that's
overly zealous, or even plain wrong (maybe 412 makes no sense if the
client is sending "*"?).

> 3. What should a server do if it cannot parse a candidate (say, the
> client sends an mDNS candidate, and the server doesn't implement
> mDNS, or the server is IPv6-only and the client sends an IPv4
> candidate).  Should it return success and ignore the candidate, or
> should it return an error?  If so, which one?
> 

I think it's up to the implementation: in my case I'm just sending a
204 back whether Janus will accept the trickle candidate or not,
without waiting for a response.

> 4. What should the server do if restarting ICE fails?  Should it
> ignore the restart and return the same ETag as before, should it
> return an error to the client and continue with the previous ICE
> generation, or should it return the error and close the RTP stream
> (with RTCP BYE)?
> 

That's a good point: in my implementation, I'm sending a 400 back
(arbitrary choice, some text in the draft would indeed help), but I'm
not actively tearing down the PeerConnection. In theory, if an ICE
restart is required and the ICE restart itself fails, I guess it's
reasonable to assume the PC will eventually go down by itself anyway,
but maybe some guidance in the draft would indeed help.

Lorenzo

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Thu, 3 Mar 2022 13:09:32 +0100

> the only person who would find such guidance useful.)

I am not sure if it is a good idea to put a guide for implementation, as
the entity-tag is an opaque value and I don't want the client side to make
any assumptions about its semantics. A simple incremental number would
work, or a uuid v4, or a timestamps with ms accuracy, or just the full ice
username.

> it spelled out explicitly.

No, the differentiating between the ice restarts and tricke ice has to be
done by checking the ice username/password on the sdp fragment. Now that we
can outdated past tricke ice candidate request it should be just a matter
of checking if the username and fragment corresponds to the stored ones.

> return success and ignore the candidate, or should it return an error?  If

The server should always report a 204 No content if the body has the
correct syntax and return standard 400 Bad request error codes if the body
is invalid. If the candidate is not supported (mDNS or ipv6 for
example)  the server should just silently ignore it. Do you feel that this
should go into the spec?

> return the error and close the RTP stream (with RTCP BYE)?

How would the ice restart fail?

---

From: Sergio Garcia Murillo <[email-masked]>
Date: Mon, 7 Mar 2022 10:42:17 +0100

I have added new changes to try to address the feedback received so far:

https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/24/commits/2b708fd05390bbd683636d681b2536ac8175314d

Please, review it so I can publish a new draft today before the cut
off date.

Best regards
Sergio