DRAFT: ICMP protocol support (issue #250)

[darxriggs]

This is the implementation to support the ICMP protocol. (see issue #250)

ICMP is the 1st non-transport level protocol in packetbeat. So a big question is how to integrate such protocols in the current infrastructure.

Please provide any kind of feedback.

Design Decisions

icmpMessage

• does not contain Tuple
  • Tuple is already part of icmpTransaction
  • this saves memory (as Request & Response would contain a Tuple)

icmpTransaction

• keep it simple/minimal

icmpTuple

• no precomputation of hashableIcmpTuple required as its is only required once per message
  • saves memory (no "raw" and "revRaw" fields in icmpTuple)
  • saves time - 800ns per request-reponse pair
    • "raw" hash only required for request
    • "revRaw" hash only required for response
• based on implementation for DNS protocol
• Hashable() implementation is performance optimized

Icmp

• not filling "transport" field
  • this is for the iso/osi layer "transport" protocol
  • icmp is one layer below
• "bytes_in" and "bytes_out" fields
  • ICMP communication can be initiated from both sides (client and server)
  • therefore it is required to detect the initiator when filling both byte fields
• do not perform the deleteTransaction() operation inside
  publishTransaction() as Cache.Delete() tries to aquire a lock but Cache.Cleanup() already aquired one -> deadlock

BPF Filter

• order of filter tokens is relevant (especially for vlan)

  • also see https://www.snellman.net/blog/archive/2015-05-18-whats-wrong-with-pcap-filters/

  • tested with scapy

    • basic config

      conf.iface='lo'
      conf.iface6='lo'
    

    • scenarios

      • ICMPv4 without VLAN

        sendp(Ether(src='00:00:00:00:00:01', dst='00:00:00:00:00:02')/IP(src='10.0.0.1', dst='10.0.0.2')/ICMP(id=5, seq=1, type=8)/"test")
        sendp(Ether(src='00:00:00:00:00:02', dst='00:00:00:00:00:01')/IP(src='10.0.0.2', dst='10.0.0.1')/ICMP(id=5, seq=1, type=0)/"test")
      

      • ICMPv4 with VLAN

        sendp(Ether(src='00:00:00:00:00:01', dst='00:00:00:00:00:02')/Dot1Q(vlan=10)/IP(src='10.0.0.1', dst='10.0.0.2')/ICMP(id=5, seq=1, type=8)/"test")
        sendp(Ether(src='00:00:00:00:00:02', dst='00:00:00:00:00:01')/Dot1Q(vlan=10)/IP(src='10.0.0.2', dst='10.0.0.1')/ICMP(id=5, seq=1, type=0)/"test")
      

      • ICMPv6 without VLAN

        sendp(Ether(src='00:00:00:00:00:01', dst='00:00:00:00:00:02')/IPv6(src='::1', dst='::2')/ICMPv6EchoRequest(id=5, seq=1)/"test")
        sendp(Ether(src='00:00:00:00:00:02', dst='00:00:00:00:00:01')/IPv6(src='::2', dst='::1')/ICMPv6EchoReply(id=5, seq=1)/"test")
      

      • ICMPv6 with VLAN

        sendp(Ether(src='00:00:00:00:00:01', dst='00:00:00:00:00:02')/Dot1Q(vlan=10)/IPv6(src='::1', dst='::2')/ICMPv6EchoRequest(id=5, seq=1)/"test")
        sendp(Ether(src='00:00:00:00:00:02', dst='00:00:00:00:00:01')/Dot1Q(vlan=10)/IPv6(src='::2', dst='::1')/ICMPv6EchoReply(id=5, seq=1)/"test")
      

• system tests
  • attention: BPF filter cannot be tested via system tests
    • it's evaluated by the kernel
    • so it's only evaluated for online capturing but not for offline pcap files

[monicasarbu]

@darxriggs It's great that you are adding support for the ICMP protocol. Here is a quick tutorial about how to add a new protocol in Packetbeat: https://www.elastic.co/guide/en/beats/packetbeat/current/_developer_guide_adding_a_new_protocol.html. Please let us know if you have any questions.

[darxriggs]

@monicasarbu Thanks for the info. I already read this guide. But as ICMP is neither a TCP nor a UDP protocol the guide is only partly useful.

As already stated by you in issue #267 some parts are already outdated. Publishing a transaction now is done via publisher.Client instead of directly via a channel.

[ruflin]

I would suggest to name it withIcmp. I'm aware that there are still quite a few variables that use _ but we should migrate to the new naming standard camelCase for the new variables (in case it doesn't break something).

[darxriggs]

Ok. I was already wondering what the naming convention is in the project but didn't find a consistent one.

But inside the Config structs you still go with Camel_case so that it's underscores in the .yml config file?

[ruflin]

No, we also plan to go with camelCase in the config. See here for an example: https://github.com/elastic/filebeat/blob/master/etc/filebeat.yml#L43

[darxriggs]

I updated all my "internal" code to use camelCase. The "external" code which is in other files like config.go and protos.go I left as is, not to mix both naming conventions in a single file.

I suggest that the switch from under_score to camelCase is done in one go.

[ruflin]

@darxriggs This looks very promising. It is especially cool that you also added system tests.
@urso Can you also have a look at this as this is completely new protocol?

[urso]

we've got some additions in tsg/gopacket. Longterm we will try to get these back into upstream and update all references thereafter.

[darxriggs]

Yes I know about additions. I also have pull request open (google/gopacket#134) but nobody is looking at it.

[urso]

All in all looks good, but reporting to ES can be improved + really nice to have feature. Thanks for your PR.

[darxriggs]

I'll let you know when I am finished incorporating the review comments and some more tests & cleanup.

[tsg]

@darxriggs Are there still changes you want to do or is this mostly ready to be merged? I'm asking because we are planning to do a reorganization of our repositories this week (merging the official Beats in a single repo). The way we plan to do it will keep this repository alive, so this PR won't be lost or closed.

However, we'll move all packetbeat files in a packetbeat/ folder, so this will mean it will be likely more work to merge it. Let me know what you think, we could merge it and keep improving on it later, but I'm also happy to keep it open and do the rebase work after we change the repos.

[darxriggs]

@tsg Sorry for being so quiet for some time.
I have one more local commit which makes the "local IPs" configurable (as requested by @urso). But I am not really sure if it's worth all the complexity. So I would go without this commit. This means go with the code available in the pull request.

Some final questions:
a) Should I rebase the PR on master and squash everything together or are you doing it?
b) I didn't follow the latest changes in the codebase. So for example the change from field timestamp to @timestamp is missing. I guess there is other things changed. How do we deal with this?

[tsg]

That's great news! In our normal workflow the person who submits the PR does the history cleanup and rebase. But the rebase might be complex in this case due to your point b) and I don't want to put more work on you, so if squashing to a single commit is all you need, I can do that for you tomorrow. Let me know please.

[darxriggs]

Ok. What did I do finally?

• update the initial pull request comment (it's no longer a draft)
• rebase on master
• perform the @timestamp field renaming
• incorporate the transaction_timeout configuration possibility
• squash everything together
• run the tests

I guess now it's up to you.