search

cloudflare / goflow

The high-scalability sFlow/NetFlow/IPFIX collector used internally at Cloudflare.
BSD 3-Clause "New" or "Revised" License
859 stars 172 forks source link

Fix reading PPTP packet samples in sFlow parser #51

Closed tamihiro closed 4 years ago

tamihiro commented 4 years ago

Without this patch, the header of a PPTP (PPP over GRE) packet sample is incorrectly parsed, like SrcAddr and DstAddr end up as nil.

lspgn commented 4 years ago

Hello @tamihiro, Thank you for the PR. Could you provide an sFlow or PPP packet capture for testing?

lspgn commented 4 years ago

Okay I went through your changes and tested with packet capture. Didn't take into account the non-Ethernet case in GRE.

I believe you can achieve the same thing by setting: hasEncap = true inside the following condition:

if (etherType[0] == 0x8 && etherType[1] == 0x0) ||
(etherType[0] == 0x86 && etherType[1] == 0xdd) {

I'm still not satisfied on how it should be done. Tried something in the following branch: https://github.com/cloudflare/goflow/compare/fix/gre-decoding

tamihiro commented 4 years ago

Tried something in the following branch:

The way you fixed it looks much nicer than my PR, and I've tested with it and achieved the same result, with the only obvious exception of HasEncap field being false when it parses PPTP packets. As long as you mean the value of this field holds no significance other than matching two if conditions within the code, I'm fine with your fix.

lspgn commented 4 years ago

Your PR was very useful: thank you again. We use HasEncap internally to say that the packet may have more layers on top of others (not just the simple ETH+IP+TCP). I'll see if I can improve it later on: decoding more layers into the PPP (but we're entering the case of "which IP layer do we keep").

I'll add the decoding of the PPP layer that encapsulates IPv4/IPv6.