Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for EDNS and DNSSEC #803

Merged
merged 3 commits into from Jan 25, 2016

Conversation

@McStork
Copy link
Contributor

commented Jan 21, 2016

Library change from gopacket to miekg/dns
  • Modify payload objects and attributes used throughout the dns package
  • FQDNs now terminate with a dot as any absolute domain name should RFC1035
  • Update glide with the miekg/dns library
Also
  • Do a minor cleaning in the function that decodes DNS over TCP
  • Move DnsMessage struct from dns_tcp.go to dns.go
  • Change the flag 'recursion_allowed' to 'recursion_available' DNS Header Flags
  • Change the SOA field that was named 'ttl' to 'minimum' RFC 1035
TODO list
  • Change decode library to miekg/dns
  • EDNS
  • DNSSEC
@elasticsearch-release

This comment has been minimized.

Copy link

commented Jan 21, 2016

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

data = rr.IP.String()
case layers.DNSTypeSOA:
// We don't have special handling for this type
logp.Debug("dns", "Unsupported RR type %s", dnsTypeToString(rrType))

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jan 22, 2016

Member

Should the default case set data = x.Rdata? I'm assuming Rdata is raw data.

This comment has been minimized.

Copy link
@McStork

McStork Jan 22, 2016

Author Contributor

Yes. At first I thought that couldn't be achieved ... because only type RFC3597 has the Rdata field, other types having the fields according to their format.
But a method ToRFC3597 helps fetch this field from any RR type.
With some lines of code (e.g. here), the Rdata is fetched. I'm not sure a simpler solution exists though.

func dnsQuestionToString(q layers.DNSQuestion) string {
name := nameToString(q.Name)
if len(name) == 0 {
name = "Root"

This comment has been minimized.

Copy link
@andrewkroh

andrewkroh Jan 22, 2016

Member

So this "Root" handling is taken care of by mkdns (sorry I didn't look)?

This comment has been minimized.

Copy link
@McStork

McStork Jan 22, 2016

Author Contributor

If the domain name length == 0, then it is set to "." See func UnpackDomainName

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

@McStork, the changes look really solid to me. Thanks

A few things...

  • Re-generate the docs/fields.asciidoc from the modified fields.yml. There is a python script in the scripts directory for this.
  • Update the project changelog to say that we switched to miekg/dns for parsing DNS. And note in the backward compatibility section about the FQDN's ending in dots and the rename of the field.
@andrewkroh

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

@elastic/beats Should the glide.yaml contain the commit hash of github.com/miekg/dns? I think we should include the commit hash so we don't have to guess at what point we took the library, but I don't really know all the details of glide.

@McStork

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2016

@andrewkroh Generating fields.asciidoc showed that two fields from ICMP (icmp.request.message, icmp.response.message) are missing from the doc. This affects v1.1 btw. Should a ticket be open?

@andrewkroh

This comment has been minimized.

Copy link
Member

commented Jan 22, 2016

Regarding the fields.asciidoc on 1.1. We updated it - #821. Thanks for pointing it out.

McStork added 3 commits Jan 19, 2016
Library change from gopacket to miekg/dns:
* Modify payload objects and attributes used throughout the dns package
* FQDNs now terminate with a dot as any absolute domain name should [RFC1035](https://tools.ietf.org/html/rfc1035)
* Update glide with the miekg/dns library

Also:
* Do a minor cleaning in the function that decodes DNS over TCP
* Move DnsMessage struct from dns_tcp.go to dns.go
* Change the flag 'recursion_allowed' to 'recursion_available' [DNS Header Flags](http://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-12)
* Fetch Rdata for yet unhandled and true unknowns RR types
* Re-generate the docs/fields.asciidoc
* Update CHANGELOG
* Add commit hash of lib miekg/dns to glide.yaml

Also
* Change the SOA field that was named 'ttl' to 'minimum' [RFC 1035](https://www.ietf.org/rfc/rfc1035.txt)
@McStork McStork force-pushed the McStork:dns-miekg branch from f20a301 to 8eabce0 Jan 25, 2016
@andrewkroh

This comment has been minimized.

Copy link
Member

commented Jan 25, 2016

LGTM so I am going to merge it now and additional changes can be done is new PR. Thanks

andrewkroh added a commit that referenced this pull request Jan 25, 2016
Change decode library to miekg/dns
@andrewkroh andrewkroh merged commit fe9655c into elastic:master Jan 25, 2016
3 checks passed
3 checks passed
CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@McStork

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2016

@andrewkroh Ok! Let's do it this way then.

@McStork McStork deleted the McStork:dns-miekg branch Feb 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.