Re: [DNSOP] Eric Rescorla's No Objection on draft-ietf-dnsop-dns-capture-format-08: (with COMMENT)

[Sara Dickinson <[e-mail]>]

> Begin forwarded message:
> 
> From: Eric Rescorla <[e-mail]>
> Subject: Eric Rescorla's No Objection on draft-ietf-dnsop-dns-capture-format-08: (with COMMENT)
> Date: 21 November 2018 at 02:07:20 GMT
> To: "The IESG" <[e-mail]>
> Cc: Tim Wicinski <[e-mail]>, [e-mail], [e-mail], [e-mail], [e-mail]
> Resent-From: <[e-mail]>
> Resent-To: [e-mail], [e-mail], [e-mail], [e-mail], [e-mail]
> 
> Eric Rescorla has entered the following ballot position for
> draft-ietf-dnsop-dns-capture-format-08: No Objection

Hi, 

Many thanks for the review


> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Rich version of this review at:
> https://mozphab-ietf.devsvcdev.mozaws.net/D4670
> 
> 
> I support Ben's DISCUSS.

Ack - hopefully our response to him addresses your concerns.

> 
> Am I understanding the design correctly in that you can't have
> literals in the individual per-query values, but instead references to
> the block tables, so you can't stream inside a block?

Yes, that is correct - do you think we need to add text to clarify this?

> 
> IMPORTANT
> S 7.5.1.
>>     | earliest-time    | O | A | A timestamp (2 unsigned integers,      |
>>     |                  |   |   | "Timestamp") for the earliest record   |
>>     |                  |   |   | in the "Block" item. The first integer |
>>     |                  |   |   | is the number of seconds since the     |
>>     |                  |   |   | Posix epoch ("time_t"). The second     |
>>     |                  |   |   | integer is the number of ticks since   |
> 
> I don't know what a "tick" is, so you need some definitionf or this.

From 7.4.1.1:

ticks-per-second | M | U | Sub-second timing is recorded in       |
   |                  |   |   | ticks. This specifies the number of    |
   |                  |   |   | ticks in a second. 

This was the solution to a request to allow flexibility in how granular the sub-second timing increments are. 

> 
> COMMENTS
> S 7.2.
>> 
>>     o  The column O marks whether items in a map are optional.
>> 
>>        *  O - Optional.  The item may be omitted.
>> 
>>        *  M - Mandatory.  The item must be present.
> 
> FWIW, I think you might be happier with a mandatory "X" and optional
> nothing, but that's up to you.

Yeah - this might make more sense. 

> 
> 
> S 7.4.1.1.1.
>> 
>>     +------------------+---+---+----------------------------------------+
>>     | Field            | O | T | Description                            |
>>     +------------------+---+---+----------------------------------------+
>>     | query-response   | M | U | Hints indicating which "QueryResponse" |
>>     | -hints           |   |   | fields are omitted, see section        |
> 
> Nit: generally, you would say "indicating which fields are included"
> if 1 means included.

The issue is that if the bit is set the field might still be missing because although the configuration was set to collect it the data wasn’t available to the encoder from some other reason. However when the bit is not set it means that the data will definitely not be present because the collector is configured not to collect it. 

We do discuss this problem in section 6.2.1 - perhaps a reference in the table back to that discussion is what is needed?

> 
> 
> S 7.5.3.
>>     +---------------------+---+---+-------------------------------------+
>> 
>>  7.5.3.  "BlockTables"
>> 
>>     Arrays containing data referenced by individual "QueryResponse" or
>>     "MalformedMessage" items in this "Block".  Each element is an array
> 
> This is not a sentence.

Suggest: “Map of arrays containing data... "

> 
> 
> S 7.5.3.
>>     | qrr               | O | A | Array of type "Question". Each entry  |
>>     |                   |   |   | is the contents of a single question, |
>>     |                   |   |   | where a question is the second or     |
>>     |                   |   |   | subsequent question in a query. See   |
>>     |                   |   |   | Section 7.5.3.3.                      |
>>     |                   |   |   |                                       |
> 
> So if I understand this correctly:
> 
> QRR is a map of integers to question
> QLIST is a map of integers to question lists, with each question list
> consisting of a set of indexes int o QRR?

Yes, exactly.

> 
> 
> 
> S 7.5.3.2.
>> 
>>     +--------------------+---+---+--------------------------------------+
>>     | Field              | O | T | Description                          |
>>     +--------------------+---+---+--------------------------------------+
>>     | server-address     | O | U | The index in the item in the "ip-    |
>>     | -index             |   |   | address" array of the server IP      |
> 
> So am I understanding correctly that you can't put the server address
> literally in here. It has to be in the block tables?

Correct.

> 
> 
> S 7.5.3.2.
>>     +--------------------+---+---+--------------------------------------+
>>     | server-address     | O | U | The index in the item in the "ip-    |
>>     | -index             |   |   | address" array of the server IP      |
>>     |                    |   |   | address. See Section 7.5.3.          |
>>     |                    |   |   |                                      |
>>     | server-port        | O | U | The server port.                     |
> 
> Isn't the server port generally constant? It seems like you might save
> some bits by having this attached to the server and then indixed
> abvoe.

Yes it is (or likely to be one of 3 values), and I think you are right there is a small saving in what you are suggesting but we chose consistency of representation (with client address) over that slight added saving...

> 
> 
> 
> S 7.5.3.2.
>>     |                    |   |   | used to service the query.           |
>>     |                    |   |   | Bit 0. IP version. 0 if IPv4, 1 if   |
>>     |                    |   |   | IPv6                                 |
>>     |                    |   |   | Bit 1-4. Transport. 4 bit unsigned   |
>>     |                    |   |   | value where 0 = UDP, 1 = TCP, 2 =    |
>>     |                    |   |   | TLS, 3 = DTLS. Values 4-15 are       |
> 
> You might want to specify DoH
> 
> 
> S 7.5.3.5.
>>     |                    |   |   | Bit 0. IP version. 0 if IPv4, 1 if   |
>>     |                    |   |   | IPv6                                 |
>>     |                    |   |   | Bit 1-4. Transport. 4 bit unsigned   |
>>     |                    |   |   | value where 0 = UDP, 1 = TCP, 2 =    |
>>     |                    |   |   | TLS, 3 = DTLS. Values 4-15 are       |
>>     |                    |   |   | reserved for future use.             |
> 
> Again, probably want to specify DoH.

Yes - we’ll add it. 

> 
> 
> S 17.3.
>>     [18] https://github.com/dns-stats/draft-dns-capture-
>>          format/blob/master/file-size-versus-block-size.svg
>> 
>>  Appendix A.  CDDL
>> 
>>     This appendix gives a CDDL [I-D.ietf-cbor-cddl] specification for
> 
> Is this a normative appendix?

As picked up in other reviews, we will make the CDDL document a normative reference. 

Best regards

Sara.