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

Properly encapsulate providers to allow for different detonation UUIDs #295

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

christophetd
Copy link
Contributor

@christophetd christophetd commented Jan 26, 2023

Current state

"Providers" (i.e. instances of the AWS SDK, GCP SDK etc.) are currently globally shared. Each attack technique retrieves the provider it needs by using the global providers namespace:

// https://github.com/DataDog/stratus-red-team/blob/main/v2/internal/attacktechniques/aws/credential-access/ssm-retrieve-securestring-parameters/main.go#L55
func detonate(params map[string]string) error {
	ssmClient := ssm.NewFromConfig(providers.AWS().GetConnection())
        // ...
}

When Stratus Red Team detonates an attack technique, it injects an UUID in the user-agent of the relevant SDK. This UUID is also globally accessible through providers.UniqueExecutionId.

The problem

The detonation UUID is currently global per process. For instance, if we detonate multiple Stratus Red Team attack techniques through the programmatic interface, all the detonations have same UUID.

This is a problem because it defeats the very purpose of this UUID mechanism: creating a causation between a detonation and the resulting logs.

The solution

  • Remove all global variables related to providers
  • Introduce a new interface CloudProviders and structure CloudProvidersImpl holding references to the appropriate providers:
// CloudProviders provides a unified interface to access the various cloud providers SDKs
type CloudProviders interface {
	AWS() *providers.AWSProvider
	K8s() *providers.K8sProvider
	Azure() *providers.AzureProvider
	GCP() *providers.GCPProvider
}
  • Instantiate providers as requested, to avoid e.g. throwing AWS authentication errors if we detonate an Azure attack technique
  • Modify the signature of the detonate and revert attack technique functions to inject a CloudProviders (dependency injection):
func detonate(params map[string]string, providers stratus.CloudProviders) error {
	ssmClient := ssm.NewFromConfig(providers.AWS().GetConnection())
        // ...
}

Retro-compatibility

This should be fully retro-compatible.

Manual tests

  • GCP: UUID is properly injected
  • AWS: UUID is properly injected
  • Azure: UUID is properly injected
  • K8s: UUID is properly injected
  • If authenticated only against GCP, no errors are thrown when detonating an AWS attack technique
  • If authenticated only against AWS, no errors are thrown when detonating an AWS attack technique
  • If authenticated only against Azure, no errors are thrown when detonating an AWS attack technique
  • If authenticated only against K8s, no errors are thrown when detonating an AWS attack technique

@christophetd christophetd marked this pull request as ready for review January 26, 2023 15:16
func (m *Runner) GetUniqueExecutionId() string {
return providers.UniqueExecutionId.String()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: This used to be the global unique execution ID that's now properly encapsulated

@christophetd christophetd added the kind/enhancement New feature or request label Jan 26, 2023
Copy link

@juliendoutre juliendoutre left a comment

Choose a reason for hiding this comment

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

The changes make sense to me ✅

I noticed you call the VerifyPlatformRequirements function before most commands are actually executed (warmup, detonate and revert). This function currently initialises its own providers with a different UUID than the ones used later during the command execution (handled through the runner's logic). Is it on purpose? Would it make sense to use the same UUID for both? Or maybe it does not matter?

@christophetd
Copy link
Contributor Author

Will add a comment - in that case it doesn't matter as it's only for checking authentication so we don't care about the UUID

@christophetd
Copy link
Contributor Author

Also reviewed by @rcobb-scwx who says the PR looks fine so I'll go ahead, merge it and release shortly

@christophetd christophetd merged commit bedcae3 into main Jan 30, 2023
@christophetd christophetd deleted the uuid-encapsulation branch January 30, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants