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

Start playing with dnstap #55

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

alanhlam
Copy link

@alanhlam alanhlam commented Oct 5, 2018

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

wip, go-audit dnstap client

e.g. New functionality for producing whatsits.

Related Issues

e.g. Fixes #206 and closes #230

Test strategy

e.g. Add tests around whatsit production.

audit.go Outdated
if err != nil {
el.Fatal(err)
}

dnstapClient, err := NewDnsTapClient(config.GetString("dnstap.socket"))
Copy link
Member

Choose a reason for hiding this comment

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

should this block be within a if config.GetString("dnstap.socket") != ""?

Copy link
Author

Choose a reason for hiding this comment

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

good call, thanks for that!

@wadey
Copy link
Member

wadey commented Oct 5, 2018

My biggest question is what happens when a bunch of hostnames all resolve to the same IP address (common case for a reverse proxy fronting a bunch of services). It seems like it could be hard to trust this value since it will always just be the last lookup. Do we just hope that the dnstap event will be processed right before the go-audit event?

We could cache and output all known hostnames for a given IP, but that could be unbounded size...

@@ -6,6 +6,10 @@ socket_buffer:
# Maximum max is net.core.rmem_max (/proc/sys/net/core/rmem_max)
receive: 16384

# Configure dnstap socket path if available
dnstap:
socket: /var/run/dnstap.sock
Copy link
Collaborator

Choose a reason for hiding this comment

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

dnstap.enabled: bool as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will also need config directives for the cache size and timeout

marshaller.go Outdated
case 1306:
// delay the mapping
time.Sleep(time.Millisecond * 100)
msg.mapDns(m)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If map dns fails then you need to exit and let the dnstap input flush this event

parser.go Outdated
start += 6
if end = strings.IndexByte(data[start:], spaceChar); end < 0 {
end = len(data) - start
if end > 34 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you arrive at this length? Make it a constant and document the parts that get to 34

parser.go Outdated
switch family := saddr[0:4]; family {
// 0200: ipv4
case "0200":
octet, _ := hex.DecodeString(saddr[8:16])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't ignore the error

parser.go Outdated

switch family := saddr[0:4]; family {
// 0200: ipv4
case "0200":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's support ipv6 as well

parser.go Outdated

host, ok := c.Get(ip)
if ok {
amg.DnsMap[ip] = host.(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There could be a performance enhancement here by separating ipv4 and ipv6 caches where ipv4 uses a proper uint32 and ipv6 uses a string. Not a today problem though.

parser.go Outdated
break
}

data = data[next:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure a saddr event can ever have multiples, we could avoid a few more ops by not doing a loop and just looking for the single occurrence

dnstap.go Outdated
"github.com/patrickmn/go-cache"
)

const defaultTimeout = time.Hour
Copy link
Collaborator

Choose a reason for hiding this comment

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

cache and timeout as members of the DnsTapClient instance

dnstap.go Outdated

const defaultTimeout = time.Hour

var c = cache.New(defaultTimeout, defaultTimeout*2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should use values derived from the config with sane defaults. The cache lib we use would be more beneficial if we could control the size of the cache and the expiry.

dnstap.go Outdated
}

func (d *DnsTapClient) Receive() {
for {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a defer to close and delete the socket on shutdown

dnstap.go Outdated
if err != nil {
el.Printf("msg.Unpack() failed: %s \n", err)
} else {
for i, r := range m.Answer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is where you want to check and see if any events are waiting for this data to "complete" the event

Copy link
Collaborator

@nbrownus nbrownus Oct 10, 2018

Choose a reason for hiding this comment

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

This is also where you want to try and flush old messages. Actually it might not matter since 1305 shoves events through for us anyways.

dnstap.go Outdated
}

func (d *DnsTapClient) cache(dt *dnstap.Dnstap) {
m := new(dns.Msg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a defer to close the socket

@CLAassistant
Copy link

CLAassistant commented Jul 30, 2019

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants