fix: clean errors for expired tokens/bad accounts, guardrail bypass, audit timestamps - #20
Merged
Merged
Conversation
…S rejects it list_accounts, list_account_roles, and get_role_credentials let a revoked or expired SSO access token raise a raw botocore ClientError straight out of the library, and never cleared the now-useless cached token — every subsequent command failed the same way until the caller knew to run `orgctl logout` themselves. On UnauthorizedException/ForbiddenException (AWS's codes for "this access token is no good", as opposed to a permission denial on the specific action), these now clear the cached token and raise SsoTokenExpiredError with a message pointing at `orgctl login`. Any other ClientError (e.g. a real AccessDeniedException) still passes through unaltered.
- whoami and check-policy called resolve_account/resolve_role/ get_role_credentials with no error handling at all, unlike every other command — an unknown account/role, or an SSO token AWS has since rejected, dumped a raw traceback instead of a clean message. - exec, shell, export-env, list-remote, and creds-process now also catch SsoTokenExpiredError (see the sso.py fix) around their AWS calls, instead of only around the initial login() call — a token can be rejected on any later call just as easily as on login. - doctor printed "OK cache dir writable" unconditionally, without ever testing that a write actually succeeds; it now does a real probe write+delete and reports FAIL if that fails. - also updated the now-adjacent creds-process timestamp formatting off the deprecated datetime.utcfromtimestamp(). Introduces a small _guard() helper so each command's account/role resolution and AWS calls funnel through the same clean-error handling instead of repeating a try/except per command.
…in deny patterns The previous fix only widened the recursive-s3-rm pattern to allow a global flag between "aws" and the service name. The other four built-in patterns (delete-account-alias, leave-organization, close-account, s3 rb --force) still required "aws" to be immediately followed by the service, so `aws --profile x organizations close-account ...` (or any other flag inserted before the service name, which is normal `aws` CLI usage) slipped past them undetected.
… push time push_to_cloudwatch stamped every event with time.time() at push time instead of the time the command actually ran, so CloudWatch showed everything bunched at push time rather than when it happened — defeating the point of an audit trail. Each event's timestamp is now parsed from its own "ts" field, falling back to push time only for an entry with no/unparseable ts (e.g. a hand-edited log line).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #18, fixing four more correctness bugs found in review. One commit per fix, each with regression tests.
list_accounts,list_account_roles, andget_role_credentialslet a rejected access token raise a rawClientError, and never cleared the now-useless cached token — every subsequent command failed the same way until you knew to runorgctl logout. OnUnauthorizedException/ForbiddenExceptionthey now clear the cached token and raiseSsoTokenExpiredErrorpointing atorgctl login; any otherClientError(e.g. a real permission denial) still passes through unaltered.whoami/check-policycrashed with a raw traceback on an unknown account/role — unlike every other command, they had no error handling around account/role resolution at all. All commands that use the token after login (exec,shell,export-env,list-remote,creds-process,whoami,check-policy) now also catch the newSsoTokenExpiredError.doctor's "cache dir writable" check used to print OK unconditionally without testing anything; it now does a real probe write+delete.awsimmediately followed by the service name (only the recursive-s3 rmone was widened in fix: token cache key, policy simulator creds, s3 rm guardrail, sync-aws-config #18), soaws --profile x organizations close-account ...and similar slipped past. All five patterns now tolerate a global flag in between.audit-log --push-cloudwatchstamped every event with push time, not when the command actually ran, so CloudWatch showed everything bunched at push time — defeating the point of an audit trail. Each event now uses its own recordedts, falling back to push time only if that's missing/unparseable.Verification
ruff check,ruff format --check,mypy src: clean.pytest: 108 passed (was 80); coverage 51% → 70%.cli.pywent from 0% to 53% andsso.pyfrom 27% to 60% — this is a newtests/test_cli.pyexercising the CLI directly viaclick.testing.CliRunner, which is exactly the gap that let bugs Bump actions/setup-python from 5 to 7 #1/Bump googleapis/release-please-action from 4 to 5 #2 through unnoticed.git stashper file) and confirmed to fail there and pass with the fix; a few new tests intentionally cover already-correct paths (e.g.creds-process's existingConfigErrorhandling) and pass either way.Not in this PR
Still open from the original review: cache files are briefly world-readable before
chmodlocks them down (no-op on Windows anyway),export-envdoesn'tshlex.quoteits output, README/threat-model claims about audit coverage oncreds-process/whoami/check-policyand about credentials never leaving the child process (contradicted byexport-env/creds-processprinting them by design), andprotected_account_idsonly blockingexecnotshell.