Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in new KeyMgmt threat and tests #406

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mlysaght2017
Copy link
Contributor

@eddie-knight - added in todos as well on two threats related to KMS that are currently down as common, but may be better shifted to KMS specific threats - let me know your thoughts.

@mlysaght2017 mlysaght2017 requested review from a team as code owners September 22, 2024 18:14
eddie-knight
eddie-knight previously approved these changes Sep 22, 2024
Comment on lines +111 to +112
threats:
- CCC.KeyMgmt.TH01
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should map common-controls to specific-threats

@@ -10,7 +10,7 @@ threats:
- TA0005
- T1562
- id: CCC.TH02
title: Vendor-hosted keys are compromised
title: Vendor-hosted keys are compromised #TODO: Should this be in the crypto service?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense to me for this one.

@@ -32,7 +32,7 @@ threats:
- TA009
- T1557
- id: CCC.TH04
title: Attacker encrypts data with client-managed keys
title: Attacker encrypts data with client-managed keys #TODO: Should this be in the crypto service?
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a matter of a non-KMS asset being compromised, right? So there is a threat to any service that allows the key to perform destructive action.

Pivot question... Should this instead be "Client managed keys are compromised" ?

damienjburks
damienjburks previously approved these changes Sep 22, 2024
Copy link
Contributor

@damienjburks damienjburks left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants