adulau / pdns-qof

Passive DNS Common Output Format
https://datatracker.ietf.org/doc/draft-dulaunoy-dnsop-passive-dns-cof/
36 stars 15 forks source link

Clean up https://github.com/adulau/pdns-qof/wiki/Additional-Fields #17

Open djw1149 opened 4 years ago

djw1149 commented 4 years ago

The additional fields registry at https://github.com/adulau/pdns-qof/wiki/Additional-Fields contains three fields that have long since been added to the COF draft: sensor_id, zone_time_first, and zone_time_last. I suggest that they be removed from the registry or an additional column be added to the registry table showing the draft version number or RFC number in which each additional field was added to the I-D or RFC (as/if becomes applicable).

aaronkaplan commented 4 years ago

hi djw1149,

yes, I totally agree. However, noticed that we do not have any definition of the term (import of...) "dns master file". (compare the definition of zone_time_first). I think that's another bug in the text.

Thanks for catching this! I think we had overlooked the registry. One thing that should go into the registry is the request that milli/nano second resolution should be added optionally (personally I don't think that makes much sense or is actually needed, but at FIRST meetings, folks requested that).

Best, a.

On 24.01.2020, at 19:28, djw1149 notifications@github.com wrote:

The additional fields registry at https://github.com/adulau/pdns-qof/wiki/Additional-Fields contains three fields that have long since been added to the COF draft: sensor_id, zone_time_first, and zone_time_last. I suggest that they be removed from the registry or an additional column be added to the registry table showing the draft version number or RFC number in which each additional field was added to the I-D or RFC (as/if becomes applicable).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

djw1149 commented 4 years ago

For the master file format, perhaps we add a reference to RFC1035 section "5.1. Format" in our 3.5.2 and 3.5.3? We already reference "DNS master file RFC 1035..." in our section "3.3.3 rdata".

For the milli/nano resolution, are you saying to add additional variations of all time based fields with the higher resolution data format? i.e. a new time_first, time_last, zone_time_first, and zone_time_last? We actually probably don't need it for zone_time_first and zone_time_last.

Perhaps:

time_first_hires

This field returns the first time that the record / unique tuple (rrname, rrtype, rdata) has been seen by the passive DNS with a higher resolution than the standard time_first field. The timestamp is expressed in milliseconds (decimal) since 1st of January 1970 00:00:00.0000 (the Unix epoch), but this is 1000 times larger than a Unix timestamp. The time zone MUST be UTC. This field is represented as a JSON [RFC4627] number. If BOTH time_first and time_first_hires are present in a record, then INTEGER(time_first_hires / 1000) MUST equal time_first, otherwise the behavior is undefined.

time_last_hires

(Similar to above: s/time_first/time_last/g)

aaronkaplan commented 4 years ago

See version 07. Good enough?

djw1149 commented 4 years ago

Could you clarify:

if time_first_hires is present then time_first need not be present. (and the implementation is still "conforming")

if time_last_hires is present then time_last need not be present.

aaronkaplan commented 4 years ago

On 06.07.2020, at 15:13, D Waitzman notifications@github.com wrote:

Could you clarify:

if time_first_hires is present then time_first need not be present. (and the implementation is still "conforming")

if time_last_hires is present then time_last need not be present.

Good point, I would not do that because the time_*_hires fields are optional, whereas the others are mandatory. The hires fields are therefore mere additional information , provided by those servers who have that info in the first place (not all do) and I feel the approach above would confuse clients.

I am for always showing time_first and time_last. You never know where the data is passed onto.

Best, a.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

djw1149 commented 4 years ago

Aaron, your response makes sense, but a COF object can validly have zone_time_first but not time_first. Implementation MUST support all the mandatory fields. does not mean that all the mandatory fields MUST be present in every COF object.

aaronkaplan commented 4 years ago

On 06.07.2020, at 15:56, D Waitzman notifications@github.com wrote:

Aaron, your response makes sense, but a COF object can validly have zone_time_first but not time_first. Implementation MUST support all the mandatory fields. does not mean that all the mandatory fields MUST be present in every COF object.

True, but I'd like to keep that simple and give a guarantee / contract to readers of the COF that they can assume certain fields.

But valid point of course. Maybe we should clarify that better that readers of the COF can expect time_first, time_last always? (and the other fields possibly in addition).

Makes sense for you?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

vixie commented 4 years ago

i would still prefer that the _hires times be just the fraction, thus not duplicating the time_first and time_last, which should remain mandatory. if we had time_first_nano and time_last_nano it ought to be future proof. sensor_id is of course optional.

aaronkaplan commented 4 years ago

On 06.07.2020, at 17:55, paul vixie notifications@github.com wrote:

i would still prefer that the _hires times be just the fraction, thus not duplicating the time_first and time_last, which should remain mandatory. if we had time_first_nano and time_last_nano it ought to be future proof. sensor_id is of course optional.

okay, hmm... but a) AFAIK that's what came out of the FIRST pDNS SIG. b) are we OK to keep time_first, time_last no matter what now? (i.e. no matter if _hires is fraction or not or full or nanoseconds?)

In case (b) is OK for you, I can add a sentence (thanks for pointing it out!) about the mandatory-ness of time_first and time_last (even in the presence of _hires). Idea behind it: don't break existing clients.

Best, a.

bapril commented 4 years ago

okay, hmm... but a) AFAIK that's what came out of the FIRST pDNS SIG. b) are we OK to keep time_first, time_last no matter what now? (i.e. no matter if _hires is fraction or not or full or nanoseconds?)

In case (b) is OK for you, I can add a sentence (thanks for pointing it out!) about the mandatory-ness of time_first and time_last (even in the presence of _hires). Idea behind it: don't break existing clients.

The pattern that is currently in service should be supported without a compelling reason to break backwards compatibility. In current implementations the records contain either a pair of time_ or a pair of zonetime fields. Duplicating timestamps is unnecessary. I suggest updating the mandatory field list as follows:

(time_first AND time_last) OR (zone_time_first AND zone_time_last)

aaronkaplan commented 4 years ago

On 06.07.2020, at 18:55, bapril notifications@github.com wrote:

okay, hmm... but a) AFAIK that's what came out of the FIRST pDNS SIG. b) are we OK to keep time_first, time_last no matter what now? (i.e. no matter if _hires is fraction or not or full or nanoseconds?)

In case (b) is OK for you, I can add a sentence (thanks for pointing it out!) about the mandatory-ness of time_first and time_last (even in the presence of _hires). Idea behind it: don't break existing clients.

The pattern that is currently in service should be supported without a compelling reason to break backwards compatibility.

Exactly!

In current implementations the records contain either a pair of time_ or a pair of zonetime fields. Duplicating timestamps is unnecessary. I suggest updating the mandatory field list as follows:

(time_first AND time_last) OR (zone_time_first AND zone_time_last)

Sounds like a good compromise, but ... w.r.t to backwards compatibility, I am not really sure if users of the format implement the zonetime* fields. The only fields which are mandatory are the time_first, time_last fields.

Hm... a bit of a conundrum because time_first, time_last are mandatory, zone_time_first, zone_time_last (as well as the *_hires ones) are optional.

I wonder if it's better to solve this before it goes to the IETF or during that process....

What do you think?

bapril commented 4 years ago

Sounds like a good compromise, but ... w.r.t to backwards compatibility, I am not really sure if users of the format implement the zonetime* fields.

Farsight's DNSDB uses this model today. We send either time_[first|last] or zonetime[first|last], but not both.

The only fields which are mandatory are the time_first, time_last fields. Hm... a bit of a conundrum because time_first, time_last are mandatory, zone_time_first, zone_time_last (as well as the *_hires ones) are optional. I wonder if it's better to solve this before it goes to the IETF or during that process.... What do you think?

I agree we should get this solved. I propose restructuring the mandatory fields section such that the zonetime[first|last] are allowed to appear in place of time_[first|last], as long as one of the pairs is always present.

aaronkaplan commented 3 years ago

Added an additional note. I would still discourage this since it's really confusing for implementers.

aaronkaplan commented 3 years ago

See https://github.com/adulau/pdns-qof/pull/21

@vixie , @djw1149 , @bapril, works for you? I'd like to push for a consolidated version and finally send to the standards track

djw1149 commented 3 years ago

What I think is supposed to be my name is misspelled at https://github.com/adulau/pdns-qof/blob/413e0cffa9c714116b5b916e49c241fcfd367f71/i-d/pdns-qof.xml#L404 " <author fullname="Paul Vixie, Weizman, April, Kaplan, et.al"/>" should be <author fullname="Paul Vixie, Waitzman, April, Kaplan, et.al"/>