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.

Sign up for GitHub

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

Jump to bottom

Add DHCPv4 protocol to Packetbeat #7647

Merged
merged 2 commits into from
Aug 9, 2018
Merged

Add DHCPv4 protocol to Packetbeat #7647

merged 2 commits into from
Aug 9, 2018

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Jul 19, 2018 •
edited
Loading

Packetbeat will capture and index individual DHCP packets for IPv4.

Parsing is provided by https://github.com/insomniacslk/dhcp.

This expands the work done in #7359.

Packetbeat DHCPv4 Overview Dashboard

dashboard-packetbeat-dhcpv4

I created a sample watch to alert when new clients are detected on the network: https://gist.github.com/andrewkroh/b8fe93c0dead7eb963e37ac4ca1a332a

packetbeat-new-dhcp-client-detected

Note to self: https://play.golang.com/p/rHecAoaBmI9

@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch from e2eff1e to 954398b Compare July 20, 2018 02:05
@andrewkroh andrewkroh mentioned this pull request Jul 20, 2018
Closed
@andrewkroh
Copy link
Member Author

I've been debating whether or not correlate request and responses.

"When did a client make a request and not get a response?" - This is a question you cannot answer without correlation. And we don't get a response time metric either.

I think response times could be measured for both discover/offer and request/ack exchanges.

  • DHCPDISCOVER -> DHCPOFFER (multiple offers are possible)
    • Send one event for each offer so that we report the response time for each server.
  • DHCPREQUEST -> DHCPACK/NAK (should be just one)
  • DHCPDECLINE (no measurement)
  • DHCPRELEASE (no measurement)

But I think it could be a while before I personally have time to focus on this more. So perhaps we open an issue and track correlation as an enhancement and see if someone wants to take it on?

houndci-bot
houndci-bot reviewed Jul 20, 2018
View reviewed changes
packetbeat/magefile.go Outdated
@@ -118,6 +119,20 @@ func Update() error {
return sh.Run("make", "update")
}

func Fields() error {
Copy link

Choose a reason for hiding this comment

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

exported function Fields should have comment or be unexported

@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch 6 times, most recently from ec69ce1 to 3b80e39 Compare July 21, 2018 02:50
@andrewkroh
Copy link
Member Author

I believe that the Windows build is failing because fields.yml is not generated during CI. #7670 fixes the issue.

I don't understand how the system tests that validate fields names against fields.yml have been working or why it began failing in this PR.

@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch 2 times, most recently from 9b8fd6f to 5eab323 Compare July 30, 2018 22:44
@andrewkroh
Copy link
Member Author

This is passing CI now, conflicts are resolved, and it's ready for review.

exekias
exekias approved these changes Aug 1, 2018
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM, This is a great addition!

Do we flag new protocols as Experimental/Beta in Packetbeat?

Sorry, you will need to run make update.

@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch from 5eab323 to 4e5e391 Compare August 1, 2018 15:09
@andrewkroh
Copy link
Member Author

Do we flag new protocols as Experimental/Beta in Packetbeat?

We haven't done that before. But that did remind me to update the documentation to include DHCP.

@andrewkroh
Copy link
Member Author

I'd like to do a squash this myself before merging to ensure that Co-authored-by: is added for @brianwaskiewicz since I used his pull-request as my starting point.

@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch from 6390d24 to 22c2826 Compare August 3, 2018 17:29
@andrewkroh
Copy link
Member Author

I squashed it into two commits. One for vendor and one with our code.

Please use Rebase and Merge.

@adriansr adriansr self-requested a review August 7, 2018 13:12
adriansr
adriansr approved these changes Aug 9, 2018
View reviewed changes
Copy link
Contributor

@adriansr adriansr left a comment

Choose a reason for hiding this comment

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

LGTM

I think it's a good idea to create a separate issue to add the correlation

packetbeat/protos/dhcpv4/option_ip_addresses.go Outdated
if len(data) < 2+length {
return nil, dhcpv4.ErrShortByteStream
}
servers := make([]net.IP, 0, length%4)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think you want length / 4 here

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. Good catch. I think I was looking at one of the parsers inside of the dhcp lib when I "wrote" that.

Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed that line and squashed the commit.

andrewkroh and others added 2 commits August 9, 2018 10:14
@andrewkroh
Add github.com/insomniacslk/dhcp to vendor
514796e 
Added a build tag client.go to fix windows.
@brianwaskiewicz @andrewkroh
Add DHCPv4 protocol to Packetbeat
92cb11d 
Packetbeat will capture and index individual DHCP packets for IPv4.

This adds a dashboards too.

Parsing is provided by github.com/insomniacslk/dhcp.

Co-authored-by: Brian Waskiewicz <brian_waskiewicz@hotmail.com>
@andrewkroh andrewkroh force-pushed the feature/packetbeat-dhcpv4 branch from 22c2826 to 92cb11d Compare August 9, 2018 14:15
@adriansr adriansr merged commit 6eb21a4 into elastic:master Aug 9, 2018
@andrewkroh
Copy link
Member Author

This needs back-porting to 6.x so that it gets included in the next minor (v6.5.0). 6.4.0 is already feature frozen.

@andrewkroh andrewkroh added the needs_backport PR is waiting to be backported to other branches. label Aug 13, 2018
@andrewkroh andrewkroh mentioned this pull request Aug 13, 2018
Closed
@andrewkroh andrewkroh mentioned this pull request Aug 29, 2018
Merged
@andrewkroh andrewkroh added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 29, 2018
andrewkroh added a commit that referenced this pull request Sep 11, 2018
@andrewkroh
Cherry-pick #7647 to 6.x: Add DHCPv4 protocol to Packetbeat (#8149)
3cc460a 
* Add github.com/insomniacslk/dhcp to vendor

Added a build tag client.go to fix windows.

(cherry picked from commit 6565915)

* Add DHCPv4 protocol to Packetbeat

Packetbeat will capture and index individual DHCP packets for IPv4.

This adds a dashboards too.

Parsing is provided by github.com/insomniacslk/dhcp.

Co-authored-by: Brian Waskiewicz <brian_waskiewicz@hotmail.com>
Co-authored-by: Andrew Kroh <andrew.kroh@elastic.co>
(cherry picked from commit 6eb21a4)
@andrewkroh andrewkroh deleted the feature/packetbeat-dhcpv4 branch November 8, 2018 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@adriansr adriansr adriansr approved these changes

@exekias exekias exekias approved these changes

Assignees
No one assigned
Labels
Packetbeat review v6.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andrewkroh @exekias @houndci-bot @adriansr @brianwaskiewicz