-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add Collector security documentation #5209
base: main
Are you sure you want to change the base?
Conversation
Still lots to do. I'll let everyone know when it's in a state fit for reviewing. 👍 |
|
||
We recommend enabling only the minimum required components for a Collector | ||
configuration: one receiver and one exporter. Configuring only the minimum set | ||
of components exposes a minimum set of required ports. |
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.
"Ports" seem to be too specific, consider rephrasing it to "exposes a minimum attack surface" - it could be TCP/UDP ports, component dependency (SBOM), etc.
## Receivers and exporters | ||
|
||
We recommend enabling only the minimum required components for a Collector | ||
configuration: one receiver and one exporter. Configuring only the minimum set |
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.
"one receiver and one exporter" seems very strict, what if the user wants to fork the data to two different targets? (e.g. are we suggesting that forking data is not recommended from security perspective?)
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.
I agree with @reyang -- where is this coming from?
settings using configuration parameters. If these settings are available, you | ||
should proceed with caution before modifying the default configuration values. | ||
Improperly setting these values might expose the OpenTelemetry Collector to | ||
additional attack vectors including resource exhaustion. |
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.
Resource exhaustion is a great topic, I feel the current description is a bit misleading - even if the settings are correct, OpenTelemetry Collector can still cause resource exhaustion - is this the current state or there is already a solid solution for resource governance?
otlp: | ||
protocols: | ||
grpc: | ||
endpoint: localhost:4317 |
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.
I don't recall the exact details, would this bind to both IPv4 and IPv6, or the Collector has a recommendation regarding how to specify IPv4/v6 explicitly?
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.
IIRC, "localhost" binds to both. In fact, localhost is the default today already, so this recommendation might need to be reworded, as it's probably reminiscent from the days where we bound to all IPs (0.0.0.0).
Collector configuration. Running a secure Collector can help you | ||
|
||
- Protect telemetry that might contain sensitive information, such as personally | ||
identifiable information (PII), application-specific data, or network traffic |
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.
I'm pretty sure we have a guidance somewhere stating that people should not store PII in telemetry data. Perhaps we should reinforce it here, and add a link to that place? Something like: "help you protect telemetry that shouldn't, but might contain sensitive information, such ..."
## Receivers and exporters | ||
|
||
We recommend enabling only the minimum required components for a Collector | ||
configuration: one receiver and one exporter. Configuring only the minimum set |
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.
I agree with @reyang -- where is this coming from?
of components exposes a minimum set of required ports. | ||
|
||
- Receivers and exporters can be either push- or pull-based. In either case, the | ||
connection established should be over a secure and authenticated channel. |
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.
connection established should be over a secure and authenticated channel. | |
connection established should at least be over a secure channel, potentially authenticated as well. |
The idea is: always secure (even on networks that you deem as trusted), and while authentication is recommended, it can be waived on internal network communications.
|
||
- Receivers and exporters can be either push- or pull-based. In either case, the | ||
connection established should be over a secure and authenticated channel. | ||
- Unused receivers and exporters should be disabled to minimize the attack |
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? What does that mean concretely? Just by not invoking those components via config file? The current recommendation is to use OCB to only have the required components as part of the binary, effectively reducing the attack surface. But this might have been mentioned later already?
otlp: | ||
protocols: | ||
grpc: | ||
endpoint: localhost:4317 |
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.
IIRC, "localhost" binds to both. In fact, localhost is the default today already, so this recommendation might need to be reworded, as it's probably reminiscent from the days where we bound to all IPs (0.0.0.0).
|
||
### Forwarding | ||
|
||
A forwarding extension is used when you need to collect telemetry that's not |
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.
I feel like this is misplaced here... I don't think the forwarding extension is related to security, nor is a best practice.
while the `host_observer` requires the OpenTelemetry Collector to run in | ||
privileged mode. | ||
|
||
<!--- But what about Juraci's comment here: https://github.com/open-telemetry/opentelemetry.io/pull/3652/files?diff=unified&w=0#r1417409370 ---> |
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's OK -- what we shouldn't be telling people is to run things as root or blindly disable security protections (like selinux).
### Subprocesses | ||
|
||
Extensions can also be used to run subprocesses when the Collector can't | ||
natively run the collection mechanisms (for example, FluentBit). Subprocesses |
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.
I'm also not sure where this is coming from: the only component that I know of spawning new processes is the jmx receiver.
|
||
## Storing configuration information securely | ||
|
||
<!--- TODO: SHOULD ensure sensitive configuration information is stored securely. How? --> |
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.
Perhaps add a reference to the secrets management practices for Kubernetes?
|
||
To limit the exposure of servers to authorized users: | ||
|
||
- Enable authentication, using bearer token authentication extensions and basic |
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.
- Enable authentication, using bearer token authentication extensions and basic | |
- Enable authentication, for instance by using bearer token authentication extensions and basic |
This PR moves end user security documentation from a README in the Collector core repository to the OTel docs website.
Based on decisions in previous issues and PRs, the following are assumed:
Tracking issue: #3479
Related to: #3227
NOTE: Much of the work for this PR was done by @mjingle in #3652. With her permission, I am building on her efforts.