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

DNS - implementing TCP #486

Merged
merged 3 commits into from
Dec 16, 2015
Merged

DNS - implementing TCP #486

merged 3 commits into from
Dec 16, 2015

Conversation

McStork
Copy link
Contributor

@McStork McStork commented Dec 9, 2015

#386

Thanks @andrewkroh for the rebase.

@elasticsearch-release
Copy link

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

}
}

payload := make([]byte, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preferred way to declare and empty slice is var payload []byte because it avoids an allocation if the slice is never appended.

@andrewkroh
Copy link
Member

@McStork Thanks for the great work! I left a few comments (all minor).

@McStork
Copy link
Contributor Author

McStork commented Dec 9, 2015

Thanks for the review. Will update soon.

@McStork McStork force-pushed the dns-tcp-rebase branch 2 times, most recently from 544a0b7 to 93152d4 Compare December 10, 2015 17:23
@McStork
Copy link
Contributor Author

McStork commented Dec 10, 2015

Gaps are better managed now. However I wounder about the PrepareForNewMessage method because the stream.data is not reset, only stream.message is. But if it was reset, we would not be able to get the ID field of the original stream from the reverse stream.

I will have to read more of the tcp.go file to make sure I understand how all methods of an app layer work together ^^'.

@McStork
Copy link
Contributor Author

McStork commented Dec 14, 2015

@andrewkroh As you suggested in the review, I added Notes for when a response cannot be decoded in a TCP stream (Fin and Gap). That's a partial implementation of the TODO list point 'Publish a message when packets are received that cannot be decoded.'.
In order to publish notes for a Query that cannot be decoded, we would need a different way to build the transaction's hash. To publish such Notes would be really useful as you said in the ParseUdp method:

// This means that malformed requests or responses are being sent or
// that someone is attempting to the DNS port for non-DNS traffic. Both
// are issues that a monitoring system should report.

@andrewkroh Do you want to do more review ? Can we expect a merge soon ? =)
It would be nice if I could start working on EDNS/DNSSEC this week.

@tsg
Copy link
Contributor

tsg commented Dec 15, 2015

LGTM, yes, I think we can merge it today, but I'd like to wait for @andrewkroh to have one final look.

* Implement DNS over TCP
* Publish Notes when a response fails to decode in Gap and Fin

var payload []byte

// Offset is critical
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the reason for the two byte offset. "The message is prefixed with a two byte length field which gives the message length, excluding the two byte length field." - RFC 1035 - 4.2.2

This code should use the length field to short-circuit any processing. There is no need to attempt to decode the data when number of bytes is less than the length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it will print a debug message "Packet's data is null."

@andrewkroh
Copy link
Member

I built Packetbeat from your branch and ran some DNS queries over TCP (dig google.com +tcp). My queries are being parsed correctly, but the responses are not.

The events have status: Error and notes: Response packet's data could not be decoded as DNS. I think the cause is the two byte offset.

@McStork
Copy link
Contributor Author

McStork commented Dec 15, 2015

Thanks, I will look into it

@McStork
Copy link
Contributor Author

McStork commented Dec 15, 2015

I couldn't reproduce it. This message is only displayed if you get a Fin or Get with unvalid response DNS data.
All of your dig +tcpcommands fail ? All tests are validating though.

I will keep trying ;)

* return from Parse() if the packet length is <= Offset
* check if streams nil in publishDecodeFailureNotes()
@andrewkroh
Copy link
Member

Try the attached PCAP which contains a single DNS query for google.com over TCP. It should reproduce the Error status.

dns-tcp-google.com.zip

./packetbeat -c etc/packetbeat.dev.yaml -e -v -d "dns,tcp" -I dns-tcp-google.com.pcap -waitstop 10

@McStork
Copy link
Contributor Author

McStork commented Dec 16, 2015

It's good that you could make a split TCP query without DNSSEC.
At the same time it's weird to see that your transaction contains a 1 byte payload in the first response and the rest of the payload in the second response packet ^^.

The offset was not managed how it should have been:
* TCP decode Offset is now managed in the function decodeDnsData()
* Add a unit test calling the Parse() method on a split query

Also:
* remove assignements and use pointers to alter DnsStream objects in
Parse()
return priv
}
if dataLength <= DecodeOffset {
logp.Debug("dns", EmptyMsg+" addresses %s",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is not empty, but it is incomplete and the code will wait for more bytes to complete the message. I think the other protocols log messages for a similar case so maybe take a look at those messages.

andrewkroh added a commit that referenced this pull request Dec 16, 2015
@andrewkroh andrewkroh merged commit 9cf0f3d into elastic:master Dec 16, 2015
@andrewkroh
Copy link
Member

@McStork Thanks for you contribution!

I left a few minor comments about possible enhancements. They are not critical to the feature so I am merging now.

@McStork
Copy link
Contributor Author

McStork commented Dec 16, 2015

@andrewkroh Thank you very much for the reviews. I will open a new PR to address your comments.


payload := pkt.Payload

stream := &priv.Data[dir]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, pointer of pointer. I'd rather byte the bullet and do the assignment in line 753 instead of having to type (*stream) all over the place.

Check redis code for some cleaned-up solution (using pointers for connection) for cast and append (using streambuf.Buffer though). https://github.com/elastic/beats/blob/master/packetbeat/protos/redis/redis.go#L107

the dnsPrivateData data type is actually the connection context. Consider naming it so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that 😃

@McStork McStork deleted the dns-tcp-rebase branch January 6, 2016 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants