Re: [Insipid] Genart last call review of draft-ietf-insipid-logme-marking-11

["Dawes, Peter, Vodafone Group" <[e-mail]>]

Hello Francesca, 
Thanks a lot for reviewing the draft, our responses in-line below. We have submitted revision -12 (https://datatracker.ietf.org/doc/draft-ietf-insipid-logme-marking/) which incorporates these responses and we believe resolves all of the issues. 

Best regards,
Peter and Arun

> -----Original Message-----
> From: Francesca Palombini <[e-mail]>
> Sent: 06 July 2018 12:30
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; 
> [e-mail]
> Subject: Genart last call review of 
> draft-ietf-insipid-logme-marking-11
> 
> Reviewer: Francesca Palombini
> Review result: Almost Ready
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area 
> Review Team (Gen-ART) reviews all IETF documents being processed by 
> the IESG for the IETF Chair.  Please treat these comments just like 
> any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-insipid-logme-marking-11
> Reviewer: Francesca Palombini
> Review Date: 2018-07-06
> IETF LC End Date: 2018-07-10
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary: This draft is on the right track but has open issues, 
> described in the review.
> 
> Major issues: -
> 
> Minor issues: The following are, in order of importance, the issues I have.
> Following, I tried to point out the specific details in the doc, 
> which, when specified, might refer to one of those more general issues.
> 
> *A) Although the scenarios shown (4.1.2.x and 5.1.x) are very 
> descriptive and useful, it is quite difficult to extrapolate from the 
> examples what is the required behavior of UAs or proxies supporting 
> the "log me" marking. For example, Section 4.5.2.5 really gives 
> requirements/rules on Proxy 1 (from the originating Network), although 
> from the way the document is structured, at first glance one could 
> think the section is about terminating network requirements. Some 
> rules are given in Section 4.2 and 4.3, but they seem to cover only 
> the stateless interaction. I think some additional text (possibly 
> introducing stateful processing, i.e. the first paragraph of 4.5.2 at 
> the same
> time) with rules for stateful processing is missing and could be added there.

We added explanation text at the beginning of Section 4.5.2 "Stateful processing" to fully describe the reasons for the behaviour of stateful intermediaries. The new text states which entities are impacted by each stateful processing case and includes a reference to the Section that illustrates the case.

> 
> *B) It is said in the doc that UAs or intermediaries MAY mark dialogs 
> that are "related" to the originally marked dialog. A pointer to what 
> dialogs are related to each other would be useful: from the doc, the 
> example is REFER relates to INVITE, and INVITE relates to REFER, any 
> else? is there a section in a specification that could be referenced, 
> or a "rule" given (for example dependent on local-UUID value, or other)?

In section 3.7. "Marking Related Dialogs" added text that describes a "related dialog" and includes a reference to RFC 7989 section 6

> 
> *C) About error cases (section 5) I have several concerns:
> 
>     - I don't see anywhere that those two described (marker missing from a
>     previously marked dialog and marker appearing mid dialog) are the
>     comprehensive list of error cases. Is that correct? What if a marker
>     disappear and then reappears? Is the intermediary supposed to keep 
> logging
>     (because the marker did not appear mid-dialog) or not?

Added new section 5. "Error Cases" with a summary list of all error types and a new section 6. "Non-Error Cases" to describe exceptions. All errors can be considered as either a missing marker or a marker that appears when it should not, which was already described but the summary list adds the case you mention and also a marker missing from a message retransmission. 
> 
>     - The document explains that detecting error cases is difficult, and only
>     possible for stateful intermediaries. I'd like to see explicit requirements
>     for stateful intermediaries to be able to detect errors, and that such
>     requirements cover the comprehensive list of error cases described above.
> 
>     - I seem to understand that the only consequence of an "error" case (for
>     the only 2 cases listed so far) is "stop logging and stop marking". Is that
>     correct? What about previously logged messages in the dialog? 
> Should those
>     be kept? Deleted? It is not specified. This could be part of some security
>     considerations.

Also, we restructured the error cases section 5. to clearly spell out how errors are detected and then added a new  subsection 4.6 "error handling" to the end of the SIP entity behaviour section to define the error handling requirements for SIP entities. The text in 4.6 was previously in the error cases descriptions in section 5 but fits better in Section 4 because it specifies SIP entity behaviour.

The new Section 4.6 "Error Handling" subsection of SIP entity behaviour also has new text stating that previously logged messages are retained.

> 
> *D) I don't think Section 4.5.2.1.1 is at the right place. I think 
> such a section would fit better in the error handling section (Section 
> 5) to explain why some "missing markers" are errors while others 
> aren't. Also, it actually shows an example where log in marking is in 
> fact supported by the originating UA, so it should definitely not be 
> under section 4.5.2.1, at worse it should be under
> 4.5.2.2 (that is probably just an overlook).

Section 4.5.2.1.1 is moved to section 6. "Non-Error Cases" as it is normal (non-error) behaviour.

> 
> *E) Terminology section missing: the drafts including the terminology 
> are referenced in the document, but it would have been good to have a 
> section in the beginning (sub-section of introduction is common) 
> mentioning which ones these are. At least RFC7989, possibly also RFC8123.

So far we didn't make a change. We believe that our responses to your other comments mean that all cases in the draft where new terminology appears now has a reference to explain that terminology, e.g. test case identifier, dialog-initiating request.

> 
> *F) I would suggest a reformulation of the rules in 4.2 and 4.3 to 
> have a
> separation: originating endpoint rules, then terminating endpoint 
> rules for
> 4.2 and stateless intermediary, stateful intermediary in 4.3. This is 
> just a suggestion for better readability in my opinion, feel free to disregard.
> 

The lists of rules in sections 4.2 and 4.3 are now divided into originating or terminating and user agent or intermediary as per the comment.

> Detailed Comments:
> 
>    This document defines a new header field parameter "logme" for the
>    "Session-ID" header field.
> 
> Good with a reference for "Session-ID" header field, at the end of 
> this sentence. ---

In Section 1 "Introduction", the reference to RFC 7989 moved to end of first sentence as commented.

> 
>    as a "log me" parameter since the session identifier is typically
>    passed through SIP B2BUAs or other intermediaries, as per the
>    Session-ID requirement REQ3 in ([RFC7206]).  The "logme" parameter
> 
> For B2BUA, see point E.
> ---

To explain the term B2BUA, when it first appears in section 3.1 we added a reference to RFC 7092 https://tools.ietf.org/html/rfc7092 (A Taxonomy of Session Initiation Protocol (SIP) Back-to-Back User Agents) .

> 
>    Marking starts with a dialog-initiating request and continues for the
>    lifetime of the dialog, and applies to each request and response in
>    that dialog.
> 
> Can a ref be added for which are the "dialog-initiating request"? Or 
> put it in terminology (point E). ---

In 3.2.  "Starting and Stopping Logging" a reference is added to section 12.1 of RFC 3261 to describe a dialog-initiating request.

> 
>    A user agent or intermediary adds a "log me" marker in a request or
>    response in two cases: firstly because it is configured to do so, or
>    secondly because it has detected that a dialog is being "log me"
>    marked, causing it to maintain state to ensure that all requests and
>    responses in the dialog are similarly "log me" marked.  Once the 
> "log
> 
> Can we change to: "A user agent or intermediary adds a "log me" marker 
> in an unmarked request or response in two cases: firstly because it is 
> configured to do so, or ...". ---

In 3.2. "Starting and Stopping Logging" changed "adds a 'log me' marker in a request..." to "adds a 'log me' marker in an unmarked request..." as commented.

> 
> Section 3.2: Can you add some text about an intermediary that does not 
> support marking? It seems hinted from the following examples that it 
> would just remove it. Can it be made explicit in this section? ---

Added a new subsection of section 3.4.  "Passing the Marker". Section 3.4.3.  "Across a Non-Supporting SIP Intermediary" explains that "log me" might be forwarded or dropped by a non-supporting intermediary. 

> 
>    If a request or response is "log me" marked, then all re-
>    transmissions of the request or response MUST be similarly "log me"
>    marked.  Likewise, re-transmissions of a request or response that was
>    not "log me" marked MUST NOT be "log me" marked.
> 
> What if the MUST NOT is not complied with? What should the endpoint or 
> intermediary do in that case? I assume error, but it should be 
> specified. (See point C.2 above) ---

Missing log me marker in retransmissions is added to the list of error cases in section 5. "Error Cases".

> 
>    marking extends to the lifetime of the dialog.  In addition, as
>    discussed in Section 3.7, "log me" marked dialogs that create related
>    dialogs (REFER) may transfer the marking to the related dialogs.  In
>    such cases, the entire "session", identified by the Session-ID
>    header, is "log me" marked.
> 
> See point B.
> ---

Covered by added text that describes a "related dialog" and includes a reference to RFC 7989 section 6 in response to point B

> 
>    The local Universally Unique Identifier (UUID) portion of Session-ID
>    [RFC7989] in the initial SIP request of a dialog is used as a random
>    test case identifier.  This provides the ability to collate all
> 
> Although it is pretty much clear what it means, it sounds like test 
> case identifier is defined somewhere? Ref? Terminology? (point E) ---

In order to define a test case identifier, added a reference to REQ5 in RFC 8123 at the end of 3.3. "Identifying Test Cases".

> 
>    creation and ends when the dialog ends.  However, dialogs related to
>    a "log me" marked dialog MAY also be "log me" marked.  An example 
> is
> 
> I guess that MAY depend on a policy in place. Can that be explicit?
> ---

So far we didn't make a change. We don't think this is defined by an individual policy. SIP devices that are compliant to RFC 7989 will typically add Session-ID in related dialogs that are considered to be part of the same communication session.

> 
>    transfer.  Alice's terminal inserts a "logme" marker in the REFER
>    request and 200 OK responses to NOTIFY requests in dialog2.  Both
> 
> See point B.
> ---

The reference to RFC 7989 added to Section 3.7 points to an explanation of signalling and SIP messages for related dialogs.

> 
>    dialog3 is not logged by Alice's terminal, however the test case
>    identifier ab30317f1a784dc48ff824d0d3715d86 is also the test case
>    identifier local-uuid) in INVITE F5.  Also, the test case identifier
>    of dialog2, which is logged by Alice's terminal, can be linked to
>    dialog1 and dialog3 because the remote-uuid component of dialog2 is
>    the test case identifier ab30317f1a784dc48ff824d0d3715d86.
> 
> This is more for my own understanding: is this the case all the time 
> in SIP? Or is this what this doc specify? If it is the latter, then it 
> should be probably clarified. Maybe it would be good to clarify 
> anyway. ---

The behaviour in section 3.7. "Marking Related Dialogs" is how SIP devices that support Session-ID will behave because they will follow Section 6 of RFC 7989.

> 
> Section 4.2 and 4.3: Don't see any rule for related dialogs. I 
> understand that is optional, but I think it should be here (stated as 
> optional). (see point B)
> ---

Added four new bullets in the lists of rules in 4.2 and 4.3 for related SIP dialogs. The four bullets cover originating or terminating UAs and originating or terminating intermediaries.

> 
>    The retention time period for logged messages should be the minimum
>    needed for each particular troubleshooting or testing case.  The
>    retention period is configured based on the data retention policies
>    of service providers and enterprises.
> 
> should -> SHOULD?
> ---

Capitalized SHOULD in 7.4.5 "Retaining Logs"

> 
>    Consent to turn on "log me" marking for a given session must be
>    provided by the end user or by the network administrator.  It is
> 
> may -> MAY?
> ---

Capitalized MUST in 7.4.6.  "User Control of Logging".

> 
> Nits/editorial comments:
> 
>    when this dialog and it's related dialogs end.  It is considered an
> 
> it's -> its
> ---

Corrected it's to its in section 3.2 "Starting and Stopping Logging" text "when this dialog and its related dialogs end".

> 
>    A SIP intermediary MUST copy the "log me" marker into forked
>    requests.
> 
> Can a ref be added to the spec describing forked requests?
> ---

In 3.8.  "Forked Requests", added a reference to RFC 3261 sections 4, 16.6.

> 
>    o  The originating user agent itself logs signaling.
> 
> Change to: The originating user agent itself logs marked signaling requests.
> Additionally, see point F. ---

In 4.2 and 4.3, changed "requests" to "marked requests" in four of the rules bullets

> 
>    echoed by a terminating UA.  SIP intermediaries on the signaling path
>    between these UAs that do not perform the tasks described in
>    Section 4.5.2 can simply log any request or response that contains a
>    "log me" marker in a stateless manner, if it is allowed per local
>    policy.
> 
> That can should be a MUST, I think.
> ---

Changed "can" to "MUST" in 4.5. "Stateless SIP Intermediaries".

> 
> Section 4.5.2.1:
> 
>    F12 - Proxy 1 receives ACK with with no "log me" marker.  It doesn't
>    consider this as an error since it is configured to "log me" mark on
>    behalf of Bob's UA.
> 
> Bob -> Alice
> ---

Corrected Bob to Alice in message F12 in Figure 3.

> 
> 4.5.2.3: Typo in the figure: after F2, missing characters on the lines 
> of Proxy
> 2 and Bob. Same just before F19. ---
> 
>    requests and responses entering network B.  However, Proxy 2 supports
>    maintains the marking state of the dialog and "log me" marks 
> requests
> 
> remove supports (or maintains)
> ---

Corrected the spacing in two places in Figure 4. Also changed "supports maintains" to "supports maintaining".

> 
> 4.5.2.4: Typo in the figure: wrongly aligned characters under F9.
> ---

Corrected alignment in Figure 6.

> 
> 4.5.2.4:
> 
>    F2 - Proxy 2 removes "log me" marker in the INVITE request F2 before
>    forwarding it as F7.
> 
> F7 -> F4. Also, maybe add "The same applies to F13 and F19", to be 
> consistent.
> ---

In Figure 6, corrected F7 to F4 and added text about F13, F19.

> 
> 4.5.2.5: Typo in the figure: wrongly aligned characters under F9.
> ---

Corrected character alignment in Figure 7.

> 
> 4.5.2.5:
> 
>    F2 - Proxy 2 removes "log me" marker in the INVITE request F2 before
>    forwarding it as F7.
> 
> F7 -> F4. Also, maybe add "The same applies to F13 and F19", to be 
> consistent.
> ---

In Figure 7, corrected F7 to F4 and added text about F13, F19.

> 
> 7.4.4: "Log me" in the beginning of the paragraph, but 7.4.5 second 
> paragraph starts with "log me". Which one is it? (I personally have no 
> preference). Same for "log me" marking and "log me" Marking (in 7.4.5)
> 
> ---

"Log me" is capitalized when it is the start of the sentence so we corrected "log me" to "Log me" at the beginning of paragraph 2 in 7.4.5 "Retaining Logs".

> (please keep my address in the to or cc in responses to this review, 
> for easier
> tracking)
> 
> Best regards,
> Francesca