Templated Bash TTPs #61

l50 · 2023-04-22T17:38:12Z

Proposed Changes

Added templated bash TTP generation
Created unit tests for new exported functions
Created integration tests for new functionality

Related Issue(s)

TTP Template - Bash #20

Testing

Created integration tests to represent the testing that I was doing throughout development.

Run the following:

go build

# Create bash TTP that employs logic from a provided bash script file.
./ttpforge -c config.yaml new ttp --path ttps/lateral-movement/ssh/rogue-ssh-key.yaml --template bash --ttp-type file --args "arg1,arg2,arg3" --cleanup --env "EXAMPLE_ENV_VAR=example_value"

# Create bash TTP that employs inline logic provided in the ttp YAML.
./ttpforge -c config.yaml new ttp --path ttps/lateral-movement/ssh/ssh-master-mode.yaml --template bash --ttp-type basic --cleanup

Documentation

N/A

Screenshots/GIFs (optional)

N/A

Checklist

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

- Working on adding the ability to create a new ttp with templates - Closes #55

- Working bash template for basic and file-based TTPs - All new exported functions are commented and have unit tests

- Created integration tests for new.go - Closes #20 - Fixed incorrect path in pre-existing executeYAML_test

d3sch41n

Blocking on the Chdir beahvior as I think that's an important design issue to iron out - strongly encourage reconsidering that approach so that we can support brew install ttpforge, internal TTP repos (for both us and clients), etc - happy to hear your thoughts

pkg/files/file.go

pkg/files/file_test.go

cmd/run.go

cmd/root.go

cmd/new.go

cmd/executeYAML_test.go

cmd/new_test.go

- Flagged by Sam here: #61 (comment)

- ExecuteYAML and test now live in the files package as per Sam's recommendations and feedback - Duplicate functionality in `run.go` - appears to be resolved in trunk already - Results from age of branch and when it was created - Added new functions to the files package as per Sam's feedback here: #61 (comment)

- Inlined test files employed for TestExecuteYAML - Removed old directories that appeared to be leftover from a previous PR - Updated yaml_test to table testing format as per https://github.com/facebookincubator/TTPForge/blob/main/docs/testing.md

cmd/root.go

pkg/files/file.go

l50 · 2023-04-28T20:13:20Z

The concerns outlined by you all should be addressed now. I just need to fix a few of the tests and we should be good.

- Decoupled bash reliance for template creation - Tests updated and working - Add ExpandHomeDir() and test - Updated TestPathExistsInInventory to be more realistic - Moved bash templates to their own directory - Fixed pkg tests to specifications in PR - Updated TemplateExists to support multiple inventoryPaths

- Updated codespell to not check go test files

l50 · 2023-04-29T01:34:32Z

Alright, everything is passing and ready for another review from you all.

d3sch41n

Unfortunately, the tests still don't actually verify that the template code works as expected (see specific comment at appropriate point in the code) - you can still make arbitrary breaking changes and the tests will pass. I think you have three options:

make the test assert against the template file contents, rather than trying to run it
Call a lower level function (like ExecuteYAML) to run the generated template and check it's error status
Remove the challenging template tests - make a TODO to add tests that can catch template regressions, land your PR and add them later

Any of these work for me, I can stamp once you implement one of them

To prevent this type of defect from sneaking through in the future, I recommend the TDD approach (in spirit if not literal practice) of deliberately introducing breaking changes to see if your tests break, then digging in and debugging if they don't break as expected.

cmd/new_test.go

d3sch41n · 2023-05-02T17:44:42Z

cmd/new_test.go

+			runOutput := new(bytes.Buffer)
+			runCmd.SetOut(runOutput)
+
+			err = runCmd.Execute()


This still doesn't error out on an invalid template, so it won't catch regressions as you intent to with this test - I can modify templates/bash/bashTTP.yaml.tmpl to have the contents THIS SHOULD CAUSE A FAILURE BUT DOES NOT and the test will still pass - it's using the correct template after your latest change, but runCmd.Execute() does not return an error because that cobra command is just implemented with a Run() function that can't return an error.

I'm not sure there's an easy way around that - you could try changing Run() command to use log.Fatal but I think that will just straight-up kill your test before you hit any asserts, which will be very confusing for users. Putting more discussions in review summary.

Test result for reference (the ERROR entries come from logging but don't actually cause the test to fail)

=== RUN TestCreateAndRunTTP OMG TEST DIR: /var/folders/5w/hrcjpf993snbdj3236k6wvt80000gn/T/cmd-new-test2968834320 === RUN TestCreateAndRunTTP/Create_basic_bash_TTP 2023-05-02T12:32:42.150-0500 INFO cmd/root.go:160 Using config file: config.yaml 2023-05-02T12:32:42.154-0500 INFO cmd/root.go:160 Using config file: config.yaml 2023-05-02T12:32:42.154-0500 ERROR files/yaml.go:38 failed to run TTP {"error": "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `THIS SH...` into blocks.TTPTmpl"} 2023-05-02T12:32:42.154-0500 ERROR cmd/run.go:42 failed to execute TTP {"error": "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `THIS SH...` into blocks.TTPTmpl"} === RUN TestCreateAndRunTTP/Create_file-based_bash_TTP 2023-05-02T12:32:42.154-0500 INFO cmd/root.go:160 Using config file: config.yaml 2023-05-02T12:32:42.156-0500 INFO cmd/root.go:160 Using config file: config.yaml 2023-05-02T12:32:42.157-0500 ERROR files/yaml.go:38 failed to run TTP {"error": "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `THIS SH...` into blocks.TTPTmpl"} 2023-05-02T12:32:42.157-0500 ERROR cmd/run.go:42 failed to execute TTP {"error": "yaml: unmarshal errors:\n line 1: cannot unmarshal !!str `THIS SH...` into blocks.TTPTmpl"} --- PASS: TestCreateAndRunTTP (0.01s) --- PASS: TestCreateAndRunTTP/Create_basic_bash_TTP (0.01s) --- PASS: TestCreateAndRunTTP/Create_file-based_bash_TTP (0.00s) PASS ok github.com/facebookincubator/ttpforge/cmd 1.020s

So this runCmd.Execute will never error because there is no error return on RunTTPCmd. You will need to change the function of RunTTPCmd to return an error otherwise there is no way to flag this for success/failure

CrimsonK1ng · 2023-05-02T19:28:48Z

pkg/files/file.go

+//	if err != nil {
+//	    log.Fatalf("failed to create directory: %v", err)
+//	}
+func CreateDirIfNotExists(fsys afero.Fs, path string) error {


Unused, remove in future

pkg/files/file.go

cmd/run.go

CrimsonK1ng · 2023-05-02T19:47:57Z

pkg/blocks/ttps.go

+
+	// This section is necessary to get the proper formatting.
+	// Resource: https://pkg.go.dev/gopkg.in/yaml.v3#section-readme
+	m := make(map[interface{}]interface{})


The usage of interface in both areas removes the ability to validate when writing back to disk. In the Marshal section of the readme you can use the defined struct in order to Marshal back out https://pkg.go.dev/gopkg.in/yaml.v3#Marshal

- Update TestCreateAndValidateTTP to check TTPForge's response to invalid TTPs

…ertain inputs

…ces aren't present

l50 · 2023-05-03T18:24:52Z

as such, I can make a breaking edit to templates/bash/bashTTP.bash.tmpl and give it the following contents:
#!/bin/bash
set -ex

GET REKT!!!!

It won't run correctly:

~/pt/tools/ttpforge (i20*) » ./ttpforge -c config.yaml run ttps/lateral-movement/ssh/rogue-ssh-key.yaml jaysong@jaysong-mp
2023-05-03T12:20:44.442-0600 INFO cmd/root.go:160 Using config file: config.yaml
2023-05-03T12:20:44.443-0600 INFO blocks/ttps.go:204 [*] Validating Steps
2023-05-03T12:20:44.443-0600 INFO blocks/filestep.go:258 executor set via extension {"exec": "sh"}
2023-05-03T12:20:44.444-0600 INFO blocks/ttps.go:215 [+] Finished validating steps
2023-05-03T12:20:44.444-0600 INFO blocks/ttps.go:220 [+] Running current TTP: rogue-ssh-key.yaml
2023-05-03T12:20:44.444-0600 INFO blocks/ttps.go:226 [+] Running current step: example_file_step
2023-05-03T12:20:44.444-0600 INFO blocks/filestep.go:172 ========= Executing ==========

GET 'REKT?'
/Users/jaysong/pt/tools/TTPForge/templates/bash/bashTTP.bash: line 4: GET: command not found
2023-05-03T12:20:44.456-0600 ERROR blocks/filestep.go:207 bad exit of process {"stdout": "", "stderr": "+ GET 'REKT?'\n/Users/jaysong/pt/tools/TTPForge/templates/bash/bashTTP.bash: line 4: GET: command not found\n", "exit code": 127}
2023-05-03T12:20:44.457-0600 ERROR blocks/filestep.go:176 {error 26 0 exit status 127}
2023-05-03T12:20:44.457-0600 ERROR blocks/ttps.go:230 error encountered in stepCopy execution: %v {"error": "exit status 127"}
2023-05-03T12:20:44.457-0600 ERROR files/yaml.go:95 failed to run TTP {"error": "exit status 127"}
2023-05-03T12:20:44.457-0600 ERROR cmd/run.go:42 failed to execute TTP {"error": "exit status 127"}

but the tests are passing, which is not ideal

I think this is a case of "over-mocking" - in this case, you need to use the real template files from templates/ because that's the actual functionality that you need to test - concerns about "is it idiomatic to reference the file system in tests" are secondary to this imperative.

We are using real template files for the tests - see this line in new_test.go:

if err := copy. Copy(templatesDir, filepath.Join(testDir, "templates")); err != nil {
		t.Fatalf("failed to copy templates dir: %v", err)
	}

CrimsonK1ng

Tests work now, other details present can be resolved at a later date.

d3sch41n

approved - left some pointers for the future, no need to change anything but if u could add a reply comment on whether or not the marshal stuff is dead code that'd be helpful

ship it

d3sch41n · 2023-05-03T19:14:23Z

pkg/blocks/ttps.go

+// interface{}: The formatted YAML string representing the TTP object.
+//
+// error: An error if the encoding process fails.
+func (t *TTP) MarshalYAML() (interface{}, error) {


Is this code leftover from an earlier implementation? I was checking if the tests fail when I screw it up (to verify that issues noted earlier are addressed) and found that when I deleted the whole thing all tests still pass - the diff of my change:

-// MarshalYAML is a custom marshalling implementation for the TTP structure. It encodes a TTP object into a formatted -// YAML string, handling the indentation and structure of the output YAML. -// -// Returns: -// -// interface{}: The formatted YAML string representing the TTP object. -// -// error: An error if the encoding process fails. -func (t *TTP) MarshalYAML() (interface{}, error) { - marshaled, err := yaml.Marshal(*t) - if err != nil { - return nil, fmt.Errorf("failed to marshal TTP to YAML: %v", err) - } - - // This section is necessary to get the proper formatting. - // Resource: https://pkg.go.dev/gopkg.in/yaml.v3#section-readme - m := make(map[interface{}]interface{}) - - err = yaml.Unmarshal(marshaled, &m) - if err != nil { - return nil, fmt.Errorf("failed to unmarshal YAML: %v", err) - } - - b, err := yaml.Marshal(m) - if err != nil { - return nil, fmt.Errorf("failed to marshal back to YAML: %v", err) - } - - formattedYAML := reduceIndentation(b, 2) - - return fmt.Sprintf("---\n%s", string(formattedYAML)), nil -} - -func reduceIndentation(b []byte, n int) []byte { - lines := bytes.Split(b, []byte("\n")) - - for i, line := range lines { - // Replace tabs with spaces for consistent processing - line = bytes.ReplaceAll(line, []byte("\t"), []byte(" ")) - - trimmedLine := bytes.TrimLeft(line, " ") - indentation := len(line) - len(trimmedLine) - if indentation >= n { - lines[i] = bytes.TrimPrefix(line, bytes.Repeat([]byte(" "), n)) - } else { - lines[i] = trimmedLine - } - } - - return bytes.Join(lines, []byte("\n")) -} -

All tests pass

❯ go test ./... ? github.com/facebookincubator/ttpforge [no test files] ok github.com/facebookincubator/ttpforge/cmd 0.373s ok github.com/facebookincubator/ttpforge/pkg/blocks 0.196s ok github.com/facebookincubator/ttpforge/pkg/files 0.285s ok github.com/facebookincubator/ttpforge/pkg/logging (cached) ok github.com/facebookincubator/ttpforge/pkg/strings (cached)

Sanity check - I'm on latest commit of the PR branch:

❯ git rev-parse --abbrev-ref HEAD i20 ❯ git rev-parse HEAD 67ed7c0107b6a80a5592585928480fc33041534d

d3sch41n · 2023-05-03T19:20:52Z

cmd/new.go

+
+func createTTP() (*blocks.TTP, error) {
+	ttp := &blocks.TTP{
+		Name:        filepath.Base(newTTPInput.Path),


for the future - would be preferable to restrict the global var newTTPInput to just being used in the cobra flag parsing, then make the instance of NewTTPInput a function parameter of all these other functions, instead of using the global var everywhere - global vars in general cause lots of problems for testability/maintainability

d3sch41n · 2023-05-03T19:57:04Z

cmd/new_test.go

-
-			err = runCmd.Execute()
-			require.NoError(t, err, fmt.Sprintf("failed to run TTP: %v", err))
+			output := captureStdout(t, func() {


good stuff - exactly what I was looking for

d3sch41n · 2023-05-03T20:08:14Z

Just to clarify re the last comment

I think this is a case of "over-mocking" - in this case, you need to use the real template files from templates/ because that's the actual functionality that you need to test - concerns about "is it idiomatic to reference the file system in tests" are secondary to this imperative.

We are using real template files for the tests -...

Yup - my comment there referenced an earlier version of the PR

l50 added 4 commits April 22, 2023 11:04

Fixed some bugs in the container docs

0c7cfa8

Mocked ttp generation; bug fixes

8407b9f

- Working on adding the ability to create a new ttp with templates - Closes #55

Working to generate improperly formatted TTP yaml

defb118

Working bash template

cd933ac

- Working bash template for basic and file-based TTPs - All new exported functions are commented and have unit tests

facebook-github-bot added the cla signed label Apr 22, 2023

l50 changed the title ~~Templated~~ Templated Bash TTPs Apr 22, 2023

l50 force-pushed the i20 branch from 0b98c85 to acbc87e Compare April 22, 2023 17:39

l50 requested review from CrimsonK1ng and d3sch41n April 22, 2023 17:40

Updated exiting cobra for testing; minor fix

bc66f0a

- Created integration tests for new.go - Closes #20 - Fixed incorrect path in pre-existing executeYAML_test

l50 force-pushed the i20 branch from acbc87e to bc66f0a Compare April 25, 2023 03:29

l50 added enhancement New feature or request type/major labels Apr 25, 2023

d3sch41n requested changes Apr 26, 2023

View reviewed changes

l50 added 4 commits April 26, 2023 13:51

Resolved issue with CreateDirIfNotExists

7a6ea24

- Flagged by Sam here: #61 (comment)

Update TTPInput

d7755c2

Improvements for yaml_test

35e8605

- Inlined test files employed for TestExecuteYAML - Removed old directories that appeared to be leftover from a previous PR - Updated yaml_test to table testing format as per https://github.com/facebookincubator/TTPForge/blob/main/docs/testing.md

l50 force-pushed the i20 branch from 1a5239c to 35e8605 Compare April 26, 2023 22:14

Updated container doc to avoid conflicts with trunk

7da947a

CrimsonK1ng reviewed Apr 27, 2023

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

CrimsonK1ng reviewed Apr 27, 2023

View reviewed changes

pkg/files/file.go Outdated Show resolved Hide resolved

l50 force-pushed the i20 branch from 2681193 to 12f873d Compare April 28, 2023 20:11

l50 force-pushed the i20 branch 2 times, most recently from 27f3a06 to 4ca6a07 Compare April 28, 2023 20:23

l50 force-pushed the i20 branch from 4ca6a07 to 1c761fc Compare April 29, 2023 01:13

Linting; minor pc update

9054225

- Updated codespell to not check go test files

l50 added 2 commits May 2, 2023 10:38

Employ real templates for new_test.go

03559d4

Fixed incorrect template parsing path

77c4d24

l50 requested review from d3sch41n and CrimsonK1ng May 2, 2023 17:41

d3sch41n requested changes May 2, 2023

View reviewed changes

Replace createBashTestTemplates with otiai10/copy

0e79f99

CrimsonK1ng reviewed May 2, 2023

View reviewed changes

pkg/files/file.go Show resolved Hide resolved

CrimsonK1ng reviewed May 2, 2023

View reviewed changes

cmd/run.go Show resolved Hide resolved

CrimsonK1ng reviewed May 2, 2023

View reviewed changes

l50 added 6 commits May 2, 2023 21:33

Input validation fixes; increased test coverage

b45b8d7

- Update TestCreateAndValidateTTP to check TTPForge's response to invalid TTPs

Reduced complexity of functions

7b6ddcc

Updated TemplateExists to return the full path instead of a bool

9a1406a

Moved check for file existence w/ fs.StatFS so that it doesn't miss c…

652825d

…ertain inputs

Add more complete exported func comment; var renaming for clarity

69a997c

Fixed minor bugs in yaml_test

0e22c36

l50 requested review from d3sch41n and CrimsonK1ng May 3, 2023 07:43

l50 mentioned this pull request May 3, 2023

💡 [REQUEST] - Go TTP Template #79

Open

Update ExpandHomeDir to trim the input path and ensure unexpected spa…

321c0bf

…ces aren't present

l50 added 2 commits May 3, 2023 12:46

Ensure that output from running TTPs is checked for integration tests

2481a9d

Added copyright header to embed.go

67ed7c0

CrimsonK1ng approved these changes May 3, 2023

View reviewed changes

d3sch41n approved these changes May 3, 2023

View reviewed changes

l50 merged commit e143818 into main May 3, 2023

l50 deleted the i20 branch May 3, 2023 20:31

l50 mentioned this pull request May 3, 2023

The usage of interface in both areas removes the ability to validate when writing back to disk. In the Marshal section of the readme you can use the defined struct in order to Marshal back out https://pkg.go.dev/gopkg.in/yaml.v3#Marshal #80

Closed

CrimsonK1ng mentioned this pull request May 4, 2023

Additional E2E Tests and Bugfix #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Templated Bash TTPs #61

Templated Bash TTPs #61

l50 commented Apr 22, 2023 •

edited

Loading

d3sch41n left a comment

l50 commented Apr 28, 2023 •

edited

Loading

l50 commented Apr 29, 2023

d3sch41n left a comment

d3sch41n May 2, 2023

CrimsonK1ng May 3, 2023

CrimsonK1ng May 2, 2023

CrimsonK1ng May 2, 2023

l50 commented May 3, 2023

CrimsonK1ng left a comment

d3sch41n left a comment

d3sch41n May 3, 2023

d3sch41n May 3, 2023

d3sch41n May 3, 2023

d3sch41n commented May 3, 2023

Templated Bash TTPs #61

Templated Bash TTPs #61

Conversation

l50 commented Apr 22, 2023 • edited Loading

Proposed Changes

Related Issue(s)

Testing

Documentation

Screenshots/GIFs (optional)

Checklist

d3sch41n left a comment

Choose a reason for hiding this comment

l50 commented Apr 28, 2023 • edited Loading

l50 commented Apr 29, 2023

d3sch41n left a comment

Choose a reason for hiding this comment

d3sch41n May 2, 2023

Choose a reason for hiding this comment

CrimsonK1ng May 3, 2023

Choose a reason for hiding this comment

CrimsonK1ng May 2, 2023

Choose a reason for hiding this comment

CrimsonK1ng May 2, 2023

Choose a reason for hiding this comment

l50 commented May 3, 2023

CrimsonK1ng left a comment

Choose a reason for hiding this comment

d3sch41n left a comment

Choose a reason for hiding this comment

d3sch41n May 3, 2023

Choose a reason for hiding this comment

d3sch41n May 3, 2023

Choose a reason for hiding this comment

d3sch41n May 3, 2023

Choose a reason for hiding this comment

d3sch41n commented May 3, 2023

l50 commented Apr 22, 2023 •

edited

Loading

l50 commented Apr 28, 2023 •

edited

Loading