Re: [DNSOP] Alexey Melnikov's Discuss on draft-ietf-dnsop-dns-capture-format-08: (with DISCUSS and COMMENT)

[Sara Dickinson <[e-mail]>]

> On 19 Nov 2018, at 20:37, Alexey Melnikov <[e-mail]> wrote:

Hi Alexey, 

Thanks for the review.

> 
> Alexey Melnikov has entered the following ballot position for
> draft-ietf-dnsop-dns-capture-format-08: Discuss
> ----------------------------------------------------------------------
> DISCUSS:
> ----------------------------------------------------------------------
> 
> Thank you for this document, it is a useful contribution to RFC series. I
> enjoyed reading it.
> 
> I have a small list of issues that is hopefully easy to fix:
> 
> 1)
> 
> In 7.4.2:
> 
>   | filter           | O | T | "tcpdump" [pcap] style filter for      |
>   |                  |   |   | input.                                 |
> 
> This makes the [pcap] reference Normative. If you don't want to do that, please
> fully specify syntax in this document.

Is that true if it is an optional field? 

> 
> 2) I believe [I-D.ietf-cbor-cddl] reference needs to be Normative due to use of
> CDDL in Appendix A. If you don't think CDDL is normative, you need to state
> that in Appendix A.

Yes indeed - it should be normative, will fix. 


> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> Was CDDL in Appendix A validated with a tool? This information is missing from
> the shepherding write-up.

We (the authors) have used the CDDL tool described on http://cbor.io/tools.html <http://cbor.io/tools.html> to read the CDDL in Appendix A and ensured an example instance can be created. 

Did you have some other validation tool in mind?

> 
> 6.2.3.  Storage flags
> 
>   The Storage Parameters also contains optional fields holding details
>   of the sampling method used and the anonymisation method used.  It is
>   RECOMMENDED these fields contain URIs pointing to resources
>   describing the methods used.
> 
> Please add a Normative Reference for URI spec here (RFC 3986).

Yes, will do. 

> 
> 7.5.3.2.  "QueryResponseSignature"
> 
>   | qr-transport-flags | O | U | Bit flags describing the transport   |
>   |                    |   |   | 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       |
>   |                    |   |   | reserved for future use.             |
>   |                    |   |   | Bit 5. 1 if trailing bytes in query  |
>   |                    |   |   | packet. See Section 11.2.            |
> 
> Would something like DoH appear as a separate transport choice?

No, we need to add DoH to this list (it didn’t exist when we started the draft!). 

> 
> How can new values be added to the list if there are no IANA Considerations?

As described in response to the DISCUSS on this issue we propose IANA create a C-DNS registry with
subregistries with keys for each of the different maps used in C-DNS.
New entries in these subregistries would follow Expert Review 

> 
> 7.5.3.5.  "MalformedMessageData"
> 
>   |                    |   |   |                                      |
>   | mm-transport-flags | O | U | Bit flags describing the transport   |
>   |                    |   |   | used to service the query. Bit 0 is  |
>   |                    |   |   | the least significant bit.           |
>   |                    |   |   | 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.             |
> 
> If the above bits supposed to be the same as for qr-transport-flags,
> maybe it is worth defining them once and referencing in all relevant places?

The qr-transport-flags and mm-transport-flags are different in that the qr-transport-flags include Bit 5, the trailing bytes indicator.

In the CDDL a base ’TransportFlags’ type is defined and then

mm-transport-flags     => TransportFlags,

qr-transport-flags    => QueryResponseTransportFlags,

 QueryResponseTransportFlagValues = &(
      query-trailingdata : 5,
  ) / TransportFlagValues
  QueryResponseTransportFlags = uint .bits QueryResponseTransportFlagValues

We can add some text to the table descriptions in sections 7.5.3.2 and 7.5.3.5 to clarify the relationship. 

> 
> 7.6.  "QueryResponse"
>   | query-size           | O | U | DNS query message size (see        |
>   |                      |   |   | below).                            |
>   |                      |   |   |                                    |
>   | response-size        | O | U | DNS query message size (see        |
>   |                      |   |   | below).                            |
> 
> I think "DNS response message size" for response-size.
> 
> It is odd to have RFC 2119 language in B.2, which is itself informative.


Good catch :-)

Many thanks

Sara.