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

[Sergio Garcia Murillo <[e-mail]>]

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=is%3Aissue+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 AM Russ Housley via Datatracker <
[e-mail]> 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/7cb69f00a6d7078545c6497643d17e49c60ec474

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/51ffb5e26958518e449e9e7bec6fd50da7bb52fc


>
> Section 5: Please reference RFC 4086 for guidance on generation of random
> numbers.
>
Added
https://github.com/wish-wg/webrtc-http-ingest-protocol/commit/70a101238f0adf04b430808c344a50eebe865367


> 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/0d26798db60bc3f756f29b04da47804af6325fbe

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/b752a47eb4b50028e1425517c23c05d63f917f8b


>
> 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/90badef02ba1bd03350a324a553fa9542c5a4296


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