Add DHCP protocol handling for packetbeat #7359

brianwaskiewicz · 2018-06-19T01:20:38Z

First at contributing to packetbeat; this adds support for parsing DHCP messages.

elasticmachine · 2018-06-19T01:20:40Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2018-06-19T01:20:47Z

packetbeat/protos/dhcp/dhcp.go

+	protos.Register("dhcp", New)
+}
+
+func New(


exported function New should have comment or be unexported

houndci-bot · 2018-06-19T01:23:49Z

packetbeat/protos/dhcp/dhcp.go

+	protos.Register("dhcp", New)
+}
+
+// Creates and returns a new dhcpPlugin.


comment on exported function New should be of the form "New ..."

andrewkroh · 2018-06-19T13:24:38Z

Thanks for submitting this pull request.

The PR is currently failing the initial CI smoke tests because the code needs to be formatted. If you run make fmt and make update that should hopefully fix things.

Secondly when adding a new protocol we also like to have some "system tests" in place that execute Packetbeat against a PCAP file containing DHCP traffic. These tests should perform some assertions on the output events. It's like end-to-end test for Packetbeat. Those tests are contained in packetbeat/tests/system. You can use one of the existing tests, such as dns, as an example to guide you.

brianwaskiewicz · 2018-06-20T03:27:57Z

@andrewkroh Added a test and fixed the formatting. I think the make check is failing because the fields.go and fields.asciidoc need to be updated. Any pointers on the best way to do that?

…

________________________________ From: Andrew Kroh <[email protected]> Sent: Tuesday, June 19, 2018 6:24:57 AM To: elastic/beats Cc: brianwaskiewicz; Author Subject: Re: [elastic/beats] Add DHCP protocol handling for packetbeat (#7359) Thanks for submitting this pull request. The PR is currently failing the initial CI smoke tests because the code needs to be formatted<https://travis-ci.org/elastic/beats/jobs/393907452#L592-L594>. If you run make fmt and make update that should hopefully fix things. Secondly when adding a new protocol we also like to have some "system tests" in place that execute Packetbeat against a PCAP file containing DHCP traffic. These tests should perform some assertions on the output events. It's like end-to-end test for Packetbeat. Those tests are contained in packetbeat/tests/system<https://github.com/elastic/beats/tree/master/packetbeat/tests/system>. You can use one of the existing tests, such as dns<https://github.com/elastic/beats/blob/master/packetbeat/tests/system/test_0032_dns.py>, as an example to guide you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#7359 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJbqn59C0ZxXtGHNjx738UPEff3G6jpiks5t-PupgaJpZM4UstEw>.

andrewkroh · 2018-06-20T03:29:49Z

If you run make update that should regenerate those files and include your new DHCP content.

andrewkroh · 2018-06-21T02:17:26Z

jenkins test this

adriansr

Thanks for your contribution!

There are a few issues I'd like to see improved, have a look at the comments I left.

adriansr · 2018-07-18T10:54:02Z

packetbeat/protos/dhcp/_meta/fields.yml

+      type: group
+      fields:
+        - name: transaction_id
+          type: string


string is not a valid type in Elasticsearch, this must be keyword.

adriansr · 2018-07-19T08:31:25Z

packetbeat/protos/dhcp/dhcp.go

+	for idx < len(payload) {
+		result = append(result, []byte{payload[idx], payload[idx+1], payload[idx+2], payload[idx+3]})
+		idx += 4
+	}


I don't like this function much. It's unnecessary allocation that is happening for every DHCP packet.
All the uses of this function's result can be replaced by slicing pkt.Payload:
rows[3] => pkt.Payload[12:16]
combineRows(rows,11,16) => pkt.Payload[44:108]

Also the bounds check is wrong and will cause a panic for a payload whose size is not a multiple of 4.
for idx + 4 <= len(payload) or similar should do.

adriansr · 2018-07-19T08:32:44Z

packetbeat/protos/dhcp/dhcp.go

+func bytesToIPv4(bytes []byte) string {
+	ip := net.IPv4(bytes[0], bytes[1], bytes[2], bytes[3])
+	str := ip.String()
+	if str == "0.0.0.0" {


Instead of comparing with "0.0.0.0" string, use ip.IsUnspecified()

adriansr · 2018-07-19T08:51:13Z

packetbeat/protos/dhcp/dhcp.go

+
+func trimNullBytes(b []byte) []byte {
+	index := bytes.Index(b, []byte{0})
+	return b[0:index]


This will panic if for some reason the byte 0 is not found within the slice. You need to validate the result of bytes.Index.

Nit: use bytes.IndexByte

adriansr · 2018-07-19T08:54:14Z

packetbeat/protos/dhcp/dhcp.go

+
+func (dhcp *dhcpPlugin) parsePacket(pkt *protos.Packet) beat.Event {
+	dhcpFields := make(map[string]interface{})
+	rows := payloadToRows(pkt.Payload)


This function needs bounds checking, make sure that Payload has the expected length.

adriansr · 2018-07-19T09:21:29Z

packetbeat/protos/dhcp/dhcp.go

+	hwaddr := combineRows(rows, 7, 4)
+	dhcpFields["client_hwaddr"] = bytesToHardwareAddress(hwaddr, int(pkt.Payload[2]))
+	serverName := trimNullBytes(combineRows(rows, 11, 16))
+	dhcpFields["server_name"] = string(serverName)


Is this "server_name" actually used in DHCP or is it a BOOTP leftover? I couldn't find much information about it and in my captures is always empty. I suggest you don't set the field at all in dhcpFields if empty.

adriansr · 2018-07-19T09:28:27Z

packetbeat/protos/dhcp/dhcp.go

+				idx = len(options)
+			} else {
+				idx++
+				remaining = int(options[idx])


Missing bounds check here, it can panic on a malformed or crafted packet.

adriansr · 2018-07-19T09:50:52Z

packetbeat/protos/dhcp/dhcp.go

+		idx++
+	}
+	return result
+}


Same as just using payload[240:], don't need a function for this unless there is a point in copying the slice, which you can do with copy(...)

adriansr · 2018-07-19T09:53:17Z

packetbeat/protos/dhcp/dhcp.go

+			}
+		} else {
+			body = append(body, options[idx])
+			remaining--


This function can be improved to read the option at once instead of looping and appending every byte:

remaining = int(options[idx]) if idx + remaining + 1 > len(options) { error... } result[option] = options[idx+1:idx+1+remaining]

As before, I'm not sure if it's necessary to copy into a new slice or not.

adriansr · 2018-07-19T09:59:44Z

I'm worried that this protocol can create a lot of noise on a busy network.

What do you think about adding a config option to only report dhcp messages based on their type. Something like:

report_types: [ 'DHCPACK', 'DHCPNACK' ]

So that only those two types get reported.

Also I'm not 100% sure which events would be interesting. Do ACK and NACK contain all the information (IP assignments) and do all transactions end in ACK or NACK?

andrewkroh · 2018-07-19T15:03:32Z

For transacted protocols like this, where there is a request/response and transaction ID to link the two packets, Packetbeat usually combines the whole transaction into a single event.

I think we should keep it as you have it for DHCP and not combine the request and responses. There are scenarios in DHCP where a client sends only a single message, like a DHCPDECLINE, and does not expect a response. It would be simpler to not have to put any protocol state knowledge into the code.

Cons of Combining Request/Response

The src and dest IPs used in the two packets of the transaction are not the same. The client uses src 0.0.0.0 and broadcasts to 255.255.255.255. And the server responds to client's assigned address.
A request might receive multiple responses if there is more than one DHCP server. Handling this would be messy in the schema (arrays of objects).

Pros

With transactions we can detect error scenarios where the client never received a response and indicate this is the event. However this can be handled in other ways such as with a flag in the request event or by use Watcher.
We can compute a response time.

andrewkroh

The main packetbeat config files also need to be updated to include the dhcp proto. Those are in packetbeat/_meta/beat*yml.

andrewkroh · 2018-07-18T22:40:09Z

packetbeat/protos/dhcp/config.go

+}
+
+var (
+	defaultConfig = dhcpConfig{}


This should be initialized with the default ports.

defaultConfig = dhcpConfig{ ProtocolCommon: config.ProtocolCommon{ Ports: []int{67, 68}, }, }

andrewkroh · 2018-07-19T01:00:10Z

packetbeat/protos/dhcp/dhcp.go

+}
+
+func (dhcp *dhcpPlugin) ParseUDP(pkt *protos.Packet) {
+	event := dhcp.parsePacket(pkt)


There should be some error handling here. If the data isn't actually DHCP (like there aren't enough bytes make a valid DHCP packet) then an event should not be sent.

andrewkroh · 2018-07-19T13:17:31Z

packetbeat/protos/dhcp/_meta/fields.yml

+          type: string
+          description: The IP address of the DHCP server that the client should use for the next step in the bootstrap process.
+
+        - name: gateway_ip


I think a better name for this would be relay_ip.

andrewkroh · 2018-07-19T13:27:45Z

packetbeat/protos/dhcp/_meta/fields.yml

+          type: string
+          description: The type of hardware used for the local network (Ethernet, LocalTalk, etc)
+
+        - name: message_type


I think message_type, server_identifier, and subnet_mask should be prefixed with option. to highlight these are options in the DHCP message.

andrewkroh · 2018-07-19T13:49:17Z

packetbeat/protos/dhcp/dhcp.go

+		Timestamp: pkt.Ts,
+		Fields: map[string]interface{}{
+			"transport":   "udp",
+			"ip":          pkt.Tuple.DstIP.String(),


I believe that the ip/port and client_ip/client_port values need to be swapped for the BOOTREPLY packets in order or accurately represent the role of the two hosts in the exchange.

andrewkroh · 2018-07-19T14:00:55Z

packetbeat/protos/dhcp/dhcp.go

+	dhcpFields := make(map[string]interface{})
+	rows := payloadToRows(pkt.Payload)
+	dhcpFields["transaction_id"] = hex.EncodeToString(rows[1])
+	dhcpFields["client_ip"] = bytesToIPv4(rows[3])


Several of these IPs may be unspecified. We should omit them if they are unset in the message.

andrewkroh · 2018-07-19T14:03:23Z

packetbeat/protos/dhcp/dhcp.go

+	dhcpFields["client_hwaddr"] = bytesToHardwareAddress(hwaddr, int(pkt.Payload[2]))
+	serverName := trimNullBytes(combineRows(rows, 11, 16))
+	dhcpFields["server_name"] = string(serverName)
+	dhcpFields["op_code"] = int(pkt.Payload[0])


It would be nice to render this value as a string like BootRequest and BootReply.

andrewkroh · 2018-07-19T15:15:18Z

I think it would make sense to call this dhcpv4 because we will likely want to add a dhcpv6 in the future. And dhcpv6 would be a separate protocol operating on separate ports.

andrewkroh · 2018-07-20T02:12:58Z

@brianwaskiewicz I expanded on your pull request in #7647. I tried a third-party library for the parsing.

andrewkroh · 2018-08-13T16:00:23Z

DHCP support has been merged in #7647.

@brianwaskiewicz, you're listed as a co-author in the commit. Thanks!

Add DHCP protocol handling for packetbeat

5e36e85

houndci-bot reviewed Jun 19, 2018

View reviewed changes

add comment

e80125f

houndci-bot reviewed Jun 19, 2018

View reviewed changes

add comment

ac054a9

ruflin added review Packetbeat labels Jun 19, 2018

brianwaskiewicz added 3 commits June 19, 2018 17:52

formatting

03f9f21

add test

88a3cf9

push updated list.go

bf01234

brianwaskiewicz added 2 commits June 19, 2018 21:05

regenerate fields.go

7616f17

fix test

d4172e7

brianwaskiewicz added 2 commits June 20, 2018 19:18

updated config

d9f14ad

add status field

6dc1256

andrewkroh requested a review from adriansr July 17, 2018 22:22

adriansr suggested changes Jul 19, 2018

View reviewed changes

adriansr added the enhancement label Jul 19, 2018

andrewkroh requested changes Jul 19, 2018

View reviewed changes

andrewkroh mentioned this pull request Jul 19, 2018

Add DHCPv4 protocol to Packetbeat #7647

Merged

andrewkroh closed this Aug 13, 2018

andrewkroh mentioned this pull request Aug 29, 2018

Cherry-pick #7647 to 6.x: Add DHCPv4 protocol to Packetbeat #8149

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DHCP protocol handling for packetbeat #7359

Add DHCP protocol handling for packetbeat #7359

brianwaskiewicz commented Jun 19, 2018

elasticmachine commented Jun 19, 2018

houndci-bot Jun 19, 2018

houndci-bot Jun 19, 2018

andrewkroh commented Jun 19, 2018

brianwaskiewicz commented Jun 20, 2018 via email

andrewkroh commented Jun 20, 2018

andrewkroh commented Jun 21, 2018

adriansr left a comment

adriansr Jul 18, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr Jul 19, 2018

adriansr commented Jul 19, 2018

andrewkroh commented Jul 19, 2018 •

edited

Loading

andrewkroh left a comment

andrewkroh Jul 18, 2018

andrewkroh Jul 19, 2018

andrewkroh Jul 19, 2018

andrewkroh Jul 19, 2018

andrewkroh Jul 19, 2018

andrewkroh Jul 19, 2018

andrewkroh Jul 19, 2018

andrewkroh commented Jul 19, 2018

andrewkroh commented Jul 20, 2018

andrewkroh commented Aug 13, 2018

Add DHCP protocol handling for packetbeat #7359

Add DHCP protocol handling for packetbeat #7359

Conversation

brianwaskiewicz commented Jun 19, 2018

elasticmachine commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Jun 19, 2018

brianwaskiewicz commented Jun 20, 2018 via email

andrewkroh commented Jun 20, 2018

andrewkroh commented Jun 21, 2018

adriansr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriansr commented Jul 19, 2018

andrewkroh commented Jul 19, 2018 • edited Loading

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Jul 19, 2018

andrewkroh commented Jul 20, 2018

andrewkroh commented Aug 13, 2018

andrewkroh commented Jul 19, 2018 •

edited

Loading