🐛 aws-secret-manager-edgecases-enhancement#6534
🐛 aws-secret-manager-edgecases-enhancement#6534LittleSalkin1806 wants to merge 8 commits intomondoohq:mainfrom
Conversation
LittleSalkin1806
changed the title
|
@tas50 would be great if this can be reviewed. As this has an impact on every kms key. |
|
I'll see if we can get some subject matter experts on this one for you @LittleSalkin1806 |
|
I am just curious if you would handle this for an edge case of secretmanager normalization for example eventbridge special cases or if you would handle it in theory as it could happen in other services as well. What would be your approach here ? |
|
/review |
|
sorry for the delay. i have the pr checked out locally, addressing review comments and looking at the code thank you for the contribution! |
- Add early guard for empty region when normalizing bare UUID keys - Remove dead extractKmsKeyId function (the "key/" prefix branch was unreachable) - Downgrade cross-account log from Error to Warn (expected operational condition) - Add table-driven unit tests for NormalizeKmsKeyRef Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a KMS key belongs to a different AWS account, we can't fetch its details, but we should still return the resource with the ARN populated so security tools can see which KMS key is referenced. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When a KMS key belongs to a different AWS account, the lazy-loaded fields (metadata, tags, aliases, keyRotationEnabled, keyState) would fail with AccessDeniedException or ValidationException. Now these methods detect cross-account keys and return empty values instead of making API calls that will always fail. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Log warning when ARN parse fails in isCrossAccountKey - Remove duplicate arn/region assignments in cross-account block (already set before the check) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add isCrossAccountKey() checks to createdAt, deletedAt, enabled, description, and grants methods that were added during rebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vjeffrey
force-pushed
the
edgecase-enhancement-aws-secrets-manager
branch
from
f56f504 to
5151f51
Compare
![mondoo-code-review[bot]](https://avatars.githubusercontent.com/in/2834902?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cross-account KMS keys are silently returned with false/empty defaults instead of signaling missing data, and the metadata method loses its caching.
Additional findings (file/line not in diff):
- 🟡
providers/aws/resources/aws_kms.go:170—keyRotationEnabled()returnsfalse, nilfor cross-account keys. Returning a zero-value boolean with no error is indistinguishable from "rotation is genuinely disabled". Security policies queryingaws.kms.key.keyRotationEnabled == falsewill flag cross-account keys as non-compliant even though the real state is unknown.
Consider setting the field to null state (plugin.StateIsNull | plugin.StateIsSet) or returning an explicit error so downstream consumers can differentiate "unknown" from "disabled". The same concern applies to enabled() (line 300).
- Replace hand-rolled UUID heuristic with google/uuid.Parse for proper validation of KMS key ID format - Rewrite metadata() to use getKeyMetadata() so it shares the cached DescribeKey result with keyState, createdAt, deletedAt, etc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
KMS Keys API calls use KeyID = support multiple formats aliases, UUID(ID) or arn format.
I tried to stick as much as possible to the current code structure with out refactoring but let KMS Key handle normalization.
When we look at types
KmsKeyId is already an ARN from the AWS API for secetmanager and probably most of the others.
But as you can see from the screenshot eventbridge connection and potentially others have edge cases.
types.secretsmanager states that only arn should be rerturned but UUId was returned.
I could replicate this by creating event bridge connection using the basic auth and then selecting a key
IMPORTANT! kms key arn is returned if i select an alias or kmskeyarn but if i just search and select kmskey id then only the UUID is returned by go sdsk
I did some normalization on kms side to handle such cases as we dont always know if this case will happen or not for the responding service where the key is linked.
Todos:
I would still need this information and distungish on mql level.
Therefore i added some todo with the idear
accessible: boolEvery Key which exist and we dont have access return accessible: false
This should probably be an another issue.