ff9b15933deb25260375e78c6813699394b62902
1
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 ergosteur
|
ff9b15933d |
Final review pass: fix STARTTLS fallback, PS null-count bugs, badge scoping; add README
Ran a full review pass over both audit scripts and the HTML converter, independently cross-checked against an automated code review, and fixed everything that survived verification: - ad_audit.py: connect() ignored the boolean return of conn.start_tls(). ldap3 doesn't raise on STARTTLS failure by default, so a rejected/ downgraded STARTTLS silently fell through to an unencrypted bind -- the bind DN and password would go out in cleartext with no warning. Now aborts before calling bind() if start_tls() returns False. - ad_audit.py: "Enabled accounts that have never logged on" detail rows weren't filtering out disabled accounts, unlike the count used to decide whether to show the finding at all -- so a disabled account could appear in a list titled "enabled accounts." - ad_audit.py: replaced deprecated datetime.datetime.utcnow() (scheduled for removal) with timezone-aware datetime.now(timezone.utc); made FILETIME_EPOCH aware too so the stale-account comparison keeps working without a naive/aware TypeError. - ad_audit.py: removed dead code (unused defaultdict import, unused UAC_NOT_DELEGATED constant, unused scope_bit variable, import os buried inside main() instead of at module level). - Invoke-ADAudit.ps1: GroupCount/MemberCount used (@($x)).Count, which PowerShell evaluates to 1 (not 0) when $x is $null -- @($null) is a one-element array, not an empty one. This silently broke "empty group" detection (the report's own headline feature) for any group with genuinely zero members, and inflated GroupCount for users with no group memberships. Added Get-SafeCount to null-check before counting. - Invoke-ADAudit.ps1: $maxDepth came back as $null (not 0) when a SearchBase had zero OUs, printing a blank instead of "0" in the report. - Invoke-ADAudit.ps1 / ad_audit.py: the h4 finding-detail title wasn't escaped through ConvertTo-MdSafe/md_escape in one of its three occurrences, inconsistent with the other two -- fixed for consistency even though today's titles are all fixed strings. - md_to_html.py: badge_findings_table()'s regex matched any single-word table cell equal to a severity name, not just the Severity column of the findings table -- an OS name, group name, or object class that happened to literally be "Critical"/"High"/etc (plausible under a tiering naming scheme) would get rewritten into a colored badge anywhere in the report. Rescoped to the one table whose header row is literally "Severity | Finding | Count | Notes", which only this generator ever emits. - Added README.md covering both scripts' usage, what each collects, and a documented known limitation (finding-detection logic is duplicated between the Python and PowerShell implementations with no shared source of truth). Every fix verified with a reproduction before and after: a mocked start_tls() failure confirms bind() is never reached; mocked $null MemberOf/Members confirm counts are now 0; a zero-OU domain confirms "Maximum nesting depth: 0" instead of blank; a disabled never-logged-on account confirms it's excluded from the enabled-accounts finding; and AD data literally named "Critical"/"High" confirms it no longer gets badged outside the real findings table. |