Add external process execution guidelines #4652
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4652 +/- ##
==========================================
+ Coverage 90.48% 90.74% +0.25%
==========================================
Files 179 180 +1
Lines 10594 10652 +58
==========================================
+ Hits 9586 9666 +80
+ Misses 785 765 -20
+ Partials 223 221 -2
Continue to review full report at Codecov.
|
| - If an external process needs to be executed limit and hard-code the location where | ||
| the executable file may be located, instead of allowing the input to dictate the |
There was a problem hiding this comment.
| - If an external process needs to be executed limit and hard-code the location where | |
| the executable file may be located, instead of allowing the input to dictate the | |
| - If an external process needs to be executed, limit and hard-code the absolute path | |
| to the executable file instead of allowing the input to dictate the |
We should be clear that an absolute path should be specified and no reliance on $PATH should be allowed.
There was a problem hiding this comment.
I do not think we need to limit this to the absolute path only. As long as it is a strictly predictable path I think it should be allowed. For example one good approach could that we we put otelcol executable in some directory, e.g. /usr/bin/otelcol and allow execution from a subdirectory with a hard-coded name, e.g. anything located in /usr/bin/otelcolscripts/ is allowed. In the code this can be expressed as a directory relative to the directory where otelcol executable is located, yet I believe it is sufficiently safe.
I do agree that reliance on $PATH should not be allowed.
What do you think?
There was a problem hiding this comment.
I think that allowing relative paths are dangerous. If the otelcol executable is in /usr/bin then specifying . or .. or ../sbin seem undesirable. Having the behavior change if the executable is moved also seems potentially surprising. That said, I think what you've proposed would be better than doing nothing and shouldn't prevent us from further tightening the restrictions in the future.
There was a problem hiding this comment.
If the otelcol executable is in /usr/bin then specifying . or .. or ../sbin seem undesirable.
Absolutely. I was unclear in my description. I think the end user should not be allowed to specify arbitrary relative paths (nor they should be allowed to specify arbitrary absolute paths for that matter, see below). However there may be an implementation logic that uses a hard-coded relative path to a well known place which cannot be exploited.
For example imagine we want to have a receiver that allows scripts to be executed and their output captured as log records. It would be absolutely unacceptable to allow the path to the scripts to be a config option and allow this path to be either a relative path or an absolute path because that would mean a compromised config file allows malicious actors to execute arbitrary commands. Limiting this config option to absolute paths doesn't prevent this. The problem is in allowing arbitrary directories to be specified in the config file.
As opposed to that imagine that we have a receiver that allows scripts to be executed, but the limitation is that the scripts must be located in a specific directory. To achieve this we can hard-code the directory to be for example /usr/bin/otelcolscripts and this absolute path can literally be a hard-coded string in the receiver implementation. Alternatively the receiver implementation can calculate this directory to be an otelcolscripts subdirectory relative to the location where otelcol executable is stored. This relative approach allows more flexibility in the placement of the otelcol executable and the scripts that we want to be executable, yet is limited enough that it is impossible to run arbitrary commands from /usr/bin or any other directory that is not explicitly blessed by virtue of having the right name.
|
@open-telemetry/collector-approvers please review. |
The guidelines are necessary to explain how we want to generally approach external process execution. This was recently brought up in open-telemetry/opentelemetry-collector-contrib#6512 If we accept these guidelines the following should happen: - The prometheusexecreceiver should be modified to allow only a hard-coded list of exporters. - The fluentbitextension should be either removed or significantly limited in terms of what locations and what executable file names it can allow. - open-telemetry/opentelemetry-collector-contrib#6512 will be rejected, possibly substituted by a plugin system that @zenmoto referred to in open-telemetry/opentelemetry-collector-contrib#6512 (comment) if we find useful to have such plugin system.
tigrannajaryan
force-pushed
the
update-guidelines
branch
from
09b8958 to
63aca88
Compare
Good point. We have open issues for prometheusexecreceiver and fluentbitextension, I think once we decide how exactly these need to work we can link to their implementations as examples. |
The guidelines are necessary to explain how we want to generally approach external process execution. This was recently brought up in open-telemetry/opentelemetry-collector-contrib#6512
If we accept these guidelines the following should happen:
limited in terms of what locations and what executable file names it can allow.
will be rejected, possibly substituted by a plugin system that @zenmoto
referred to in [extension/subprocess] Create subprocess extension (#6467) opentelemetry-collector-contrib#6512 (comment)
if we find useful to have such plugin system.