Subject: Re: [sip-clf] AD review: draft-ietf-sipclf-format-05

From: Gonzalo Salgueiro <[email-masked]>
Date: Wed, 8 Feb 2012 19:25:36 -0500

Robert - 

Thanks for your thorough and thoughtful review. Below are some inline comments to your feedback. 

I will release the -06 version of the draft once we have settled on all of the below points you have raised.

On Jan 26, 2012, at 6:09 PM, Robert Sparks wrote:

> AD review: draft-ietf-sipclf-format-05
> 
> Summary: This document has open issues to address before progressing to IETF LC
> 
> Major (mostly in document order)
> 
> 1) The version string claims it is hexadecimal encoded. The version for this
> document is expressed by the character A. Does that mean this is version 10
> (base 10)? Are we limited to versions 10 through 15? (we only have one byte
> and section 6 points out that index lines always start with an alphabetical
> character). This description needs to be clarified. While you're looking at
> it, consider discussing whether a given log file can have records with
> different versions in it.

In Section 4.1, the text says that the Version is "hexadecimal encoded."  
This was inherited from the original indexed-ASCII draft and is confusing. 
I will update it to indicate that it is ASCII encoded so we aren't subject 
to the limit of 6 versions. 

For quickly skipping of index records, the first character in the log
file for index records is an alphabetic character (Section 6).  So,
given this, we felt that characters "A" -> "Z" (i.e., 26 versions)
should be enough.  If the working group feels that we need more
versions, one could think of having the set ["a"-"z","A"-"Z"] as
versions, thus giving us 52 version numbers.

Regarding the same log file having records with different record
versions --- since the version number is per CLF record, it does
allow for having a SIP CLF file contain different records.  However,
our expectation is that this would not happen too often.  We can put
a sentence or two saying the above.

> 2) The description of Timestamp and Fractional Seconds is very confusing.
> What does it mean to be represented in big-endian fashion with a most
> significant octet and to be decimal encoded? It's very far from clear that
> you intend this to be an ascii string representing an integer, and it's
> not clear that you intend that it to be prefix-padded with 0s if necessary.
> Did the group consider using ABNF to describe these fields (or the whole
> log record) to avoid this kind of ambiguity?

Good point. The text needs clarifying. I will remove the portion of the text 
that states "Represented in big-endian fashion with most significant octet 
first from zero starting at the left, or high-order, position.  Decimal encoded." 
from the description of both Timestamp and Fractional Seconds fields. 
I will also supplement the text with the following summary text:

The timestamp is represented in the log file as a UTF-8 string
representing the date and time of the request or response
represented as the number of seconds and milliseconds since the
Unix epoch.  The number of milliseconds MUST be separated by a
"." from the number of seconds.  It is not required to zero-pad
the seconds and milliseconds, although if they are zero padded,
a SIP CLF reader MUST be able to interpret them by discarding
leading zeroes.

> 3) The document should be clear about whether the flags field values are
> extensible, and if so, how extension is expected to be managed. How will
> you handle SIP received over DCCP, Websockets, or something we haven't
> heard of yet?

About the only flag that is expected to be extended is the Transport
Flag (byte 4).  Currently, the format draft defines 3 values for
it (UDP, TCP, SCTP).  If Websockets and DCCP become supported transports
in SIP, then yes, this field will need to be extended.  We can do one
of two things for extension:

 1) Have IANA open up a registry for this field
 2) Use the Options part of the SIPCLF record to encode the transport.
  That is, we define byte 4 -- Transport Flag to contain:

  U = UDP
  T = TCP
  S = SCTP
  O = Other

  Then, when a new transport is supported, a log writer MUST put "O"
  for the Transport Flag and MUST create an optional field using
  Vendor-ID 00000000.  Example: when DCCP becomes supported, it will
  be encoded as follows:

  00@00000000,000F,Transport: DCCP

If we don't think that it is reasonable for new transports to be ascribed a 
value of 'Other' in the Transport Flag field, then the IANA registry is the 
more elegant solution. Let me know your thoughts on a way forward here 
since the draft currently has no IANA dependencies.

> 4) Section 4.2 speaks of placing "the actual values for the mandatory
> fields" into the record. It's not clear what "actual values" means. There
> are other sections of the document that talk about transforming (escaping)
> values before putting them in the log record, including dealing with tabs
> and edge conditions like field that has only a question mark in it. The
> concept of encoding and escaping needs to be much clearer throughout the
> document.

I'll edit that text to clarify actual values vs. escaped values. I'll also look 
through the document and ensure the text describing the escaping 
mechanism(s) is clear.

> 5) The document should more explicitly discuss the decision to replace
> tabs with spaces. That decision was based on there being no known use
> of tabs in SIP messages that means something semantically other than
> whitespace. The discussion should point that out, and note that you
> will not be able to use the logged information to reconstruct a
> signature over the logged fields, and that any future (or proprietary)
> SIP header fields that include tabs with semantic meaning (carrying a
> blob perhaps) will lose this meaning while being logged. It should also
> be made brutally explicit whether you want this substitution to happen
> when you are logging the entire message, or a body (that might happen
> to have tabs in it).

We will add a new paragraph that goes something
like this (the new paragraph is added at the end of S4.2 paragraph
"This data MUST appear ... (0x20) prior to being logged."):

 The decision to replace tabs with spaces was based on there
 being no known use of tabs in SIP messages to convey any other
 meaning than whitespace.  Two consequences of the decision to
 replace tab with a space character are: (a) it will become
 impossible to reconstruct a signature over the logged field that
 matches the signature over fields in the original SIP message,
 and (b) any future SIP header fields that include tabs with a
 different semantic meaning than simply signifying whitespace
 will loose this meaning when logged.  And finally, the tabs to
 spaces substitution MUST occur when logging mandatory fields;
 it SHOULD occur when when logging either the entire message or
 simply a SIP body.

> 6) There are a couple of places where you talk about escaping '-' and '?'
> characters. The document should note that this could produce a logged record
> that is not syntactically parsible with a SIP parser (quoted-pair is only
> used in certain places in SIP messages). There should also be some discussion
> about whether a log reader unescapes anything.

It was never the intent for a SIP CLF to be read by a SIP parser as it would be 
entirely nonsensical to it. The belief is that the reader of the SIP CLF would know 
what to do with it. I'll add a statement to the draft indicating this explicitly.

> 7) Why does Tag -01 restrict message bodies to just the listed types, especially
> with the open ended "miscellaneous text content" type category? If there's a real
> restriction here, it needs to be much more clearly stated. Why are you allowing
> any LWS for the separator between the content type header field value and the
> message body itself? Why isn't this simply a space? It is not clear what
> "byte encoded" means for representing binary bodies - do you mean expanding
> it into a hex representation, a uuencode representation, or soemthing else?

The decision to restrict logging of bodies to the specific types listed in the draft 
was a decision made by the WG [1]. The usage of a LWS separator was specified 
as part of that proposal for logging bodies. Making the separator a space 
instead of LWS is fine, in fact, it is preferable. I'll update the draft to reflect that 
change.

I'll also draw a line in the sand and update the text indicating that "byte encoded" 
in this context means that bodies with binary data MUST be base64 encoded for 
logging (since MIME uses base64 encoding).

> 8) There's no description of Tag-02. The text following the line that starts
> the description of the tag speaks of more general things covering both tag-01
> and tag-02.

I'll expand a bit on the text that says "Log entire SIP message".

> 9) When describing repeating tag-01 or tag-02 multiple times to deal with
> large entities, the document should be clear that the fragments get logged
> in order, and how to reassemble the actual entity. It should also say whether
> it's ok to interleave (for example) tag-01 fragements with other optional fields.

This is a great callout. There is text in the description of Tag=01 that needs to 
be updated to clearly state that if your body or the entire SIP
message is > 4096 bytes, then tough luck. We can't chunk this as it would open 
up a can of worms that we don't want to deal with; like indexing the chunks 
and a reassembly scheme. Better not to go there. I will update the text 
to clearly state this point.

> 10) The description of the Length field in section 4.3 is ambiguous. It says
> "This length does not include the header shown in Figure 5." But figure 5 has
> nothing clearly identified as "the header".

I'll update the description of the Length field to state:

Length Field (4 bytes):  Indicates the length of only the "Value"
field of this optionally logged element (as shown in Figure 5),
hexadecimal encoded. This length corresponds to the length of the
"Value" field only and MUST NOT include any of the other elements
shown in Figure 5.

> 11) Where does the document talk about how to log fields that are line folded
> in the original message? Is it ok for the logged field to contain CRLFs, or is
> the intent that the field be unfolded before logging?

I will update the text to indicate that:

For mandatory fields:

 The mandatory fields MUST NOT contain CRLF when logged.

For optional fields:

If an optional field contains a CRLF as part of its normal 
production rule, the CRLF MUST be escaped by using the 
URI encoded equivalent value of "%0D%0A".

For logging bodies:

All CRLF digraphs in the SIP body MUST be escaped by using the 
URI encoded equivalent value of "%0D%0A"

For logging the entire SIP message:

All instances of CRLF digraphs, whether they appear in the SIP
headers or the body MUST be escaped by using the URI encoded 
equivalent value of "%0D%0A".

> 12) It's not clear if the \r\n used in the examples of logging the SDP body
> are there as text-conventions for showing the example in an RFC, or if you
> intend that CRLFs be replaced by \r\n's when logging. If it's just a convention,
> please make it clear that the actual log record will have CRLFs. If you were
> expecting the CRLFs to be escaped in the log, there needs to be text that
> says that normatively somewhere, and you need to consider escaping the escape
> character.

As per my comments for point #11 above, the \r\n in that example needs to be 
escaped with "%0D%0A". I'll update the examples to match with this new 
text that I will be adding.

> Minor
> 
> There is a maintenance risk in having all the detail of each block duplicated
> in Figure 1. Please consider taking that figure up a level, showing the three
> blocks and how they're separated, and let the other figures show the internal
> detail.

This is something that we have discussed and have not found an elegant solution for. 
It was thought to be very valuable to display the pertinent portion of the complete 
CLF within the section where that was discussed. I'll do my best to ensure that they 
are all copied over properly from Figure 1.

> There are blocks of text in this document that are really laying out the problem.
> Can they be moved into the problem statement/information model/framework document
> or simply deleted?
> For example: "Implementing a logging scheme for SIP is a considerable challenge.",
> "Whilst one may question the value of the From URI [...]", etc.

Ok. I'll scour the document for instances of such text and remove them.

> In section 4.1, you talk of "a single, 52 character hexadecimal encoded string"
> between bytes 8 and 55. There are only 48 octets between bytes 8 and 55
> (inclusive). Please adjust the text to note that you are also talking about
> the pointers to the optional fields or describe the shorter length.

I'll edit this text to be clearer.

> In section 4.2's description of the To tag field you say "Value of the
> tag parameter (if present)" but don't talk about what to do if the tag
> parameter isn't present. In the parallel section on the From tag, you
> don't talk about the potential for messages without From tags. If it's
> the intent to not be able to log messages from legacy 2543 devices, that's
> fine, but the document should call that out as a design decision and it wouldn't
> hurt to provide some guidance on what to do if such a message shows up.

I'll add some text to clarify and simplify this. Basically, if the tag is present in To and From, 
it is logged. If it is not present, a "-" is logged. I'll leave it at that. The intent is not 
to differentiate between messages from legacy rfc2543 devices versus the 
updated rfc3261.  

> There is a lot of descriptive text in 4.2's sections on the server and client
> transaction that is a copy of what's in the problem-statement/information model
> document. It would be better to just point to the description in the other document.

This is something that we debated about quite a bit. In the end we thought it more 
useful for implementers to have the description of all mandatory fields right in the 
format draft.

> 
> Nits
> 
> How is this a "functionally equivalent event logging mechanism"? Suggest replacing
> that sentence with the single word "analogous".

I'll make this change.

> In the introduction, the document claims to define a format that is no more difficult
> to generate, but it's not clear what it's being compared to. No more difficult than
> what?

I'll clarify this.

> Section 4.1's description of Record Length can be misinterpreted. By listing
> a subset of the fields that are included, it leaves open whether any fields
> are excluded. It would be better to say "beginning with the Version octet
> and ending with the terminating line-feed".

I'll make this change.

> It would help to be more explicit that you are not logging the CRLF at the end
> of a given header field value. (Or is it an option for a logger to choose to do
> so?)

I discussed this in one of my earlier responses. This will be stated more explicitly 
that the terminating CRLF of a logged field should not be logged. The only CRLF's 
that should be logged are those of a multi-line body, field, or complete message 
and they should be escaped as will be specified in the draft.

> Please consider scrubbing the message used in example 5 back to the minimal
> number of octets you need to make the example useful. In particular, consider
> removing elements that have not already been standardized. I'm surprised to see
> the literal strings "server-tx" and "client-tx" in the example log message. Was
> that what you really intended?

I'll look to trim down the sample message in Section 5 to a minimal set of fields. 
The use of server-txn and client-txn was intentional and done to parallel the usage in 
the data model draft. If you think a specific value is best, then I'll make one up.

> The code snippet should be delineated as such, following the
> guidance in the TLP (see section 4.b of <http://trustee.ietf.org/docs/IETF-Trust-License-Policy.pdf>

No problem. I'll add the the markers <CODE BEGINS> and <CODE ENDS> around 
the code snippet.

[1] http://www.ietf.org/mail-archive/web/sip-clf/current/msg00423.html

Thanks,

Gonzalo

> 
> 
> _______________________________________________
> sip-clf mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sip-clf
>

---

From: Robert Sparks <[email-masked]>
Date: Thu, 09 Feb 2012 09:18:40 -0600

Inline, trimming to points with responses

>> Major (mostly in document order)
<snip/>
> a SIP CLF reader MUST be able to interpret them by discarding
Actually, I was under the impression that the zero padding was
expected. The examples have them consistently - why did that happen?
Either way, group members should say something about what the intent was.

> since the draft currently has no IANA dependencies.
That's a choice the group should make - the document just needs to be 
explicit
about what's extensible (for all the fields) and if a field is 
extensible, what the mechanism
looks like. For the transport field, as an individual contributor, I 
prefer the IANA registry approach.

<snip/>
>  it SHOULD occur when when logging either the entire message or
This leaves out optional fields. It's also not clear why you use a SHOULD
for the requirement over bodies.

You should also make it clear (as part of escaping above) whether any
escaping is done before encoding binary bodies.

> entirely nonsensical to it.
I think it _was_ the intent to be able to reuse parser code.
Several early implementers were handing individual fields to the part of 
their parser that
would handle a framed header field.

> The belief is that the reader of the SIP CLF would know
It will surprise me if a simple statement will fully address this point 
- be sure to
capture whether the reader unescapes anyththing.
> was a decision made by the WG [1].
The document needs to explain _why_ this particular subset was chosen.
What basis did the group use for that choice? My read of the thread was 
that Vijay
threw an initial list together and that just ended up in the document. 
It's not clear
that the list was intended to be exclusive, just that whatever 
subsequent decisions
that were made allowed at least those types.

WHY are you not allowing images? Why are you not allowing multipart/mime?
(If you ever did actually see an S/MIME protected body, you would not be 
able to log it
the way the document stands now.)
> The usage of a LWS separator was specified
Please be careful to be specific in the instructions on when to encode
(for example, would you re-encode a binary format that was already 
base64 encoded?)

<snip/>
> to clearly state this point.

Now I'm confused. Why do you have the ability to repeat these tags?

<snip/>
> URI encoded equivalent value of "%0D%0A".
Why are you treating optional fields and mandatory fields differently here?
If there's a good reason, fine, but be sure to include the reason in the 
document.
> equivalent value of "%0D%0A".
Even when the body is binary (see previous discussion about escaping)?
The group should weigh in on whether this is what they were expecting for
the usual application/sdp case.

> are all copied over properly from Figure 1.
This has been a common theme in the IESG reviews of many recent 
documents. The issue
is not the care you take while editing this document. It's the burden 
you put on whoever
ends up with the pen on an update.
> useful for implementers to have the description of all mandatory 
Again, this is something that IESG review has put pressure against in 
many recent
documents. Expect to defend this during that review. At the very least, 
make it clear
which document is authoritative should they conflict.
> the data model draft. If you think a specific value is best, then I'll 
It's confusing as is - the example is supposed to be an actual log 
message, not a
pseudo-code description of one isn't it?

RjS

---

From: "Vijay K. Gurbani" <[email-masked]>
Date: Thu, 09 Feb 2012 12:02:27 -0600

> Inline, trimming to points with responses

Robert: Some feedback on your responses inline.

With respect to extending the Transport flag, you wrote that:

> explicit about what's extensible (for all the fields) and if a field
> is extensible, what the mechanism looks like. For the transport
> field, as an individual contributor, I prefer the IANA registry
> approach.

I agree that IANA is cleaner.  It will be nice if we hear more views
from the WG so we can proceed accordingly.

> for the requirement over bodies.

I believe that the SIP CLF record will be used as a medium to do a quick
spot check as well as a medium for detailed analysis.

When used as a medium for a quick spot check, I believe that the SIP
CLF record should be amenable to being manipulated by normal Unix text
processing tools.  Thus, those who use the standard Unix command line
text tools to look for something in the SIP CLF log file will be mostly
interested in the mandatory header fields (i.e., grep "2 INVITE" logfile
or grep "S2388-188" logfile to find all records with server transaction
ID "S2388-188").  The result could be piped to tr(1), awk(1), cut(1) or
perl for further processing.  Quick example:

   $ grep "2 INVITE" logfile | cut -f13

will print the server transaction ID (field 13) of all records
where the CSeq field is "2 INVITE".

That is why I think that tabs in the mandatory fields MUST be changed
to spaces.

Exploring the body and optional fields is part of detailed analysis
of the SIP CLF record.  To do this, one would need an actual
SIP CLF reader, which will be presumably more intelligent and does not
need a LWS delimiter to figure out boundaries (it already has the length
of the body or the optional field).

In fact, the more I think of it the more it seems that the SIP CLF
record should really consist of three distinct stanzas, each
separated by a LF:

   Index-pointers 0x0A  Mandatory-fields 0x0A Optional-fields 0x0A

Right now, we separate the SIP CLF record in two stanzas:

   Index-pointers 0x0A Mandatory-Fields-and-Optional-fields 0x0A

The problem with separating the SIP CLF record in two stanzas is
that a "grep S2388-188 logfile" will return the mandatory field
line PLUS any optional fields logged.  These optional fields are
therefore forced to change tab to spaces.  If we have 3 stanzas,
then the optional fields can retain tabs.

Regarding the SIP CLF record being syntactically amenable to a parse
by a normal SIP parser, you wrote that:

> I think it _was_ the intent to be able to reuse parser code. Several
> early implementers were handing individual fields to the part of
> their parser that would handle a framed header field.

The distinction being made here is whether individual *fields* in the
SIP CLF record can be extracted and given to a SIP parser that would
normally parse those sequence of strings versus an existing SIP parser
that will successfully parse a SIP CLF record in its *entirety*.

Clearly, the latter is not possible.  The former is a possibility.  To
be safe, the document should indicate that escaping '-' and '?'
may produce a field that is not syntactically parsed by a SIP parser
that would normally be able to parse that field if it appeared
in a SIP header (as opposed to a log file).

Thanks,

- vijay
-- 
Vijay K. Gurbani, Bell Laboratories, Alcatel-Lucent
1960 Lucent Lane, Rm. 9C-533, Naperville, Illinois 60563 (USA)
Email: vkg@{bell-labs.com,acm.org} / [email-masked]
Web:   http://ect.bell-labs.com/who/vkg/

---

From: Gonzalo Salgueiro <[email-masked]>
Date: Thu, 9 Feb 2012 16:01:21 -0500

> from the WG so we can proceed accordingly.

I'll weigh in that I think the IANA registration route is more elegant and practical from an extensibility point of view.

Regards,

Gonzalo

---

From: Gonzalo Salgueiro <[email-masked]>
Date: Thu, 9 Feb 2012 17:53:44 -0500

Hi Robert - 

Thanks for your responsiveness on this. I'll close the loop on just those points 
that Vijay didn't already touch on in his response.

<snip>

> Either way, group members should say something about what the intent was.

The examples will be updated to reflect the updated text regarding the 
zero padding.

<snip/> 
<snip>

> What basis did the group use for that choice? My read of the thread was that Vijay
> threw an initial list together and that just ended up in the document. It's not clear
> that the list was intended to be exclusive, just that whatever subsequent decisions
> (If you ever did actually see an S/MIME protected body, you would not be able to log it

We floated some text on the list that contained a list of those body types the WG thought 
MUST be logged.  Nobody responded with an alternative, neither did anyone respond 
against the proposal.  So we went with it.  I'm happy to reopen this discussion again on the 
list, but if no one else answers than we are no better off than before.

I share your concern that we are being needlessly restrictive so I'll put forth a proposal 
to the WG in the next version of the draft that removes the constraint of logging bodies 
of those specified types. The logging mechanism will remain unchanged but without 
body type restrictions.

> (for example, would you re-encode a binary format that was already base64 encoded?)

Sounds good. I'll add text to address this.

> Now I'm confused. Why do you have the ability to repeat these tags?

It *is* currently confusing. The updated text will remove this ability to have multiple 
Tag -01 and -02 entries. The only repeated Tag that will be allowed is -00 ONLY when 
logging an optional field that occurs more than once in a SIP message (e.g.  Contact, 
Route, Record-Route, etc.).

> If there's a good reason, fine, but be sure to include the reason in the document.

I'm simply laying out the escape rules for the various CLF log elements.

> the usual application/sdp case.

I think _ANY_ time a CRLF is present in a body it is escaped. If it isn't there in a 
binary body, then there is no need to escape.

> This has been a common theme in the IESG reviews of many recent documents. The issue
> is not the care you take while editing this document. It's the burden you put on whoever

I certainly understand. I just don't have a better solution at this time.

> Again, this is something that IESG review has put pressure against in many recent
> documents. Expect to defend this during that review. At the very least, make it clear

I'll do my best to defend this because I think it is what is best for this document and its 
'one stop shop' readability for implementers. If this turns out to be a battle not worth 
waging with the IESG then I will concede and add a reference.

> It's confusing as is - the example is supposed to be an actual log message, not a

Yes, it should be an actual log message. I'll update as requested.

Thanks,

Gonzalo

---

From: Peter Musgrave <[email-masked]>
Date: Thu, 16 Feb 2012 05:48:31 -0500

HI all, 

(as individual)

I think a registry is more future-proof. 

Peter