Add the ability to provide target information for a TTP #332

l50 · 2023-09-15T04:13:15Z

Proposed Changes

This pull request includes the following changes:

Added targets specification and related changes to TTPForge:
- Added new launch configurations in .vscode/launch.json.
- Extended the run command in cmd/run.go to support target specification.
- Updated the TTP data structure in pkg/blocks/ttps.go to reflect target specs.
- Modified loading and validation of TTPs in pkg/blocks/loader.go to process new target specification.
- Added a new targets package to handle target specifications with an accompanying README.
Added testSpec_test and created new tests in ttps_test.
Enhanced the TTP struct with target and cloud attributes and added relevant tests:
- Extended the TTP structure to support detailed targeting, including OS, architecture, and cloud specifications.
- Added new Targets and Cloud structures to capture specific target attributes.
- Updated unmarshalling functions to accommodate the new fields.
- Extended test cases to cover the new functionality in TTP.
- Closes 💡 [REQUEST] - OS and Arch info #196.

Related Issue(s)

Closes #196.

Testing

I have performed the following testing to validate the changes:

Ran mage runprecommit locally and fixed any issues that arose.
Ran mage runtests locally and fixed any issues that arose.

Documentation

I have updated the documentation to reflect the changes made in this pull request, including adding details about the new targets specification and related changes, as well as the enhancements to the TTP struct.

Screenshots/GIFs (optional)

N/A

Checklist

Ran mage runprecommit locally and fixed any issues that arose.
Curated your commits so they are legible and easy to read and understand.
🚀

l50 · 2023-09-15T04:14:24Z

Examples for the new functionality can be found here: https://github.com/facebookincubator/ForgeArmory/pull/82/files#diff-af74b353728adb917c4c783f3e1126a723cc3dce43d52aa1ba3b7e1034d7007c

…tests - Extended the TTP structure to support detailed targeting, including OS, architecture, and cloud specifications. - Added new `Targets` and `Cloud` structures to capture specific target attributes. - Updated unmarshalling functions to accommodate the new fields. - Extended test cases to cover the new functionality in TTP. - Closes facebookincubator#196

- Add new launch configurations in `.vscode/launch.json`. - Extend `run` command in `cmd/run.go` to support target specification. - Update TTP data structure in `pkg/blocks/ttps.go` to reflect target specs. - Modify loading and validation of TTPs in `pkg/blocks/loader.go` to process new target specification. - Add a new `targets` package to handle target specifications with an accompanying README.

- Addresses concern outlined in facebookincubator#342 (comment)

d3sch41n

I think there some tough foundational issues here - kindly hold off on this for now, we'll discuss next week and align on a solution that accomplishes what you want

d3sch41n · 2023-09-21T11:45:17Z

cmd/run.go

@@ -62,6 +63,7 @@ func buildRunCommand() *cobra.Command {
 	runCmd.PersistentFlags().BoolVar(&ttpCfg.NoCleanup, "no-cleanup", false, "Disable cleanup (useful for debugging and daisy-chaining TTPs)")
 	runCmd.PersistentFlags().UintVar(&ttpCfg.CleanupDelaySeconds, "cleanup-delay-seconds", 0, "Wait this long after TTP execution before starting cleanup")
 	runCmd.Flags().StringArrayVarP(&argsList, "arg", "a", []string{}, "variable input mapping for args to be used in place of inputs defined in each ttp file")
+	runCmd.Flags().StringArrayVarP(&targetsList, "target", "t", []string{}, "variable input mapping to specify targets")


This doesn't seem to be used - it looks like you want to pass it through like the arg values:

argValues, err := args.ParseAndValidate(tmpContainer.ArgSpecs, argsKvStrs)

But notice how argsKvStrs is used there. Your parsing doesn't appear to look at these CLI values - you pass through targetsKvStrs but don't use it anywhere in LoadTTP, you just look at the specs.

targetValues, err := targets.ParseAndValidateTargets(tmpContainer.TargetSpec) if err != nil { return nil, fmt.Errorf("failed to parse and validate targets: %v", err) } execCfg.Targets = targetValues

Correct me if I'm wrong here

In any case, I don't think this "you can pass targets on the CLI" use case would make sense. It seems to be blurring three different notions together:

Where a TTP is capable of being run (ex: can't run macOS TTP on windows)

Where you are currently running a TTP

Command arguments for control flow

Regarding (1) I think what you really want to do here is just have a pre-execution check that you are running on a compatible platform, so that the user receives an intelligible error as early as possible in the TTP execution process.

Regarding (2): For instance, if a TTP supports both macOS and linux, and you need to check which one you are executing on at runtime in order to modify TTP behavior (for instance, using a different variant of the find command), then you should just reference runtime.GOOS (possibly packed into ExecutionConfig in some form) - a user passing a different target on the command line doesn't actually change the platform on which you are running.

Regarding (3): A risk with the current approach is that you end up re-implementing everything that args handles as far as validation goes. That'll be tricky to centralize - will need to be careful

d3sch41n · 2023-09-21T12:04:52Z

pkg/blocks/loader.go

 // ttp: Pointer to the created TTP instance, or nil if the file is empty or invalid.
 // err: An error if the file contains invalid data or cannot be read.
-func LoadTTP(ttpFilePath string, fsys afero.Fs, execCfg *TTPExecutionConfig, argsKvStrs []string) (*TTP, error) {
-
+func LoadTTP(ttpFilePath string, fsys afero.Fs, execCfg *TTPExecutionConfig, argsKvStrs []string, targetsKvStrs []string) (*TTP, error) {


ref comment above - I don't think targetsKvStrs is used, it can be removed

d3sch41n · 2023-09-21T12:22:23Z

pkg/blocks/ttps.go

+// Region: The name of the cloud region to be targeted.
+type Cloud struct {
+	Provider string `yaml:"provider,omitempty"`
+	Region   string `yaml:"region,omitempty"`


Region definitely feels like a regular CLI argument, not a "this TTP only works under these conditions" specifications like the rest of your targets - I wouldn't put it in here. If you leave it as a CLI argument, you can benefit from existing args machinery around default values for instance

d3sch41n · 2023-09-21T12:24:38Z

pkg/targets/targetSpec.go

+//
+// Provider: The name of the cloud provider to be targeted.
+// Region: The name of the cloud region to be targeted.
+type Cloud struct {


This definition is duplicated in blocks/ttps.go

d3sch41n · 2023-09-21T12:29:56Z

pkg/targets/targetSpec.go

+	processedTargets := make(map[string]interface{})
+
+	if len(targetSpec.OS) > 0 {
+		processedTargets["os"] = targetSpec.OS


you will need to check that the provided values are valid GOOS values

d3sch41n · 2023-09-21T12:30:20Z

pkg/targets/targetSpec.go

+	}
+
+	if len(targetSpec.Arch) > 0 {
+		processedTargets["arch"] = targetSpec.Arch


same as above - check against valid GOARCH options

d3sch41n · 2023-09-21T12:32:59Z

pkg/targets/targetSpec.go

+		processedTargets["arch"] = targetSpec.Arch
+	}
+
+	if len(targetSpec.Cloud) > 0 {


I don't think the if is necessary, you can just do the for loop, will be skipped if length is zero

d3sch41n · 2023-09-21T12:37:35Z

pkg/targets/targetSpec.go

+// Arch: A slice containing the architectures valid for this TTP.
+// Cloud: A slice of Cloud structs, each representing a cloud provider and region valid for this TTP.
+type TargetSpec struct {
+	OS    []string `yaml:"os,omitempty"`


you probably want to change structure up a bit so that OS and Arch are explicitly coupled - I think it's increasingly realistic to have a TTP that's compatible with linux/amd64 and darwin/arm64, but not darwin/amd64 for example. The current approach will end up treating OS and Arch as a two dimensional matrix in which all cells are assumed compatible.

d3sch41n · 2023-09-21T12:40:35Z

pkg/blocks/ttps.go

+// Targets: A map of targets to be set for the TTP.
+type Targets struct {
+	OS    []string `yaml:"os,omitempty"`
+	Arch  []string `yaml:"arch,omitempty"`


This type does not appear to ever be used, as for spec purposes it is redundant with TargetSpec and your runtime target information in context.go is stored in map[string]any

d3sch41n · 2023-09-21T12:41:38Z

pkg/blocks/context.go

@@ -35,6 +35,7 @@ type TTPExecutionConfig struct {
 	NoCleanup           bool
 	CleanupDelaySeconds uint
 	Args                map[string]any
+	Targets             map[string]any


This probably shouldn't be map[string]any - Args is that data type because it stores strings, ints, bools, etc - but all your stuff is strings

d3sch41n · 2023-10-03T11:48:07Z

Closing - will reimplement as spec'd here:

#328

l50 requested review from d3sch41n, cedowens, godlovepenn and twinvega as code owners September 15, 2023 04:13

facebook-github-bot added the cla signed label Sep 15, 2023

TTPForge-bot added the area/go Changes made to go resources label Sep 15, 2023

l50 enabled auto-merge (squash) September 15, 2023 04:14

l50 changed the title ~~Os and arch metadata~~ Add the ability to provide target information for a TTP Sep 15, 2023

l50 added 5 commits September 19, 2023 12:36

Fixed test case for targets

6570e99

Add testSpec_test; create new tests in ttps_test

edd8ff0

Addressed leftover concern from PR-342

084d4f6

- Addresses concern outlined in facebookincubator#342 (comment)

l50 force-pushed the os-and-arch-metadata branch from 4ad3b46 to 084d4f6 Compare September 19, 2023 19:00

d3sch41n requested changes Sep 21, 2023

View reviewed changes

d3sch41n closed this Oct 3, 2023

auto-merge was automatically disabled October 3, 2023 11:48
Pull request was closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to provide target information for a TTP #332

Add the ability to provide target information for a TTP #332

l50 commented Sep 15, 2023

l50 commented Sep 15, 2023

d3sch41n left a comment

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n Sep 21, 2023

d3sch41n commented Oct 3, 2023

Add the ability to provide target information for a TTP #332

Add the ability to provide target information for a TTP #332

Conversation

l50 commented Sep 15, 2023

Proposed Changes

Related Issue(s)

Testing

Documentation

Screenshots/GIFs (optional)

Checklist

l50 commented Sep 15, 2023

d3sch41n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d3sch41n commented Oct 3, 2023