From sergio.garcia.murillo@gmail.com Fri Sep 29 05:47:51 2023 Return-Path: X-Original-To: wish@ietfa.amsl.com Delivered-To: wish@ietfa.amsl.com Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 1ABDFC14CE31; Fri, 29 Sep 2023 05:47:51 -0700 (PDT) X-Virus-Scanned: amavisd-new at amsl.com X-Spam-Flag: NO X-Spam-Score: -7.107 X-Spam-Level: X-Spam-Status: No, score=-7.107 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_HI=-5, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01] autolearn=ham autolearn_force=no Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dXKK87jMJ6xk; Fri, 29 Sep 2023 05:47:50 -0700 (PDT) Received: from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com [IPv6:2a00:1450:4864:20::52d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id F29E0C14CF1B; Fri, 29 Sep 2023 05:47:41 -0700 (PDT) Received: by mail-ed1-x52d.google.com with SMTP id 4fb4d7f45d1cf-533f193fc8dso13457651a12.2; Fri, 29 Sep 2023 05:47:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695991660; x=1696596460; darn=ietf.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=YKT/Yw6ZmFA3tVR9jTH0UgRVHUScxNzLe8JdmF0FIIo=; b=CACbVKXxrZ7PcSXew1EUL/XRVq4CuQ9EXW/GyzgDLwOhBxcz5N0kgDWKhmx7l927Z9 7BB0l6jilAB2ifv0SwJ/c1+Nkhi79TD/Vl9R6opoO26HIieAcXjM67fK2HSl1Idelsii I4HLxEXq9Wfn6d5U5TKCDTPglu501j986HkkP8/eMgIMKFQuyGlqiRTgsSn9aP49gsa1 Y1jg31QwlZO/5tZCmgGbB8fERiAsuiTfeMEcXQ8cFu9HzI02cMkjuF4PCbNAzKml63/d h+mv14zWpYDX/5skMrGIzWnKqCBtOaRCAo0ebkUcQXQu/0s68593A5j+ymKmWQHjNsap iFjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695991660; x=1696596460; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=YKT/Yw6ZmFA3tVR9jTH0UgRVHUScxNzLe8JdmF0FIIo=; b=F6TmCIFJYs/AEbdX2gjwhqKzaGG/WdMHP2N3GZStCYvmY0KxTGYS6spLOUQbMiBJ7k 4FOP8JdMmSQU9F5YZs3r7AtcKGOkHAmYld0U7Lcz+a38ANtYQAluGuNfqNzCR3eGgSOG ZaIdgWDSdsKZTJhqM+D8R0Y4j9enwngdQhC5Lwq7iCC4KryiydmOdUaf4mBqknLcL0CA YeMWU9Y7+Y/q0uJZPDnpgAuf6rN9n7DTPeFlCvFkE5uKmo7LUZHWuXZhgfzCzs0t1860 qvJDXnrw32VEEtsXZn0lqlPHUaFj/u9pBIDMfwRvlD8VAXjZCxRUbZil6jEF/Rnj3OaA K9KQ== X-Gm-Message-State: AOJu0Yzr6oPCmoHjrdgyu2W91WxevTxUap0ElSk8zwoHQWU6vpKCKQ/4 2w4W+sRqJ94CZKp9P5K/CEX6rD8r5ydX+YDkEe+GvPRNth8= X-Google-Smtp-Source: AGHT+IEPUL9GWEbJZOWbmmnWPq+afSmUQk5fdhR87GYqYdjOxGF+JHmPcAnqfsCx6IV340W2zrcdaS1UnwZy+lxZv1w= X-Received: by 2002:aa7:d911:0:b0:531:1f3b:cb36 with SMTP id a17-20020aa7d911000000b005311f3bcb36mr3337901edr.9.1695991659512; Fri, 29 Sep 2023 05:47:39 -0700 (PDT) MIME-Version: 1.0 References: <169092762522.31510.3211690504037589021@ietfa.amsl.com> In-Reply-To: <169092762522.31510.3211690504037589021@ietfa.amsl.com> From: Sergio Garcia Murillo Date: Fri, 29 Sep 2023 14:47:27 +0200 Message-ID: To: Russ Housley Cc: secdir@ietf.org, draft-ietf-wish-whip.all@ietf.org, last-call@ietf.org, wish@ietf.org Content-Type: multipart/alternative; boundary="0000000000006847a306067ed401" Archived-At: Subject: Re: [Wish] Secdir last call review of draft-ietf-wish-whip-09 X-BeenThere: wish@ietf.org X-Mailman-Version: 2.1.39 Precedence: list List-Id: WebRTC Ingest Signaling over HTTPS List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Sep 2023 12:47:51 -0000 --0000000000006847a306067ed401 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Russ Thank you for your review and feedback! I have created issues for the bullet points below and either incorporated a fix to the spec or have a PR ready for review which solves them: https://github.com/wish-wg/webrtc-http-ingest-protocol/issues?q=3Dis%3Aissu= e+label%3Asecdir-review+ I will merge the pending PRs and publish a new draft during next week if no other concerns are raised, Find more detailed info for each topic inline below: On Wed, Aug 2, 2023 at 12:08=E2=80=AFAM Russ Housley via Datatracker < noreply@ietf.org> wrote: > Reviewer: Russ Housley > Review result: Has Issues > > I reviewed this document as part of the Security Directorate's ongoing > effort to review all IETF documents being processed by the IESG. These > comments were written primarily for the benefit of the Security Area > Directors. Document authors, document editors, and WG chairs should > treat these comments just like any other IETF Last Call comments. > > Document: draft-ietf-wish-whip-09 > Reviewer: Russ Housley > Review Date: 2023-07-29 > IETF LC End Date: 2023-08-08 > IESG Telechat date: Unknown > > Summary: Has Issues > > > Major Concerns: > > Section 4 says: > > The HTTP POST request will have a content type of "application/sdp" > > It seems to me that this ought to be a MUST statement. Also, what > will happen if the media type is something else? Or, what happens if > the attempt to parse the content as an SDP fails? > I have a PR for review that adds mandatory language for the content type and also explains the error behaviour in case body is an sdp or malformed: https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/128/files > Section 4.1 says: > > The initial offer by the WHIP client MAY be sent after the full ICE > gathering is complete with the full list of ICE candidates, or it MAY > only contain local candidates (or even an empty list of candidates) > as per [RFC8863]. > > I do not understand this paragraph. The client MAY do X OR MAY do Y. > There is no context to tell why a client might want to do either X or Y. > > Added context and on when the client may use each of them and what is the recommended behaviour: https://github.com/wish-wg/webrtc-http-ingest-protocol/pull/129/files > Section 4.2 says: > > In order to reduce the complexity of implementing WHIP in both > clients and Media Servers, WHIP imposes the following restrictions > regarding WebRTC usage: > > However, there is no clear formatting to determine where the list of > restrictions ends. Maybe a list of bullets would be more clear. > > I formatted the restrictions so they are in subsections now: https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/7cb69f00a6d70= 78545c6497643d17e49c60ec474 Much clearer now, thanks for that. Section 5 says: > > * HTTP security: Section 11 of [RFC9112], Section 17 of [RFC9110], > etc. > > The use of "etc" is not going to help an implementer. Please be complete= . > Removed etc as I didn't really found any other reference that could be applicable: https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/51ffb5e269585= 18e449e9e7bec6fd50da7bb52fc > > Section 5: Please reference RFC 4086 for guidance on generation of random > numbers. > Added https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/70a101238f0ad= f04b430808c344a50eebe865367 > Minor Concerns: > > Please merge the definition in Section 2 and the overview in Section 3. > Figure 1 really is needed to understand the definitions, but the > definitions come first. > > The figures are not referenced from body of the document. It is best to > include a reference in the body that offers some description of what the > reader is expected to learn from the figure. When I as a Security AD, the > other Security AD was blind. The text-to-audio system that he used was > surprisingly good, but it could not handle ASCII art. The discussion of > the figures was vital to him being able to understand a document. > Tried to do a rewrite of both sections and add the figure references: https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/0d26798db60bc= 3f756f29b04da47804af6325fbe Would be great if you could take a look at it, I will double-check the actual markdown for figure reference when the new draft is released. > > The following paragraph appears twice in Section 4: > > The WHIP endpoints MUST return an "405 Method Not Allowed" response > for any HTTP GET, HEAD or PUT requests on the endpoint URL in order > to reserve its usage for future versions of this protocol > specification. > > Section 4.2 says: > > ... sections as per [RFC8858] i. > > I do not understand this reference. > Typo, solved in here: https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/b752a47eb4b50= 028e1425517c23c05d63f917f8b > > Section 4.2 says: > > This version of the specification only supports, at most, a single > audio and video MediaStreamTrack in a single MediaStream as defined > in [[!RFC8830]] ... > > Does it ever make sense for there to be zero audio and video tracks? > > I do not understand this reference. I suspect it is malformed markdown. > > Fixed in https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/90badef02ba1b= d03350a324a553fa9542c5a4296 > > Nits: > > Section 4: s/non graceful/non-graceful/ > > Section 4: s/mime type/media type/ > > Section 5: s/[RFC8446], [RFC8446],/[RFC8446]/ > > Section 5: s/enought/enough/ > > Section 5: s/legit/legitimate/ > > Section 5: s/abalanche/avalanche/ > > Section 5: s/currentlyrunning/currently running/ Good catches! Solved here: https://github.com/wish-wg/webrtc-http-ingest-protocol/issues/127 Best regards Sergio --0000000000006847a306067ed401 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Russ

Thank you for = your review and feedback! I have created issues for the bullet points below= and either incorporated a fix to the spec or have a PR ready for review wh= ich solves them:


I will merge = the pending PRs and publish a new draft during next week if no other concer= ns are raised,

Find=C2=A0more detailed info fo= r each topic inline below:

On Wed, Aug 2, 2023 at 12:08=E2= =80=AFAM Russ Housley via Datatracker <noreply@ietf.org> wrote:
Reviewer: Russ Housley
Review result: Has Issues

I reviewed this document as part of the Security Directorate's ongoing<= br> effort to review all IETF documents being processed by the IESG.=C2=A0 Thes= e
comments were written primarily for the benefit of the Security Area
Directors.=C2=A0 Document authors, document editors, and WG chairs should treat these comments just like any other IETF Last Call comments.

Document: draft-ietf-wish-whip-09
Reviewer: Russ Housley
Review Date: 2023-07-29
IETF LC End Date: 2023-08-08
IESG Telechat date: Unknown

Summary: Has Issues


Major Concerns:

Section 4 says:

=C2=A0 =C2=A0The HTTP POST request will have a content type of "applic= ation/sdp"

It seems to me that this ought to be a MUST statement.=C2=A0 Also, what
will happen if the media type is something else?=C2=A0 Or, what happens if<= br> the attempt to parse the content as an SDP fails?

=
I have a PR for review that adds mandatory language for the cont= ent type and also explains the error behaviour in case body is an sdp or ma= lformed:




Section 4.1 says:

=C2=A0 =C2=A0The initial offer by the WHIP client MAY be sent after the ful= l ICE
=C2=A0 =C2=A0gathering is complete with the full list of ICE candidates, or= it MAY
=C2=A0 =C2=A0only contain local candidates (or even an empty list of candid= ates)
=C2=A0 =C2=A0as per [RFC8863].

I do not understand this paragraph.=C2=A0 The client MAY do X OR MAY do Y.<= br> There is no context to tell why a client might want to do either X or Y.

Added context and on when the client m= ay use each of them and what is the recommended behaviour:

=C2=A0
Section 4.2 says:

=C2=A0 =C2=A0In order to reduce the complexity of implementing WHIP in both=
=C2=A0 =C2=A0clients and Media Servers, WHIP imposes the following restrict= ions
=C2=A0 =C2=A0regarding WebRTC usage:

However, there is no clear formatting to determine where the list of
restrictions ends.=C2=A0 Maybe a list of bullets would be more clear.


I formatted the restrictions so they a= re in subsections now:

Much clearer now= , thanks=C2=A0for that.

Section 5 says:

=C2=A0 =C2=A0*=C2=A0 HTTP security: Section 11 of [RFC9112], Section 17 of = [RFC9110],
=C2=A0 =C2=A0 =C2=A0 etc.

The use of "etc" is not going to help an implementer.=C2=A0 Pleas= e be complete.
=C2=A0
Removed etc as I didn&= #39;t really found any other reference that could be applicable:
=C2=A0

Section 5: Please reference RFC 4086 for guidance on generation of random numbers.
Added
=C2=A0
Minor Concerns:

Please merge the definition in Section 2 and the overview in Section 3.
Figure 1 really is needed to understand the definitions, but the
definitions come first.

The figures are not referenced from body of the document. It is best to
include a reference in the body that offers some description of what the reader is expected to learn from the figure. When I as a Security AD, the other Security AD was blind. The text-to-audio system that he used was
surprisingly good, but it could not handle ASCII art. The discussion of
the figures was vital to him being able to understand a document.

Tried to do a rewrite of both sections and add t= he figure references:

Would be great if= you could take a look at it, I=C2=A0will double-check the actual markdown = for figure reference when the new draft is released.
=C2=A0
=

The following paragraph appears twice in Section 4:

=C2=A0 =C2=A0The WHIP endpoints MUST return an "405 Method Not Allowed= " response
=C2=A0 =C2=A0for any HTTP GET, HEAD or PUT requests on the endpoint URL in = order
=C2=A0 =C2=A0to reserve its usage for future versions of this protocol
=C2=A0 =C2=A0specification.

Section 4.2 says:

=C2=A0 =C2=A0... sections as per [RFC8858] i.

I do not understand this reference.

Typ= o, solved in here:
=C2=A0

Section 4.2 says:

=C2=A0 =C2=A0This version of the specification only supports, at most, a si= ngle
=C2=A0 =C2=A0audio and video MediaStreamTrack in a single MediaStream as de= fined
=C2=A0 =C2=A0in [[!RFC8830]] ...

Does it ever make sense for there to be zero audio and video tracks?

I do not understand this reference.=C2=A0 I suspect it is malformed markdow= n.


=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Nits:

Section 4: s/non graceful/non-graceful/

Section 4: s/mime type/media type/

Section 5: s/[RFC8446], [RFC8446],/[RFC8446]/

Section 5: s/enought/enough/

Section 5: s/legit/legitimate/

Section 5: s/abalanche/avalanche/

Section 5: s/currentlyrunning/currently running/

Good catches!
Solved here:

=
Best regards
Sergio

--0000000000006847a306067ed401--