-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/168 revise the action iexecutor interface to allow for passing of in args and out args #186
base: development
Are you sure you want to change the base?
Conversation
@@ -150,7 +169,7 @@ Any successful HTTP response from an OpenC2 compliant endpoint (with a status co | |||
|
|||
#### Variables | |||
|
|||
It supports variable interpolation in the command, headers, and target definitions. | |||
This capability does not define specific variables as input, but variable interpolation is supported in the command and target definitions. It has `one` output variable of type string (see the `__soarca_openc2_http_result__`). If you want to use the output variable in your next step you will need to define a variable in your playbook as `step or playbook variabel` that is of the same type. Also you need to specify an `out_args` key see `__your_step_output_variable__` in the example. Note you can only use `one out_arg key` for this capability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix playbook variabel --> playbook variable
@@ -38,13 +38,22 @@ func TestHttpConnection(t *testing.T) { | |||
playbookId, _ := uuid.Parse("playbook--d09351a2-a075-40c8-8054-0b7c423db83f") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix for variable1 the type to Type: cacao.VariableTypeString
line 31
@@ -57,6 +64,9 @@ This module does not define specific variables as input, but variable interpolat | |||
"type": "ssh", | |||
"command": "ls -la" | |||
} | |||
], | |||
"out_args": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The agreed-upon, upcoming change in CACAO v2.1 (or v3), is to set out_args as a common property of the command. So if we want to anticipate this very reasonable change, we should set out_args within a command in commands. This also changes the description that we would have to provide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment applies to the rest of this document
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation to myself: overall, this PR makes the variables handling in SOARCA more explicit and deterministic, as now the only way to use SOARCA is by defining output variables in your step in the way that is supported by SOARCA processing.
Comment: it looks good to me - I only have two major remark.
- request changes/I am stupid remark: maybe I missed something, but I would expect these modifications to be pushed also for the playbook action step?
- general remark: CACAO v2.1/v.3 will move the out_args property inside the command object for action steps, and it will leave it unchanged in the playbook action step. While we can still push these changes now, we need to be aware that we will have to change the logic accordingly, as we are floating in a grey area of SOARCA-specific functioning for automation, and automation-facilitation attempts in the specification.
@@ -57,6 +64,9 @@ This module does not define specific variables as input, but variable interpolat | |||
"type": "ssh", | |||
"command": "ls -la" | |||
} | |||
], | |||
"out_args": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment applies to the rest of this document
@@ -32,7 +32,7 @@ In every other circumstance the step is considered to have failed. | |||
|
|||
#### Variables | |||
|
|||
This module does not define specific variables as input, but variable interpolation is supported in the command and target definitions. It has the following output variables: | |||
This module does not define specific variables as input, but variable interpolation is supported in the command and target definitions. It has `one` output variable of type string (see the `__soarca_ssh_result__`). If you want to use the output variable in your next step you will need to define a variable in your playbook as `step or playbook variable` that is of the same type. Also you need to specify an `out_args` key see `__your_step_output_variable__` in the example. Note you can only use `one out_arg key` for this capability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two points:
- We say that the output variable defined in the step can be used in the next steps only because in SOARCA we use global scope for any defined variable, right? We should mention this, as it's not exactly adhering to the spec.
- Besides than being the same type, it should also have the exact same name, right? Perhaps we can say that as well
@@ -33,7 +33,9 @@ func (finCapability *FinCapability) Execute( | |||
command cacao.Command, | |||
authentication cacao.AuthenticationInformation, | |||
target cacao.AgentTarget, | |||
variables cacao.Variables) (cacao.Variables, error) { | |||
variables cacao.Variables, | |||
inputVariableKeys []string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the difference in the property names "inputVariable" in the interface, and "inputVariableKeys" in the capabilities, on purpose? If not, I would name them the same way, just for consistency?
internal/capability/openc2/openc2.go
Outdated
@@ -16,6 +17,7 @@ type OpenC2Capability struct { | |||
type Empty struct{} | |||
|
|||
const ( | |||
maxResultVariables = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How/where is this used? I don't see it in the other capabilities or in the rest of the code of this capability but I haven't looked in depth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not used it remained of my first implementation so I will remove it good catch
@@ -44,7 +45,9 @@ func (httpCapability *HttpCapability) Execute( | |||
command cacao.Command, | |||
authentication cacao.AuthenticationInformation, | |||
target cacao.AgentTarget, | |||
variables cacao.Variables) (cacao.Variables, error) { | |||
variables cacao.Variables, | |||
inputVariableKeys []string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something, or are inputvariablekeys never used in the Execute function? This comment applies to all capabilities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true as there is no use for it in the spec at this point. We will need to see how we can implement this for now I wanted to include it in the interface as changing it again is a lot of work.
metadata.Agent) | ||
|
||
if len(metadata.Step.OutArgs) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move the out_args in the CACAO v3 position (inside a command object) then I think we will need to rethink where these checks will be performed
utils/mapper/mapper.go
Outdated
"soarca/models/cacao" | ||
) | ||
|
||
func Variables(variables cacao.Variables, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity, readability, and debugging, I would rather call these arguments something like "variablesInScope", "outArgs", "returnedVariables", "capabilityReturnArgs"
I missed the Playbook Action step so we need to update it in there as well |
46108da
to
7d1a7e9
Compare
be491f2
to
96385c5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaartendeKruijf I have a couple notes for how the assignment of variable values to out_args maps.
if !found { | ||
return cacao.NewVariables(), errors.New("key is not found in variables") | ||
} | ||
dataVariable, ok := returnedVariables.Find(capabilityReturnArgs[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic assumes that:
- a capability/command can return more than one arg separately (which might not be the case if we indeed can practically just assign the output of a command to one variable)
- the playbook implementer knows how many arguments the invocation of a command (capability) returns,
- the arguments returned from the command (capability) are in an order consistent with the order of the variables in out_args
If we're fine with these assumptions, we should at least mention this functioning in the documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are documented in modules
0dccb59
to
02c5d99
Compare
02c5d99
to
70cc0a3
Compare
No description provided.