Extend checks for Security Hub CSPM AI Security Best Practices parity, plus false-Failed bug fixes#51
Open
mehtadman87 wants to merge 9 commits into
Conversation
…ixes
Fixes the remaining PR-0 covered-control severity and correctness
defects identified by the AI Security Best Practices Gap Analysis
(docs/AI_SECURITY_BEST_PRACTICES_GAP_ANALYSIS.md), on top of the
already-committed AC-06/AC-07/SM-12/SM-14 fixes and report-layer
scoring changes:
- AC-12 (BedrockAgentCore.4): pass-path severity was Medium while the
fail-path emitted Low; both now Medium.
- AG-24: AccessDenied on ListGateways now routes through the
COULD_NOT_ASSESS disposition (N/A, Low) instead of Informational,
which understated an access gap as no issue.
- SM-11 (SageMaker.5): fail-path severity was High while the pass-path
used Medium; both now Medium, matching the Security Hub control.
- SM-15 (SageMaker.17): pass-path severity was High while the fail-path
used Medium; both now Medium.
- SM-13 (SageMaker.14): named monitoring job definitions
(MonitoringJobDefinitionName + MonitoringType) are now resolved via
the matching DescribeXJobDefinition API. Previously only inline
definitions were read, so every named-definition schedule defaulted
isolation to disabled and false-failed.
- SM-03 (SageMaker.21): split into two checks. The notebook-storage
encryption check keeps the SM-03 label (matches SageMaker.21 scope)
and drops the substring KMS test ('aws/sagemaker' in kms_key_id),
which missed AWS-managed keys referenced by key id/ARN and could
false-PASS an encryption control; detection is now presence-as-proxy
only. Domain and training-job encryption move to a new repo-only
SM-26 check.
- SM-01 (SageMaker.1): domain network-access-type sub-check moved out
to a new repo-only SM-27 check, so SM-01 findings match the control's
documented NotebookInstance-only scope.
Updates docs/SECURITY_CHECKS.md, docs/DEVELOPER_GUIDE.md, README.md,
and docs/SECURITY_CHECKS_FINSERV*.md for the new SM-26/SM-27 checks
and the corrected check-count totals (161 -> 163).
Adds regression tests for each fix and updates the SM-01/SM-03 test
classes for the split checks.
All 431 tests in tests/ and 582 tests in finserv_tests/ pass (run
separately per CI convention, matching .github/workflows/python-tests.yml).
ruff check/format clean.
Implements the remaining gap-analysis PR-1/PR-2/PR-3 work on top of the PR-0 severity/disposition fixes already on this branch, per user direction to land everything in one PR/branch. PR-1: - Fix SM-12 (SageMaker.4) to use ListEndpointConfigs and DescribeEndpointConfig / InitialInstanceCount instead of ListEndpoints / CurrentInstanceCount, which under-reported instance counts for endpoints with pending or updating variants. - Add a static Check_ID to SecurityHub_Control mapping (SECURITY_HUB_CONTROL_MAP and security_hub_controls()) in report_template.py, shared by the consolidated-report Lambda and the CodeBuild multi-account script. Adds a Security Hub Control column to the HTML report. PR-2: - Add check_bedrock_data_source_encryption (BR-33, Bedrock.1): Knowledge Base data source customer-managed KMS encryption. - Add check_browser_network_mode (AC-14, BedrockAgentCore.5) and check_code_interpreter_network_mode (AC-15, BedrockAgentCore.7): custom browser and code-interpreter VPC network mode checks. - Fix a latent UnboundLocalError in check_bedrock_data_source_encryption (a local variable was referenced in the except branch before being assigned), caught by the new BR-33 access-denied test. PR-3 (12 new SageMaker checks, SM-28 through SM-39): - Add a table-driven job-definition helper (_JOB_DEFINITION_APIS and _list_job_definitions_with_details) covering Explainability, DataQuality, ModelBias, and ModelQuality monitor job definitions. - Add checks for traffic encryption and network isolation across Clarify and Model Monitor job definitions (SageMaker.10, .11, .12, .13, .15, .20, .25), notebook platform (SageMaker.8), online feature store KMS (SageMaker.18, any-KMS family), monitoring traffic encryption via named job definitions (SageMaker.22), and inference experiment encryption (SageMaker.23 and .24). - SageMaker.20 and .25 severity seeded from the published Security Hub severity (High) rather than the family band (Medium), per gap analysis register guidance. Supporting changes: - IAM: add the new SageMaker, Bedrock, and AgentCore list/describe actions across all 5 templates (template.yaml, template-multi-account.yaml, and the 3 deployment yaml files). - Tests: unit tests for every new check (pass, fail, N/A, and could-not-assess paths) plus updated IAM-coverage tests. - Docs: SECURITY_CHECKS.md, README.md, DEVELOPER_GUIDE.md, and SECURITY_CHECKS_FINSERV.md updated for the new check counts (163 to 178 total).
Extract the FinServ Likelihood x Impact severity methodology (docs/SECURITY_CHECKS_FINSERV_SEVERITY_METHODOLOGY.md) into a severity_disposition.py sibling module for each of the three general assessment services, mirroring the existing per-module schema.py duplication pattern rather than a shared Lambda layer: - agentcore_assessments/severity_disposition.py: enum-based (SeverityEnum/StatusEnum) SEVERITY_REGISTER covering AC-00..AC-15 and AG-24..27 (46 entries). - bedrock_assessments/severity_disposition.py: string-based SEVERITY_REGISTER covering all 34 BR- checks (45 entries), fixing 13 Passed/Failed severity-drift cases along the way. - sagemaker_assessments/severity_disposition.py: string-based SEVERITY_REGISTER covering all 39 SM- checks. Roll out the COULD_NOT_ASSESS disposition (Status=N/A, Severity=Low, "COULD NOT ASSESS:" name prefix) across all three modules: - Every outer "except Exception" in every check function (plus both lambda_handler enumeration loops in AgentCore) now returns could_not_assess_row(...) instead of fabricating a Failed/High result. - Inner enumeration failures previously swallowed into a false "no resources found" N/A are re-raised into the outer handler (AgentCore AC-04/AC-05/AC-10; SageMaker's shared job-definition listing helper across SM-29..SM-39, plus SM-06, SM-08, SM-25, SM-28). - Per-component except blocks that appended a fabricated High/Medium/Failed "check error" issue alongside real findings (SageMaker SM-05, SM-06, SM-07, SM-08) now route directly through could_not_assess_row. - AgentCore AC-10's policy-check branches and AG-24's per-gateway get_gateway failure branch move from Informational to COULD_NOT_ASSESS (Low). Add a per-module drift-guard test file mirroring finserv_tests/test_severity_register.py's bidirectional source-scan (every static finding_name in app.py has a register entry and vice versa): tests/test_agentcore_severity_register.py, tests/test_bedrock_severity_register.py, tests/test_sagemaker_severity_register.py. Apply the same sys.modules cache-eviction guard consistently across all three test_*_checks.py and test_*_severity_register.py files, since agentcore_assessments, bedrock_assessments, and sagemaker_assessments each define their own same-named schema.py/severity_disposition.py (without this guard, running the full suite in some orderings makes one module's app.py bind to a different module's cached severity_disposition, which caused test_sagemaker_severity_register to see Bedrock's register when run after test_bedrock_checks). Add the missing AG-24 -> BedrockAgentCore.2 entry to SECURITY_HUB_CONTROL_MAP in report_template.py. Update docs/SECURITY_CHECKS_FINSERV_SEVERITY_METHODOLOGY.md and docs/AI_SECURITY_BEST_PRACTICES_GAP_ANALYSIS.md to reflect that tool-wide adoption (PR-0) is complete. Verified: full tests/ suite = 517 passed (both forward and reverse file-collection order); finserv_tests/ = 582 passed, unchanged; ruff check clean on all touched files.
check_agentcore_observability read loggingConfig/tracingConfig off
GetAgentRuntime, but that API response has no such fields per the AWS
API reference (only agentRuntimeArn/Id/Name/Version, authorizerConfig,
networkConfig, lifecycleConfig, etc.). Every runtime therefore always
resolved these to None, making AC-04 always report Failed regardless
of actual configuration.
AWS docs confirm CloudWatch Logs and X-Ray tracing are auto-enabled
for AgentCore Runtimes with no API-exposed disable toggle, so there is
no field this check can validate a pass/fail verdict against. Rewrote
the check to verify what IS API-visible instead: whether the
AgentCore-managed log group exists
(/aws/bedrock-agentcore/runtimes/{id}-DEFAULT). A missing log group
(runtime never invoked) is now reported as an informational ADVISORY
rather than a fabricated Failed.
Updated severity_disposition.py's register to match: removed the
stale CloudWatch Logs / X-Ray Tracing / Log Group Missing entries,
added "ADVISORY: AgentCore Runtime Log Group Not Yet Created".
Added 6 tests in tests/test_agentcore_checks.py covering: client
unavailable, no runtimes, log group present (Passed), log group
absent (Informational ADVISORY, never Failed), ResourceNotFoundException
handling, and unexpected-error routing to COULD_NOT_ASSESS. Removed a
stale duplicate TestAC04Observability class that mocked
cloudwatch_client/xray_client, which the rewritten check no longer
uses.
…match
FS-08 (check_agentcore_policy_engine) and FS-66
(check_agentcore_end_user_identity_propagation) read
authorizerConfiguration off ListAgentRuntimes summaries, but that
field only exists on GetAgentRuntime per the AWS API reference. Every
runtime therefore always resolved to "no authorizer configured",
making both checks always report Failed regardless of actual
configuration. Fixed both to call GetAgentRuntime per runtime, with
explicit accounting for runtimes whose detail call fails: if every
runtime is unassessable the check now reports N/A (COULD_NOT_ASSESS)
rather than fabricating a Passed row. Also removed FS-66's check for
an "iamAuthorizer" field, which AWS CDK/CloudFormation docs confirm
never existed on AuthorizerConfiguration.
FS-09 (check_agent_transaction_limits), FS-52
(check_bedrock_sdk_version_currency), and FS-67
(check_agent_financial_transaction_thresholds) all select candidate
Lambda functions via broad keyword matching against FunctionName
(e.g. "agent", "bedrock", "aiml"). Because this tool's own deployed
Lambdas are literally named
"aiml-security-<stack>-BedrockAssessment",
"aiml-security-<stack>-AgentCoreAssessment", etc. (see
template.yaml), those keywords always matched the tool's own
infrastructure in addition to (or instead of) any genuine
customer-deployed Bedrock/agent Lambdas. Added a shared
_is_tool_own_infrastructure() helper (checks the aiml-security-/
aiml-sec- FunctionName prefixes) and applied it as an exclusion filter
in all three checks.
Also fixes a related mock gap in
finserv_tests/test_inventory_equivalence.py: its fixture only stubbed
service == "agentcore", but the app calls
boto3.client("bedrock-agentcore-control"), so FS-08/FS-66 were
silently hitting an unconfigured MagicMock whose default behavior
happened to produce a fabricated Passed row masking the original bug
in that test. Added the correct mock branch (empty agentRuntimes list)
and updated the frozen baseline literal's FS-08/FS-66 rows from the
fabricated "Configured"/Passed result to the genuine "No AgentCore
Runtimes Found"/N/A result for that fixture's account state.
Added 14 new tests across test_checks.py (5 for FS-09, 3 for FS-52, 6
for FS-67 covering exclusion of the tool's own Lambdas alongside
continued detection of genuine external agent Lambdas) plus the
FS-08/FS-66 GetAgentRuntime mock updates and new N/A-when-unassessable
tests already present in this diff.
The reference field joined two documentation URLs with a literal "\n" inside a single string, which gets embedded raw into an HTML href attribute by the report template with no processing, breaking the link in every BR-01 row of the rendered report (confirmed via a live rerun: 8 broken hrefs in the single-account report, more in the multi-account report). Simplified to the single canonical URL, consistent with every other check in the codebase using one reference = one URL.
A live rerun against a real AWS account surfaced 19 Check_IDs that were
100% Failed with zero Passed/N-A anywhere. Investigating each check's
source against the account's actual state (verified via AWS CLI calls
to list-endpoints, list-models, list-guardrails, get-security-policy,
get-policy-version, etc.) classified 14 as genuine gaps — real findings
for an account that truly lacks the resource — and 3 as structural bugs
where the check could never pass regardless of account configuration:
FS-39 (check_sagemaker_clarify_bias) and FS-41
(check_sagemaker_clarify_explainability) checked for a
MonitoringSchedule of type ModelBias/ModelExplainability without ever
checking whether a SageMaker endpoint exists to be monitored. Per AWS
documentation, SageMaker Clarify bias/explainability monitoring only
observes a real-time endpoint's live inference traffic — an account
with zero endpoints (as verified in the account this was rerun
against) has no monitorable model, so these checks were structurally
guaranteed to fail on every account that hasn't deployed a SageMaker
endpoint, independent of whether monitoring is configured. Fixed by
adding a list_endpoints precondition: zero endpoints now reports N/A
("... Monitoring Not Applicable") before reaching the
monitoring-schedule check, matching the "nothing deployed to check"
pattern already used by FS-16/FS-20/FS-36/FS-47.
FS-50 (check_guardrail_relevance_grounding) was missing the
"if not guardrails: return N/A" guard present on every sibling
guardrail-filter check (FS-36, FS-38, FS-45, FS-47). With zero
guardrails configured, the for-loop over an empty summaries list
silently fell through to "no relevance filters found" -> Failed,
instead of the N/A that every other guardrail-filter check correctly
emits for the identical "no guardrails configured" precondition.
Fixed by adding the missing guard, mirroring FS-47's exact pattern.
Added SEVERITY_REGISTER entries (Informational) for the three new N/A
finding names. Added tests for the N/A/WARN/PASS paths of FS-39/FS-41
and the new N/A path of FS-50; updated two existing FS-39/FS-41
pass-path tests in test_checks_coverage.py to mock list_endpoints.
Updated test_inventory_equivalence.py's sagemaker mock fixture to
explicitly model an account state with a deployed-but-unmonitored
endpoint (list_endpoints non-empty, list_monitoring_schedules empty),
preserving the frozen baseline's existing FS-39/FS-41 Failed rows
rather than accidentally exercising the new N/A branch through an
unconfigured mock.
The check descriptions in SECURITY_CHECKS.md and SECURITY_CHECKS_FINSERV.md were stale relative to the behavior changes made in 507fe83 and 6087494: - AC-04's description still described verifying "CloudWatch Logs and X-Ray tracing configuration" — the exact reading that was removed because GetAgentRuntime has no such fields. Updated to describe the actual log-group-existence check and the informational-advisory treatment of an uninvoked runtime. - FS-39/FS-41's Detection field didn't mention the new ListEndpoints precondition that routes accounts with zero SageMaker endpoints to N/A instead of the monitoring-schedule check. - FS-50's Detection field didn't mention the new ListGuardrails precondition that routes accounts with zero guardrails to N/A, matching the convention already documented for FS-36/FS-38/FS-45/FS-47.
github-actions
Bot
added
documentation
The CI python-lint workflow's ruff format --check failed on PR aws-samples#51 for this file, which was drifted from a prior commit on this branch (not touched by the fixes in this PR). No semantic change — line-wrapping only, applied with the same ruff version (0.15.21) the CI workflow installs via `uv tool run ruff`.
Contributor
Author
|
@vivekmittal514 @agasthik could you please review and merge? All CI checks pass. Summary in the PR description; happy to answer any questions. |
mehtadman87
changed the title
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
Closes #50.
This PR is primarily about extending the tool's checks to close control-parity gaps against AWS Security Hub CSPM's AI Security Best Practices v1.0.0 standard (
arn:aws:securityhub:<region>::standards/ai-security-best-practices/v/1.0.0), per the gap analysis indocs/AI_SECURITY_BEST_PRACTICES_GAP_ANALYSIS.md. It adopts a documented Likelihood × Impact severity/disposition methodology tool-wide, adds 15 new checks across SageMaker/Bedrock/AgentCore, and fixes a Security Hub control-mapping gap (AG-24). Bundled into the same branch is a focused round of check-logic bug fixes, found and confirmed via live reruns against real single-account and multi-account AWS environments while doing the parity work.What's in this PR
Security Hub CSPM AI Security Best Practices parity + severity model
e008048,3406c61) — Adopts the FinServ Likelihood × Impact severity methodology tool-wide via aseverity_disposition.pysibling module per assessment service (AgentCore, Bedrock, SageMaker). Rolls out aCOULD_NOT_ASSESSdisposition (Status=N/A, Severity=Low) everywhere an exception handler previously fabricated a false Failed/High result, so genuine access/API errors no longer masquerade as findings. Fixes 13 Passed/Failed severity-drift cases in the Bedrock register and corrects the AG-24 → BedrockAgentCore.2 Security Hub control mapping, which was missing fromSECURITY_HUB_CONTROL_MAP. Also fixes several PR-0 severity/correctness defects (AC-12, SM-11, SM-13, SM-15) and splits SM-01/SM-03 so their scope matches the corresponding Security Hub control exactly.332caf2) — Adds a Security Hub Control column to the HTML report backed by a static Check_ID → control mapping. Adds BR-33 (Bedrock Knowledge Base data source KMS encryption), AC-14/AC-15 (AgentCore Browser/Code Interpreter VPC network mode), and 12 new SageMaker checks (SM-28 through SM-39) covering traffic encryption and network isolation across Clarify and Model Monitor job definitions, notebook platform, online feature store KMS, and inference experiment encryption. Fixes SM-12 to useListEndpointConfigs/DescribeEndpointConfiginstead ofListEndpoints, which under-reported instance counts for endpoints with pending or updating variants.docs/SECURITY_CHECKS.md,README.md,docs/DEVELOPER_GUIDE.md,docs/SECURITY_CHECKS_FINSERV*.md) updated to reflect the new checks (161 → 178 total).Bug fixes found via live account reruns while doing the above
Wrong-API-field bugs (previously always-Failed regardless of configuration):
agentcore_assessments/app.py) — readloggingConfig/tracingConfigoffGetAgentRuntime, but per the AWS API reference those fields don't exist on that response. Rewrote to verify the one thing that IS API-visible: whether the AgentCore-managed log group exists. A runtime that's never been invoked now reports an informational advisory instead of a false Failed.finserv_assessments/app.py) — readauthorizerConfigurationoffListAgentRuntimessummaries, which never include that field (onlyGetAgentRuntimedoes). Fixed to callGetAgentRuntimeper runtime, with N/A fallback if every runtime's detail call fails (never fabricates a Passed).Self-referential keyword-matching bugs (tool was flagging its own infrastructure):
"agent","bedrock","aiml") that also match this tool's own deployed Lambdas (aiml-security-<stack>-*). Added a shared_is_tool_own_infrastructure()exclusion helper. Verified live: the target account has 18aiml-security-*Lambdas that would have matched the old keywords; none appear in the new findings, while genuine external agent Lambdas are still correctly flagged.Missing-precondition bugs (structurally could never pass without a precondition check):
ListEndpointsprecondition: zero endpoints now reports N/A instead of a guaranteed Failed, matching the existing "nothing to check" convention already used by FS-16/FS-20/FS-36/FS-47.Report rendering bug:
referencefield joined two URLs with a literal\n, which gets embedded raw into an HTMLhrefattribute with no processing, breaking the link in every BR-01 row of the rendered report. Simplified to a single canonical URL.Documentation
docs/SECURITY_CHECKS.md(AC-04) anddocs/SECURITY_CHECKS_FINSERV.md(FS-39/FS-41/FS-50) check descriptions to match the corrected behavior, in addition to the parity-work doc updates above.Testing
finserv_tests/test_inventory_equivalence.py(its fixture didn't stubbedrock-agentcore-control, which had been masking the original FS-08/FS-66 bug behind a MagicMock artifact) and updated its frozen baseline to reflect the genuine account state.tests/= 519 passed,finserv_tests/= 597 passed (1116 total),ruff check .clean,ruff formatapplied.ash --output-dir .ash/ash_output); all 175 actionable findings are pre-existing and located exclusively in CloudFormation/SAM templates, GitHub Actions workflows, andsample-reports/*.html— none in any file touched by this PR.Not in scope
19 check IDs remain 100%-Failed in the target test account; most are legitimate gaps for that account (no CloudWatch alarms, no Macie, no model cards, etc.) and were individually verified against live AWS state rather than assumed. One additional known bug (BR-03's condition-blind marketplace-policy check) was identified during this investigation but is intentionally left out of this PR's scope.