Skip to content

Fixed the --verbose issue#9

Open
RajChowdhury240 wants to merge 4 commits intoBeyondTrust:mainfrom
RajChowdhury240:main
Open

Fixed the --verbose issue#9
RajChowdhury240 wants to merge 4 commits intoBeyondTrust:mainfrom
RajChowdhury240:main

Conversation

@RajChowdhury240
Copy link
Copy Markdown

The Problem

initially i noticed the readme talks about --verbose mode in scan but it wasnt in place and didnt work properly

Screenshot 2026-03-18 at 12 03 45 AM

Fix Summary

  • Add --verbose / -v option to bks scan for detailed per-user output
  • Verbose mode shows full identity (User ID, ARN, IAM path), full timestamps, individual Bedrock credential IDs
    with status, IAM access key IDs, and policy breakdown (managed vs inline)
  • Propagates verbose flag to the scanner so diagnostic messages during scanning are also enabled

Motivation

Running bks scan --verbose previously failed with Error: No such option: --verbose. The --verbose flag only
existed at the group level (bks --verbose scan), but users expect it on the subcommand too. The default scan
output is a compact summary table — there was no way to get detailed per-user information without switching to
--json.

Changes

bedrock_keys_security/commands/scan.py

  • Added --verbose / -v Click option
  • When set, merges with group-level verbose context and routes output to the new verbose report

bedrock_keys_security/core/scanner.py
- Added generate_verbose_table_report() method that renders a detailed per-user block including:
- User ID, ARN, full creation timestamp, IAM path
- Bedrock API credential IDs with status and creation date
- IAM access key IDs (red-highlighted for AT RISK users)
- Attached managed and inline policies listed individually
- Color-coded status and summary section

Test plan

  • bks scan --help shows -v, --verbose option
  • bks scan --verbose runs without error and shows detailed per-user blocks
  • bks scan (without --verbose) still shows the compact table as before
  • bks --verbose scan still works (group-level flag)
  • bks scan -v short flag works
  • bks scan --verbose --json still outputs JSON (verbose only affects table mode)

Test results after the fix

image image

@RajChowdhury240 RajChowdhury240 requested a review from a team as a code owner March 17, 2026 18:48
@MrCloudSec MrCloudSec self-requested a review March 19, 2026 13:15
Copy link
Copy Markdown
Member

@MrCloudSec MrCloudSec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @RajChowdhury240, nice work on this! The per-user card layout is really solid for IR triage.

Found a few things while testing that are worth cleaning up:

  1. --verbose swapping the output format: typically --verbose just means more logging, not a different report. I'd keep the standard table as the output and only use verbose to toggle the [INFO]/[CRED]/[POLICY] log lines. If we want the detailed view in the CLI, we could add a --detailed flag later.

  2. bks --verbose scan doesn't work: the scan command only checks its own local verbose param, so the group-level flag gets ignored. Using ctx.obj.verbose for the check fixes this.

  3. The ctx.obj._scanner = None reset: this isn't needed since verbose gets set on ctx.obj before the scanner is lazily created. Removing it also avoids creating a duplicate AWS session.

  4. Spinner and log lines stepping on each other: the [INFO] lines go through click.echo() while the spinner writes to stderr, so you get ⠼ Scanning[INFO] Found phantom user.... The spinner needs to clear/redraw around log output. Also check_credentials has one stray click.echo() that should go through output.warning().

  5. Small stuff: "1 access keys" should be "1 access key", and the summary block is duplicated between both report methods (easy to extract into a shared helper).

Happy to help if you have questions on any of these!

@RajChowdhury240
Copy link
Copy Markdown
Author

pushed the changes , can you test it out once and lmk what you think! Thanks

Copy link
Copy Markdown
Member

@MrCloudSec MrCloudSec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @RajChowdhury240, this is looking solid now. Two small things left:

  1. The spinner clears before log output but never redraws after, so it vanishes until the next tick. Just add a redraw after the click.echo in the log helpers.

  2. _spinner_active = False in the finally block should be inside a with _spinner_lock: to avoid a race with the log helpers.

Both minor, but worth cleaning up since this is what users see first. Rest looks good!

@RajChowdhury240
Copy link
Copy Markdown
Author

Both fixes applied:

  1. Spinner redraw after log output —

Added _redraw_spinner() helper that writes the spinner frame + label back to stderr. Called after every click.echo in info, success, warning, error, and high_risk (lines 34, 41, 48, 55, 62). This prevents the spinner from vanishing between log output and the next spin tick.

  1. _spinner_active = False under lock —
    The finally block (line 116-117) now wraps the flag reset in with _spinner_lock:, eliminating the race with log helpers. Also moved the initial flag setup (lines 105-108) under the lock for consistency.

@ipowellBT ipowellBT closed this Mar 23, 2026
@ipowellBT ipowellBT reopened this Mar 23, 2026
@ipowellBT ipowellBT closed this Mar 23, 2026
@ipowellBT ipowellBT reopened this Mar 23, 2026
@MrCloudSec MrCloudSec self-requested a review March 23, 2026 20:29
Copy link
Copy Markdown
Member

@MrCloudSec MrCloudSec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @RajChowdhury240 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants