Implement more detailed bandwidth monitoring

The point here is to provide a more detailed breakdown for the traffic - in particular, it allows seeing HTTP/HTTPS/SSH traffic separately and will also list any unusual traffic (unknown Level 3/Level 4 protocol or unknown port).

[Wladimir Palant, 2013-10-01 18:15:26]

The point here is to provide a more detailed breakdown for the traffic - in
particular, it allows seeing HTTP/HTTPS/SSH traffic separately and will also
list any unusual traffic (unknown Level 3/Level 4 protocol or unknown port).

[Wladimir Palant, 2013-10-02 07:37:08]

Running this tcpdump-based script and bwm-ng in parallel it seems that bwm-ng
doesn't account for the low-level Ethernet overhead that isn't captured by
tcpdump. So I've removed adding these 24 bytes to make the numbers more
comparable. Also, I've removed an unused function and moved normalization of the
numbers to an earlier point to make sure we don't have to do it throughout the
code.

[Wladimir Palant, 2013-10-04 12:49:52]

I forgot to check whether nagios plugins run with root privileges - turns out
they don't. So the current iteration uses sudo to run tcpdump.

Given that we clearly need better records (see claims about participating in a
DNS reflection attack) I've already pushed this change.

[Wladimir Palant, 2013-10-04 12:54:23]

For the record, the changes here turned out to be not sufficient - pnp4nagios
reacts badly when new values are being added to an existing plugin. The RRD
files had to be updated "manually" with the following script:

import os, subprocess

def convertfile(path):
  rrdfile = os.path.join(path, "Bandwidth.rrd")
  if not os.path.exists(rrdfile):
    return
  data, _ = subprocess.Popen(["rrdtool", "dump", rrdfile],
stdout=subprocess.PIPE).communicate()

  ds = []
  prep = []
  row = []
  for i in range(3, 13):
    ds.append('''
      <ds>
        <name>%i</name>
        <type> GAUGE </type>
        <minimal_heartbeat>8460</minimal_heartbeat>
        <min>NaN</min>
        <max>NaN</max>

        <!-- PDP Status -->
        <last_ds> UNKN </last_ds>
        <value>0</value>
        <unknown_sec> 0 </unknown_sec>
      </ds>''' % i)
    prep.append('''
      <ds>
        <value>NaN</value>
        <unknown_datapoints>0</unknown_datapoints>
      </ds>''')
    row.append('<v>NaN</v>')
  data = data.replace('<!-- Round Robin Archives -->', ''.join(ds) + '<!-- Round
Robin Archives -->')
  data = data.replace('</cdp_prep>', ''.join(prep) + '</cdp_prep>')
  data = data.replace('</row>', ''.join(row) + '</row>')

  os.rename(rrdfile, rrdfile + "~")
  subprocess.Popen(["rrdtool", "restore", "-", rrdfile],
stdin=subprocess.PIPE).communicate(data)

basedir = '/var/lib/pnp4nagios/perfdata'
for file in os.listdir(basedir):
  path = os.path.join(basedir, file)
  if file.startswith(".") and not os.path.isdir(path):
    continue
  convertfile(path)

[Wladimir Palant, 2013-10-09 07:39:05]

It wasn't really obvious but tcpdump has a parameter combination that makes it
stop after a certain number of seconds - I'm now using that instead of parsing
times in the script. Also, I increased the NRPE timeout for the bandwidth check.
It needs to sniff 5 seconds of traffic and process the data, on some busy
servers 10 seconds aren't always enough for that.

[Felix Dahlke, 2013-10-10 08:29:07]

http://codereview.adblockplus.org/12375002/diff/16001/manifests/monitoringser...
File manifests/monitoringserver.pp (right):

http://codereview.adblockplus.org/12375002/diff/16001/manifests/monitoringser...
manifests/monitoringserver.pp:146: check_command =>
'check_nrpe_timeout!check_bandwidth!20',
Why does this need a timeout now?

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
File modules/nagios/files/check_bandwidth (right):

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:21: return fcntl.ioctl(s.fileno(), 0x8927,
struct.pack("24s", "eth0"))[18:24]
I'm pretty sure this will only work on Linux like that. Which is fine in our
case I guess, but we could get a portable implementation like this:

from uuid import getnode
getnode()

(From: http://stackoverflow.com/a/159195/606036)

That way we would have to rely on the internals of the standard library, which
might be worse than not being portable.

If we keep this, I'd like a constant for the magic number just for clarity:

SIOCGIFHWADDR = 0x8927 # See man ioctl_list

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:47: magic_number, _, _, _, _, _, _ =
struct.unpack("IHHiIII", process.stdout.read(24))
I'd find this more readable if the result of process.stdout was saved into a
variable, as below. Preferably "global_header" which would be in line with the
docs. Or pcap_hdr maybe.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:49: raise Exception("Unexpected format")
"Unsupported byte order" or something along those lines would be more precise.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:51: header = process.stdout.read(16)
header -> record_header?

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:57: length = float(orig_len * 8) / INTERVAL
length -> bits_per_second?

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:66: destination, source, protocol =
struct.unpack("!6s6sH", payload[:14])
1. Shouldn't the offset be 8 instead of 14? What are the first 6 bytes before
the preamble?

2. Is it safe to ignore the optional 802.1Q tag field here?

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:67: payload = payload[14:]
The payload should be the field after EtherType according to Wikipedia, while
[14:] is the preamble plus 6 bytes preceeding it. Worth a comment I think, since
the libpcap docs don't seem to mention it.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:73: ihl = ord(payload[0]) & 0xF
Shouldn't it be 0x4? 0xF would get us both version and IHL I think.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:85: # The lower port number should be the
real port, the other one will be
I think this comment should move down a bit, on top of the line it relates to.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:89: port = min(source_port,
destination_port)
Why not do this based on the direction?

port = source_port if direction == "rx" else destination_port

[Wladimir Palant, 2013-10-10 09:37:46]

I've addressed the comments unless noted otherwise. Also, the previous changeset
didn't include a sudoers change that became necessary.

http://codereview.adblockplus.org/12375002/diff/16001/manifests/monitoringser...
File manifests/monitoringserver.pp (right):

http://codereview.adblockplus.org/12375002/diff/16001/manifests/monitoringser...
manifests/monitoringserver.pp:146: check_command =>
'check_nrpe_timeout!check_bandwidth!20',
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> Why does this need a timeout now?

There is always a timeout, the default is 10 seconds - which is too little for
bandwidth monitoring. tcpdump needs to run 5 seconds, plus the processing of the
data needs to happen. As I said, on some busy servers this doesn't finish in 10
seconds.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
File modules/nagios/files/check_bandwidth (right):

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:21: return fcntl.ioctl(s.fileno(), 0x8927,
struct.pack("24s", "eth0"))[18:24]
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> from uuid import getnode
> getnode()
> 
> (From: http://stackoverflow.com/a/159195/606036)

Yes, I've seen that. This function looks very much like an implementation
detail. It also has some drawbacks, from documentation: "The first time this
runs, it may launch a separate program, which could be quite slow" and "on a
machine with multiple network interfaces the MAC address of any one of them may
be returned." I'd rather have a more reliable solution even if it isn't
portable.

> If we keep this, I'd like a constant for the magic number just for clarity:
> 
> SIOCGIFHWADDR = 0x8927 # See man ioctl_list

Sure, I don't mind that. Note that this won't eliminate the need for an extended
comment - there are lots of things hardcoded here beyond that constant.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:49: raise Exception("Unexpected format")
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> "Unsupported byte order" or something along those lines would be more precise.

No, there can be no other byte order - this script and tcpdump run on the same
machine so the native byte order cannot be different between them. A different
magic number is really tcpdump producing some unexpected format.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:57: length = float(orig_len * 8) / INTERVAL
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> length -> bits_per_second?

bps?

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:66: destination, source, protocol =
struct.unpack("!6s6sH", payload[:14])
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> 1. Shouldn't the offset be 8 instead of 14? What are the first 6 bytes before
> the preamble?

14 isn't offset but length here. The Ethernet frame header as captured by
tcpdump has length 14 - preamble and start of frame delimiter aren't being
captured, interframe gap isn't either.

> 2. Is it safe to ignore the optional 802.1Q tag field here?

I guess that our servers won't participate in a VLAN any time soon, at least not
willingly (802.1Q tag could still be used between the routers of the network
infrastructure, it just won't make it to the server). We couldn't test whether
we handle the 802.1Q tag correctly anyway. Worst thing that could happen: we
would misclassify all traffic as L3 0x8100.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:67: payload = payload[14:]
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> The payload should be the field after EtherType according to Wikipedia, while
> [14:] is the preamble plus 6 bytes preceeding it. Worth a comment I think,
since
> the libpcap docs don't seem to mention it.

As with the previous comment, preamble isn't being captured. I'll add a comment
on that.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:73: ihl = ord(payload[0]) & 0xF
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> Shouldn't it be 0x4? 0xF would get us both version and IHL I think.

No, that's correct - "& 0xF0" gives you the first 4 bits, "& 0x0F" the last 4
bits.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:89: port = min(source_port,
destination_port)
On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> Why not do this based on the direction?
> 
> port = source_port if direction == "rx" else destination_port

Because our servers can open connections as well - e.g. they will do DNS and SSH
requests. Then the ephemeral port will be on our end.

[Felix Dahlke, 2013-10-10 09:44:34]

LGTM

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
File modules/nagios/files/check_bandwidth (right):

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:57: length = float(orig_len * 8) / INTERVAL
On 2013/10/10 09:37:46, Wladimir Palant wrote:
> On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> > length -> bits_per_second?
> 
> bps?

Sure.

http://codereview.adblockplus.org/12375002/diff/16001/modules/nagios/files/ch...
modules/nagios/files/check_bandwidth:73: ihl = ord(payload[0]) & 0xF
On 2013/10/10 09:37:46, Wladimir Palant wrote:
> On 2013/10/10 08:29:08, Felix H. Dahlke wrote:
> > Shouldn't it be 0x4? 0xF would get us both version and IHL I think.
> 
> No, that's correct - "& 0xF0" gives you the first 4 bits, "& 0x0F" the last 4
> bits.

Um, yes, I confused something there, it's the number of bits of course :)