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

[Eric Rescorla <[e-mail]>]

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

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

I'll leave it with him.



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

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.

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

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


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

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


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

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

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

OK. Just thought I would point it out.

-Ekr


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