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

From: Eric Rescorla <[email-masked]>
Date: Tue, 20 Nov 2018 18:07:20 -0800

Eric Rescorla has entered the following ballot position for
draft-ietf-dnsop-dns-capture-format-08: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-dnsop-dns-capture-format/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Rich version of this review at:
https://mozphab-ietf.devsvcdev.mozaws.net/D4670

I support Ben's DISCUSS.

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?

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.

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.

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.

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.

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?

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?

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.

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.

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?

---

From: Sara Dickinson <[email-masked]>
Date: Wed, 21 Nov 2018 17:25:50 +0000

> Begin forwarded message:
> 
> From: Eric Rescorla <[email-masked]>
> 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" <[email-masked]>
> Cc: Tim Wicinski <[email-masked]>, [email-masked], [email-masked], [email-masked], [email-masked]
> Resent-From: <[email-masked]>
> Resent-To: [email-masked], [email-masked], [email-masked], [email-masked], [email-masked]
> 

Hi, 

Many thanks for the review

Ack - hopefully our response to him addresses your concerns.

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

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

> nothing, but that's up to you.

Yeah - this might make more sense. 

> Nit: generally, you would say "indicating which fields are 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?

>>     "MalformedMessage" items in this "Block".  Each element is an array

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

> consisting of a set of indexes int o QRR?

Yes, exactly.

> literally in here. It has to be in the block tables?

Correct.

> some bits by having this attached to the server and then indixed

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

> Again, probably want to specify DoH.

Yes - we’ll add it. 

> Is this a normative appendix?

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

Best regards

Sara.

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 21 Nov 2018 10:32:48 -0800

On Wed, Nov 21, 2018 at 9:25 AM Sara Dickinson <[email-masked]> wrote:

>
>
> Begin forwarded message:
>
> *From: *Eric Rescorla <[email-masked]>
> *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" <[email-masked]>
> *Cc: *Tim Wicinski <[email-masked]>, [email-masked],
> [email-masked], [email-masked],
> [email-masked]
> *Resent-From: *<[email-masked]>
> *Resent-To: [email-masked], [email-masked], [email-masked],
> [email-masked], [email-masked]
>

I'll leave it with him.

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

Yes, as it seems like an unobvious tradeoff. It's worth noting that if you
put the block tables at the end, you could save memory on the encoder.

> the sub-second timing increments are.

Ah I missed that. A reference to 7.4.1.1 would help.

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

Maybe, I just thought that the text was a bit odd.

> S 7.5.3.

Maybe. I'll let you sort this out with RFC Ed.

S 7.5.3.2.
> 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...
>

OK. Just thought I would point it out.

-Ekr