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:
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 AM Russ Housley via Datatracker <noreply@xxxxxxxx> 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:
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:
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:
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:
Section 5: Please reference RFC 4086 for guidance on generation of random
numbers.
Added
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:
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:
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.
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
-- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
