1
\$\begingroup\$

I've written a little piece of code to filter Firewall-Connection-Events by a certain group of IPs. But the code can't keep up since the input is fairly huge. I am looking for ways to make this code more efficient in order to handle the Firewall-Connection-Events.

[READ_CLIENTS] The code starts with an empty list (client_IPs). The list is getting filled with unique IPs (read_clients), if the size of the file changes "read_clients" is called again to alter the list.

[READ_EVENTS] To get the events, I've used While True to loop over the event-File and return the events with yield - if there is no new input, then sleep for 0.1 seconds.

[PROCESS_AND_FILTER] After this I loop over the generator object to compare each event to each Unique-IP and seperate the result in two files.

    # -*- coding: utf-8 -*-

from IPy import IP
import os
import time

# Create String-Array to compare
path_unique_ips = '/var/log/unique.log'
# Sophos-UTM Packet-Accepted-Event | Connections to Customer-Net
path_sophos_to_customer = '/var/log/packet-accepted-sophos.log'
# match logs
match_log = '/var/log/matched.log'
no_match_log = '/var/log/not_matched.log'
# IP-Filter-Array
client_IPs = []
#get file size of unique ips file
size_ip_file = os.stat(path_unique_ips).st_size


def read_clients(path_unique_ips):
     client_IPs_file = open(path_unique_ips, "r")
     if client_IPs_file.mode == 'r':
               # read all line from client_IPs_file
               new_client_IPs = client_IPs_file.readlines()
               #check for new clients and fill array
               for new_client_IP in new_client_IPs:
                    if new_client_IP not in client_IPs:
                        client_IPs.append(IP(new_client_IP).strNormal())


def read_events(path_sophos_to_customer):
     connection_event_to_customer = open(path_sophos_to_customer, 'r')
     connection_event_to_customer.seek(0, 2)
     if connection_event_to_customer.mode == 'r':
         while True:
            new_event = connection_event_to_customer.readline()
            if not new_event:
                time.sleep(0.1)
                continue
            yield new_event
            #file size of unique IP File changed, re-run the function read_clients
            if size_ip_file is not os.stat(path_unique_ips).st_size:
                read_clients(path_unique_ips)

def process_and_filter(my_events):
    #get events in generator-object from function read_events
    # loop over generator-object, filled with events
    for new_event in my_events:
        print(new_event)
        # loop over event with all ips
        for client in client_IPs:
            # if client-ip found in event write to match.log and break loop to go for next event
            if client in new_event:
                with open(match_log, 'a+') as matched:
                    matched.write(new_event)
                break
        # if ip wasn't in event write event to no_match.log
        else:
             with open(no_match_log, 'a+') as no_match:
                no_match.write(new_event)


if __name__ == '__main__':
     read_clients(path_unique_ips)
     new_events=read_events(path_sophos_to_customer)
     process_and_filter(new_events)

Log_Event_Example :

Jan 18 14:14:14 17.17.17.17 2019: 01:18-14:14:14 firewall-1 ulogd[5974]: id="2002" severity="info" sys="SecureNet" sub="packetfilter" name="Packet accepted" action="accept" fwrule="653" initf="eth1" outitf="eth0" srcmac="aa:bb:cc:dd:ee:ff" dstmac="00:11:22:33:44:55" srcip="10.10.10.10" dstip="10.10.10.11" proto="6" length="52" tos="0x00" prec="0x00" ttl="127" srcport="58589" dstport="22" tcpflags="ACK"

\$\endgroup\$
1
\$\begingroup\$

The main performance bottleneck is probably the fact that you keep your already seen IPs in a list. Doing x in list is \$\mathcal{O}(n)\$, so the more IPs there are, the slower this will get. Another factor is that you check if new_client_IP not in client_IPs but then add IP(new_client_IP).strNormal() to the list. So even if you come across the same IP again, it will still not be in the list, since that class probably produces something different than the line (which will have a trailing newline, for example) (and if it does not produce anything different, why have this step at all?) and so you do this possibly expensive step again.

Instead just save them in a set, for which in is \$\mathcal{O}(n)\$. And even better, if you add an already existing element to a set it does not care.

client_IPs = set()
client_IPs_size = 0

def read_clients(path_unique_ips):
    """Check if the file has changed size.
    If it has, extract all IPs from it and update the global variables.
    """
    global client_IPs_size
    current size = os.stat(path_unique_ips).st_size
    if current_size != client_IPs_size:
        client_IPs_size = os.stat(path_unique_ips).st_size
        global client_IPs
        with open(path_unique_ips, "r") as client_IPs_file:
            client_IPs = {IP(line).strNormal() for line in client_IPs_file}

Note that I used with to ensure the file is properly closed, even in an event of an exception. I also used a set as stated above.

I also iterate over the file directly instead of first reading all lines into memory. The whole thing fits nicely inside a set comprehension. I also added a docstring to document what this function does.

Modifying global objects can quickly lead to hard to read code and should be avoided if possible. Here this is a bit harder to avoid, because process_and_filter depends on the global state always being correct at that specific point in the loop. Another way around this would be to use class to keep this global state and make the other functions methods of that class. This might be a bit overkill, though, so I leave that implementation to you if you feel it is necessary.


def read_events(path_sophos_to_customer):
    """Every 0.1 seconds check if a new non-blank line has been added to the end of the file.
    If there has, yield it.

    Read the file containing the list of IPs again if it has changed.
    """
    with open(path_sophos_to_customer, 'r') as connection_event_to_customer:
        connection_event_to_customer.seek(0, 2)   # seek to the end
        while True:
            new_event = connection_event_to_customer.readline()
            if not new_event:
                time.sleep(0.1)
                continue
            yield new_event

            # file size of unique IP File changed, this will read the IPs again
            read_clients(PATH_UNIQUE_IPS)

Since the check for a changed client_IPs is now inside read_clients the if clause is not needed anymore.

I also removed the check if the file is readable. Usually you want your code to either fail loudly (with an exception) if something happens (like a file not being readable) or you catch that exception, do something about it and log that it happened. Having it just silently fail by bypassing it with that if clause does not help you at all.


For the splitting of the events into two logs, I would open both files only once at the beginning. I would also extract the IP from the event (since you have not shown us an example of what an event looks like, I will have to guess and say you could use for example re.search(r'\b(?:\d{1,3}\.){3}\d{1,3}\b', new_event).group()). This moves this from having to iterate over all IPs every time to iterating over the string of new_event, which is hopefully shorter.

def process_and_filter(my_events, match_log, no_match_log):
    """Get events in generator-object from function read_events
    and write either to matched or no match log file.
    """
    with open(match_log, 'a+') as match, open(no_match_log, 'a+') as no_match:
        for new_event in my_events:
            print(new_event)
            ip = extract_ip(new_event)
            file = match if ip in client_IPs else no_match
            file.write(new_event)

With this the calling code becomes:

if __name__ == '__main__':
    new_events = read_events(PATH_SOPHOS_TO_CUSTOMER, MATCH_LOG, NO_MATCH_LOG)
    process_and_filter(new_events)

Note that Python's official style-guide, PEP8, recommends using ALL_CAPS for global constants.

\$\endgroup\$
  • \$\begingroup\$ Thanks alot correcting my code - I'll test it asap . \$\endgroup\$ – peacemaker Jan 18 at 10:44
  • \$\begingroup\$ One question - [BEFORE]: check if file_size of unique.log has changed . if so, call read_clients - [AFTER]: call read_clients to check if file_size of unique.log has changed. wasn't it faster before, since the function wasnt called all the time ? \$\endgroup\$ – peacemaker Jan 18 at 10:51
  • \$\begingroup\$ @peacemaker: Calling the function does not have a very high overhead (but it does have some). But I'm pretty sure the disk access to check the file size dominates that. If it is an issue (and you should find out if it is a bottleneck via timing) you can always move it back there. \$\endgroup\$ – Graipher Jan 18 at 11:34
  • \$\begingroup\$ I changed the script as you recommended and it works out just fine . Thanks for your help. \$\endgroup\$ – peacemaker Jan 18 at 14:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.