cisagov / Malcolm

Malcolm is a powerful, easily deployable network traffic analysis tool suite for full packet capture artifacts (PCAP files), Zeek logs and Suricata alerts.
https://cisagov.github.io/Malcolm/
Other
1.96k stars 327 forks source link

Change pcap file upload logging to be more descriptive. #285

Closed Hashfastr closed 1 year ago

Hashfastr commented 1 year ago

πŸ—£ Description

Changing error log for file upload when there's an invalid file type uploaded. Previously just printed a single trashcan emoji (πŸ—‘οΈ), changed to be more descriptive.

πŸ’­ Motivation and context

When debugging why I couldn't upload a PCAP file to Malcolm I kept getting the below error.

WARNING: watch-pcap-uploads-folder.py:  πŸ—‘   /pcap/upload/AUTOSURICATA,AUTOZEEK,all,cloud,azure,USERTAG,cloud.pcapng (application/octet-stream/pcapng capture file - version 1.0)

This is minimally descriptive only providing a trashcan emoji along with the file name and mime type. The message can be assumed that your submitted file was immediately deleted. While the attached filename and mime type seems much more descriptive of what was uploaded, but not to why the error occurred.

This pull request changed this single line error to two much more descriptive errors:

ERROR: watch-pcap-uploads-folder.py: Invalid file type uploaded application/octet-stream/pcapng capture file - version 1.0
ERROR: watch-pcap-uploads-folder.py: Deleting /pcap/upload/AUTOSURICATA,AUTOZEEK,all,cloud,azure,USERTAG,cloud.pcapng

While I like the flair emojis give scripts, putting them into error logs is a misstep and either further obfuscates the error at hand, or makes it much more difficult to parse.

πŸ§ͺ Testing

Splits one print statement into two, removes an emoji, don't really think this applies.

βœ… Pre-approval checklist

βœ… Pre-merge checklist

βœ… Post-merge checklist

Hashfastr commented 1 year ago

Unsure of labels or how to add them at the moment seeing it's greyed out for me.

mmguero commented 1 year ago

Thanks for the suggestion! There is a reason for the emojis: sometimes when I'm developing/debugging artifact processing I'm looking at hundreds or thousands of files, and they're zipping by in the logs quickly. Those icons (which I use across many of the internal malcolm scripts) make it easier to visually identify the operation at a glance.

But I do agree that in this case the message needs to be more helpful to the end user as well. I think I will probably retain the icon but also add a more descriptive error message as you suggested.