[krishsharma-code]

Summary

Daily code-quality audit of python_scripts/auth_log_parser.py. The script counts failed SSH authentications per source address, but it treated log lines as trusted data and used a token split to find the address. This PR rebuilds the parser around an explicit validation gate (ipaddress), bounds the memory it can be made to consume, and adds a 25-case test suite.

Linked Issue

Closes # — (scheduled audit run; no tracking issue raised)

Type of Change

• New script
• Bug fix
• Documentation update
• Refactor (no behaviour change)
• Chore / tooling / CI

What Changed

• Address extraction replaced. parts.index('from') → anchored regexes for the OpenSSH Failed password … from <addr> format and the PAM authentication failure; … rhost=<addr> format. The old split returned the wrong token whenever from also appeared in the username, and it silently dropped every PAM-format line (they have no from token at all).
• Validation gate. Every candidate address goes through ipaddress.ip_address(); only a real IPv4/IPv6 literal is ever stored, counted or printed. IPv4-mapped IPv6 peers (::ffff:192.168.1.50) are collapsed to plain IPv4 so one host is not counted twice.
• Memory bounds. The per-address table is capped at MAX_TRACKED_ADDRESSES (10 000) with attempts beyond it tallied as overflow; lines are truncated at MAX_LINE_CHARS (8 192) before matching. A log padded with spoofed sources or one enormous line can no longer grow memory without limit.
• Failure handling. exists()-then-open() (a TOCTOU race) replaced with a single try/except OSError; files open with errors="replace" so undecodable bytes degrade instead of aborting the run.
• CLI + exit codes. argparse with --log, --threshold, --top, --demo and --help; exit 0 clean, 1 threshold reached, 2 unreadable. The demo log is only written when it does not already exist, so the old unconditional open(..., 'w') on mock_auth.log can no longer clobber a real log.
• Docs. Module docstring states the threat model; every function has a docstring; the non-obvious decisions (cap, truncation, mapped-address collapse) are commented.
• Tests. New tests/test_auth_log_parser.py — 25 cases across detection, injection, memory bounds, failure handling, reporting and the exit-code contract.

New or Modified Scripts

Script	Language	Change
python_scripts/auth_log_parser.py	Python	Hardened parsing, bounded memory, argparse CLI, exit codes
tests/test_auth_log_parser.py	Python	New test suite (25 cases)

Safety Review

• This script only performs actions the user explicitly requests
• Any destructive operation requires an explicit flag and supports --dry-run (n/a — read-only; --demo writes only to a non-existent file)
• Any state-changing action is printed before it is carried out
• No credentials, API keys or tokens are committed
• No hardcoded personal or machine-specific paths

Testing Checklist

• python3 -m py_compile python_scripts/*.py passes
• flake8 python_scripts --select=E9,F63,F7,F82,F821,F823 --max-line-length=120 passes
• for f in bash_scripts/*.sh; do bash -n "$f"; done passes (unchanged by this PR)
• shellcheck bash_scripts/*.sh reports no errors (advisory, unchanged)
• python3 -m pytest tests/ -q → 25 passed
• I ran the new or changed script end to end on a machine I am permitted to test (--demo)
• I tested the --help output
• I tested the failure path (missing file, directory as log, undecodable bytes, non-positive --threshold)
• No new console warnings or unhandled errors

Documentation

• I added the script to the index in README.md in the correct numeric position — the index entry already exists at line 218; its description is now slightly stale ("from a mock auth.log"). The README edit was blocked by an API-side size limit on this run and is left as a follow-up.
• The script has a module docstring / header comment explaining what it does
• Non-obvious logic is commented

Code Quality Checklist

• My code follows the standards in CONTRIBUTING.md
• Imports are at the top of the file, not inside functions
• I have performed a self-review of my own code
• I have removed debugging code and unused imports

Additional Notes

Two genuine defects fixed (both behaviour-preserving in the sense that the new output is a superset of the old for well-formed logs, and strictly more correct for PAM lines):

1. PAM authentication failure lines were matched by the detector but then dropped by the address splitter, so sudo/login failures never appeared in the report.
2. A username containing from caused the wrong token to be taken as the address.

Known limitation: addresses beyond the 10 000-entry cap are only counted in aggregate, not listed. That is deliberate — listing them is exactly the memory-exhaustion path being closed.

[krishsharma-code]

Review: python_scripts/auth_log_parser.py

Scope reviewed: the file as it stands on main (53 lines), the full rewrite on this branch, the new tests/test_auth_log_parser.py (25 cases), and the repo conventions in CONTRIBUTING.md and the PR template.

What was reviewed

Security boundaries (input validation, injection, privilege handling), memory efficiency, edge-case and failure handling, and docstring/comment clarity.

What was found

1. Untrusted token reached the terminal (injection). The old code took the token after from verbatim and printed it. An unauthenticated attacker chooses the username they attempt, so they influence that line; a value carrying ANSI control sequences would be echoed to the operator's terminal. No validation stood between the log line and print().
2. Address extraction was positional, not structural. parts.index('from') returns the first from, so a username containing from yielded the wrong token. More seriously, PAM lines (authentication failure; ... rhost=1.2.3.4) were matched by the detector but carry no from token at all, so every sudo/login failure was silently discarded — a false negative in a security report.
3. Unbounded memory keyed on attacker-controlled data. failed_attempts grew one entry per distinct address with no cap; a log padded with spoofed sources was a memory-exhaustion path. Individual line length was unbounded too.
4. Failure handling. os.path.exists() followed by open() is a TOCTOU race and still raised on permission errors or a directory; the file was opened without encoding/errors, so one undecodable byte aborted the run with UnicodeDecodeError.
5. Destructive default. __main__ unconditionally wrote mock_auth.log with mode 'w' in the working directory, clobbering any real log of that name.
6. Standards. No module docstring, no function docstrings, no argparse, no exit-code contract — all four are explicit CONTRIBUTING.md requirements.

What changed and why

• Parsing now uses two anchored regexes (OpenSSH and PAM) behind a single validation gate, normalise_address(), which returns a canonical string only for a valid IP literal. Nothing unvalidated can reach the report — this closes finding 1 and the wrong-token half of finding 2. IPv4-mapped IPv6 peers are collapsed so one host is not counted twice.
• The PAM format is now counted, closing the false-negative half of finding 2. Both defects are pinned by new tests.
• MAX_TRACKED_ADDRESSES and MAX_LINE_CHARS bound memory (finding 3); attempts beyond the cap are tallied as overflow and surfaced in the report rather than dropped silently.
• exists()-then-open() is replaced by one try/except OSError, and files open with errors="replace" (finding 4). Missing file, directory-as-log and undecodable bytes each have a test.
• The demo log is written only when absent (finding 5), with a test asserting an existing file is untouched.
• argparse CLI with --log, --threshold, --top, --demo; exit 0 clean / 1 threshold reached / 2 unreadable; module and function docstrings throughout (finding 6).

Verification

py_compile clean; flake8 --select=E9,F63,F7,F82,F821,F823 --max-line-length=120 clean (full flake8 clean on both files too); pytest tests/ -q → 25 passed; --demo and --help exercised end to end; failure paths exercised.

Verdict

Submitted as COMMENT rather than APPROVE — GitHub does not permit approving one's own pull request. On the merits the change is ready to merge: both defects are fixed, the hardening is bounded and tested, and nothing was removed for well-formed logs. The only outstanding item is the stale README.md index description, noted on the PR and left as a follow-up.

[krishsharma-code]

This is the security boundary the old version lacked. Everything downstream — the counts dict, the report, the terminal — only ever sees a value that came back from here, and this returns None for anything that is not a bare IPv4/IPv6 literal. That is what makes the terminal-escape injection impossible rather than merely unlikely.

One deliberate behaviour choice worth flagging: 192.168.1.50:22 is rejected. sshd logs the port as a separate port 22 token, so a colon-suffixed address means the line is not in the expected format — failing closed is the right call for a security tool.

[krishsharma-code]

errors="replace" matters more than it looks. Without it, a single non-UTF-8 byte in a rotated or partly-written log raises UnicodeDecodeError mid-iteration and the operator loses the whole report — exactly the moment they need it. Covered by test_undecodable_bytes_do_not_abort_the_run.

[krishsharma-code]

The cap is the fix for the memory-exhaustion path: the dict key is attacker-chosen, so its size must not be attacker-controlled. Overflow is counted rather than dropped so the report never implies "only N sources" when there were more. test_address_table_is_capped asserts 10 tracked + 40 overflow for a 50-address log with max_addresses=10.

Open question for a future run: is 10 000 the right ceiling for a busy internet-facing host? A /16 of sources would reach it.

[krishsharma-code]

The old __main__ wrote this file unconditionally with mode 'w' in the working directory. Refusing to overwrite turns a silent clobber of a real mock_auth.log into a message plus a parse of the existing file. test_demo_log_never_overwrites_an_existing_file pins it.

[krishsharma-code]

Exit-code contract, matching the convention api_key_leak_scanner.py established in this repo: 0 clean, 1 threshold reached, 2 unreadable. This is what makes the script usable as a cron alert or a CI gate — the operator gets a signal without parsing stdout.