Add raw layertype for google #324

zecke · 2017-06-06T15:43:59Z

For Elasticsearch's packetbeat it is necessary to expose a LayerTypeRaw. Correct the LinkTypeRaw for mainstream platforms and add a LayerTpeRaw. Update the tests that use LinkTypeRaw right now.

googlebot · 2017-06-06T15:44:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

zecke · 2017-06-06T15:55:51Z

On 6. Jun 2017, at 23:44, googlebot ***@***.***> wrote:

I signed it!

googlebot · 2017-06-06T15:55:54Z

CLAs look good, thanks!

gconnell · 2017-06-07T18:19:06Z

layers/raw.go

@@ -0,0 +1,68 @@
+// Copyright 2012 Google, Inc. All rights reserved.
+// Copyright 2009-2011 Andreas Krennmair. All rights reserved.


this second copyright notice isn't necessary on new files, just the Google line will do.

I looked at the loop.go and decodeIPv4or6 and as I don't know who wrote it in the end copied the above copyright. If you think that Andreas's copyright don't apply then maybe Googles doesn't and I should claim it?

As mentioned elsewhere, let's do "The GoPacket Authors" and make sure you, Andreas, and I at least are in the AUTHORS file.

gconnell · 2017-06-07T18:19:50Z

layers/raw_test.go

@@ -0,0 +1,52 @@
+// Copyright 2017 Holger Hans Peter Freyther, Google, Inc. All rights reserved.


Please copyright to Google as in other files.

Can you please elaborate? Which part do you think is copyrighted by Google?

Looking through the CLA, you're absolutely right. The CLA allows us as Google to use the copyright code, but doesn't transfer the copyright to us as part of the contribution.

Would you mind, for this file, using the following language instead, and adding your name to AUTHORS?:

// Copyright 2014 The GoPacket Authors

I should probably standardize that on all the other files as well.

gconnell · 2017-06-07T18:20:25Z

layers/raw_test.go

+//   	0x0030:  1415 1617 1819 1a1b 1c1d 1e1f 2021 2223  .............!"#
+//   	0x0040:  2425 2627 2829 2a2b 2c2d 2e2f 3031 3233  $%&'()*+,-./0123
+//   	0x0050:  3435 3637                                4567
+var testPacketPacket0 = []byte{


please rename to testPacketRawICMP4 or something like that.

gconnell · 2017-06-07T18:21:12Z

layers/enums.go

 	LinkTypeSLIP           LinkType = 8
 	LinkTypePPP            LinkType = 9
 	LinkTypeFDDI           LinkType = 10
+	LinkTypeRaw            LinkType = 12


I wonder if we should keep Raw at 101 and add Raw2 or something for 12?

I need to see/understand how things will work out if there is not a 1:1 mapping from LinkType* to LayerType. Do you know how it works?

I believe it should be fine... LinkType implements a Decoder, but there's nothing against different link types decoding into the same layer type.

gconnell · 2017-06-07T18:22:14Z

layers/raw.go

+)
+
+// RawIP (DLT_RAW) contains no header and we start with the IP header
+type Raw struct {


This is great code, but I'm not 100% sure its specific purpose... why would this be useful instead of just having a packet with either IPv4 or IPv6 as the first layer?

It models how wireshark displays/shows the frame and it kind of represents the "physical" structure of the frame.

In Elasticsearch's packetbeat the DecodingLayer is used to decode (and not gopacket.Decoder but not sure if that would do)

But do you argue if the LayerTypeRaw should exist at all or if in decodeRawIP it should add its own layer?

gconnell · 2017-06-07T18:25:37Z

layers/enums.go


 const (
-	// According to pcap-linktype(7) and http://www.tcpdump.org/linktypes.html
+	// According to pcap-linktype(7) and http://www.tcpdump.org/linktypes.html with fixes from pcap/bpf.h


Sadly, it appears that raw can be 12 or 14 (https://opensource.apple.com/source/libpcap/libpcap-18/libpcap/pcap-bpf.h) or 101 https://github.com/the-tcpdump-group/libpcap/blob/1a6b088a88886eac782008f37a7219a32b86da45/pcap-common.c#L159. Note that 101 is canonical, since it's how libpcap says "this could be one of a few values". I wonder if we should do:

LinkTypeRaw 101 LinkTypeRawBSD 14 LinkTypeRawLinux 12

Thoughts? Without 101 pointing at raw, pcap files from raw interfaces will no longer work.

It is worth. On BSD it is 12 but on OpenBSD it is 14. On OpenBSD 12 is used for something else.

According to http://www.tcpdump.org/linktypes.html the value is 101 but on FreeBSD/Linux it is 12 and on OpenBSD it 14. Add these as alternative types.

Create a Raw struct and make it implement the LayerType interface. Add a testcase to decode a raw IPv4/ICMPv4 frames. Remove decodeIPv4or6 that was only used for the raw ip link type. Use "The GoPacket Authors" as copyright holder and add myself to the list of contributors.

lhausermann · 2017-07-22T15:55:52Z

@gconnell @zecke What's the status on this ?

zecke · 2017-07-22T20:03:11Z

I think I had addressed all review comments.

gadkrumholz · 2017-08-25T19:17:38Z

I'd like to add that until I merged this PR locally I couldn't get the https://github.com/google/gopacket/blob/master/examples/arpscan/arpscan.go example to receive any ARP packets in Linux (ubuntu 16.04 x64 with libpcap 1.7.4-2) ... I didn't have to change any code in the arpscan example.

Is this going to get merged soon?

Thanks.

notti · 2018-12-21T20:35:02Z

Sorry didn't see this pull request, when I added 0d76b76
Closing, since it is already fixed by this commit.

zecke force-pushed the add-raw-layertype-for-google branch from 8e47e36 to ecb7ab1 Compare June 6, 2017 16:04

tsg mentioned this pull request Jun 7, 2017

Add LayerTypeRaw for LinkTypeRaw link layers tsg/gopacket#9

Open

gconnell reviewed Jun 7, 2017

View reviewed changes

zecke force-pushed the add-raw-layertype-for-google branch from ecb7ab1 to e4082df Compare June 9, 2017 06:51

zecke added 2 commits June 9, 2017 14:56

Add additional LinkTypeRaw* types

c943a6e

According to http://www.tcpdump.org/linktypes.html the value is 101 but on FreeBSD/Linux it is 12 and on OpenBSD it 14. Add these as alternative types.

zecke force-pushed the add-raw-layertype-for-google branch from e4082df to d030929 Compare June 9, 2017 06:56

notti closed this Dec 21, 2018

		@@ -0,0 +1,68 @@
		// Copyright 2012 Google, Inc. All rights reserved.
		// Copyright 2009-2011 Andreas Krennmair. All rights reserved.

		@@ -0,0 +1,52 @@
		// Copyright 2017 Holger Hans Peter Freyther, Google, Inc. All rights reserved.

Add raw layertype for google #324

Add raw layertype for google #324

Uh oh!

Conversation

zecke commented Jun 6, 2017

Uh oh!

googlebot commented Jun 6, 2017

Uh oh!

zecke commented Jun 6, 2017 via email

Uh oh!

googlebot commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zecke Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhausermann commented Jul 22, 2017

Uh oh!

zecke commented Jul 22, 2017

Uh oh!

gadkrumholz commented Aug 25, 2017

Uh oh!

notti commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zecke Jun 8, 2017 •

edited

Loading