Skip to content
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

wip: Establish Security section for OTel documentation #3652

Closed
wants to merge 16 commits into from

Conversation

mjingle
Copy link
Contributor

@mjingle mjingle commented Dec 5, 2023

OpenTelemetry (OTel) currently does not have public documentation on security recommendations. A markdown file in the OpenTelemetry Collector repository (linked below) does address some concerns, but is not easily findable by end users.

The goal of this PR is to establish hosting and configuration security recommendations for OTel Collector end users within the OpenTelemetry public documentation site. While the inspiration document contains both end-user and Component Developer security recommendations, this public documentation adapts the existing content focused on end users and links back to the inspiration document for Component Developers.

NOTE: The inspiration document also does not cover topics in detail and I need help filling in those blanks. Refer to any <!-- TODO: --> within the files that indicates knowledge gaps. From there, the page content can be re-organized, given the available information.

This PR adds:

  • a Security Overview page
  • an OpenTelemetry Collector hosting best practices page
  • an OpenTelemetry Collector configuration best practices page

This proposed structure tries to be flexible for future Security content pages to be added. (Additional content doesn't have to focus on OTel Collector. Other Security-related areas in OTel can be addressed also, but that's not within the scope of this PR.)

Identified Areas of Potential Help

I already know this PR needs some extra help and this is my first time contributing a PR to this project. These include:

  • Filling in content gaps. (Refer to NOTE: above.)
  • Correct wiring for the menu. My goal is to have Security as a top level menu option.
  • Links to related content within the OTel community documentation, if applicable.
  • Amending the inspiration document in the OTel Collector repo to focus on Component Developers only.

Source Materials

https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/security-best-practices.md

Associated Issues

Initial issue: #3479
Related issue: #3227


Preview: https://deploy-preview-3652--opentelemetry.netlify.app/docs/security/

@mjingle mjingle requested a review from a team December 5, 2023 15:56
@mjingle mjingle marked this pull request as draft December 5, 2023 16:05
extensions are similar to receivers and exporters so the same security
considerations apply.

### Observers [component developers?]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am not familiar with Observers. Is this content related to component developers? If so, I can remove it from this PR for end users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for end-users. Perhaps an example would be useful here, for users to understand what they can do with observers.

Here's one such example, where a redis receiver is created if a redis pod is found on a Kubernetes cluster:

https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/k8sobserver

cc @rmfitzpatrick , @dmitryax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amended the header here, since Observers are for end-users. 👍

An example here would be good to have. Still need to incorporate what you shared, @jpkrohling, but I appreciate the resource you shared!

connection established SHOULD be over a secure and authenticated channel.
- Unused receivers and exporters SHOULD be disabled to minimize the attack
vector of the Collector.
- Receivers and Exporters MAY expose buffer, queue, payload, and/or worker
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a mixed audience: the first sentence targets component developers, then it proceeds with advice for users. I would rewrite as:

Some receivers and exporters might allow you tweak internal queues, buffers, and other parameters. Experiment with these values, but remember that values that are too wide (such as a large queue number) might mean that the Collector will attempt to use more memory than a setting with a smaller value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @jpkrohling! For values that are too wide, rather than:

remember that values that are too wide (such as a large queue number) might mean that the Collector will attempt to use more memory than a setting with a smaller value.

Would it be more accurate to describe as:

remember that if the values are too wide, such as a large queue number, then the OTel Collector may use more memory than necessary, compared to using a setting with a smaller value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling Circling back to ask your feedback on the above ^

Improperly setting these values may expose the OpenTelemetry Collector to
additional attack vectors including resource exhaustion.

It is possible that a receiver MAY require the OpenTelemetry Collector run in a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this if we don't have any such component today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we ask a specific person or team for confirmation about this assertion? (This sentence is located in the inspiration document here:https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/security-best-practices.md?plain=1#L124 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpkrohling Circling back to ask if there's an answer for the above-mentioned question?

data to a third-party backend. The OpenTelemetry Collector **SHOULD** be
configured to obfuscate or scrub sensitive data before exporting.

<!--- TODO: SHOULD configure obfuscation/scrubbing of sensitive metadata. How? Give more details and/or link to an existing document -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extensions are similar to receivers and exporters so the same security
considerations apply.

### Observers [component developers?]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for end-users. Perhaps an example would be useful here, for users to understand what they can do with observers.

Here's one such example, where a redis receiver is created if a redis pod is found on a Kubernetes cluster:

https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/k8sobserver

cc @rmfitzpatrick , @dmitryax

## Permissions

<!--- TODO: SHOULD not run the OpenTelemetry Collector as root/admin user. Why? (Give the reader motivation.) How do you do that?
- NOTE: MAY require privileged access for some components -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Collector SHOULD NOT require privileged access, except where the data it's obtaining is in a privileged location. For instance, in order to get pod logs by mounting a node volume, the Collector daemonset needs enough privileges to get that data.

The rule of least privilege applies here.


## Receivers and Exporters

<!--- TODO: SHOULD limit exposure of servers to authorized users. How do you do that? -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Authentication (bearer token auth extension, basic auth extension)
  • By restricting the IPs the collector is running

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial drive-by comments.

@svrnm svrnm mentioned this pull request Dec 8, 2023
@svrnm svrnm requested a review from a team December 13, 2023 09:40
@svrnm svrnm requested review from a team and djaglowski and removed request for a team December 13, 2023 09:40
@svrnm
Copy link
Member

svrnm commented Dec 13, 2023

Thinking about the "where to put this" once again: We now have established /docs/security/ and have the CVE there and will also have more content like #3675 as well. This PR contains security guidelines for the collector, so we have technically 2 places to house this:

  1. /docs/security/collector
  2. /docs/collector/security

I think we should go for option (2) but at the same time add a "Practices" page under /docs/security/practices that will list collector (and later other components as well) and link back to the component specific security, wdyt?

@jpkrohling
Copy link
Member

Thinking out "loud", there are two moments I'm concerned about security for my collector:

  • when evaluating the collector
  • when hardening it right before going into production

For the first situation, I'd prefer the security documentation to be close to the getting started, so that I can have an idea whether the collector satisfies my security needs. During the second situation, I think I'd prefer it close to the collector documentation again, as I probably don't care about the security aspects of the SDK or other parts.

In summary: I'm with you that placing it closer to the collector makes sense, as long as there's a reference to it from within the /security pages.

@cartersocha
Copy link
Contributor

I vote put it in the collector docs and agree we can link out to it

@mjingle
Copy link
Contributor Author

mjingle commented Dec 13, 2023

@svrnm My question is why not have the content in both locations? (This may not be possible with the functionality/architecture of the OTel docs site, but I still ask the question.)

I agree that when setting up the OTel Collector, information on securing your OTel Collector is very relevant and should exist within the OTel Collector documentation.

In the original vision by establishing this Security section, we're also:

  1. trying to highlight to potential OTel adopters that you can be secure and open source.
  2. encouraging security best practices related to OpenTelemetry...beyond the OTel Collector.

@svrnm
Copy link
Member

svrnm commented Dec 14, 2023

@svrnm My question is why not have the content in both locations? (This may not be possible with the functionality/architecture of the OTel docs site, but I still ask the question.)

We can certainly do this (somehow), but this often comes with some additional questions: is this a redirect (so people go from /security to /collector), is it just the same content presented at 2 places, what is the point where this list outgrows that (I assume that we eventually will have security best practices across the docs).

To make it short: yes, we can do that, but we might reconsider it eventually.

I agree that when setting up the OTel Collector, information on securing your OTel Collector is very relevant and should exist within the OTel Collector documentation.

In the original vision by establishing this Security section, we're also:

1. trying to highlight to potential OTel adopters that you can be secure and open source.

2. encouraging security best practices related to OpenTelemetry...beyond the OTel Collector.

This seems to be untouched by the decision where this content lives. Even if we have it only in the collector section I would want to have a /security/practices page that gives a general overview and then links out to relevant pages like the collector security pages.

content/en/docs/security/_index.md Outdated Show resolved Hide resolved
When setting up the OpenTelemetry (OTel) Collector, consider implementing
security best practices in both your hosting infrastructure and your OTel
Collector configuration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we explain briefly in the intro what's at stake here? Why should someone care about securing the OpenTelemetry Collector? Is it a painful process? What knowledge is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You ask great questions @theletterf! If you have the answers, I'm happy to put them in this document. I defer to the SMEs to answer these questions otherwise. At the very least, I will file issues if these questions aren't answered or incorporated by the time we merge this PR.


<!--- TODO: SHOULD configure obfuscation/scrubbing of sensitive metadata. How? Give more details and/or link to an existing document -->

Use OpenTelemetry Collector's `redaction` processor to scrub sensitive data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd remove this line or provide an example in a subsection.

Comment on lines +120 to +122
extension are only accessibly locally to the OpenTelemetry Collector. Care
should be taken when configuring these extensions for remote access as sensitive
information may be exposed as a result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extension are only accessibly locally to the OpenTelemetry Collector. Care
should be taken when configuring these extensions for remote access as sensitive
information may be exposed as a result.
extension are only accessibly locally to the OpenTelemetry Collector. Take
care to protect access to sensitive information when configuring these
extensions, as they might expose it accidentally.

Comment on lines +134 to +139
An observer is capable of performing service discovery of endpoints. Other
components of the OpenTelemetry Collector such as receivers MAY subscribe to
these extensions to be notified of endpoints coming or going. Observers MAY
require certain permissions in order to perform service discovery. For example,
the `k8s_observer` requires certain RBAC permissions in Kubernetes, while the
`host_observer` requires the OpenTelemetry Collector to run in privileged mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
An observer is capable of performing service discovery of endpoints. Other
components of the OpenTelemetry Collector such as receivers MAY subscribe to
these extensions to be notified of endpoints coming or going. Observers MAY
require certain permissions in order to perform service discovery. For example,
the `k8s_observer` requires certain RBAC permissions in Kubernetes, while the
`host_observer` requires the OpenTelemetry Collector to run in privileged mode.
An observer is a component that discovers services in endpoints. Other
components of the OpenTelemetry Collector such as receivers can subscribe to
these extensions to be notified of endpoints coming or going.
Observers might require certain permissions in order to discover services.
For example, the `k8s_observer` requires certain RBAC permissions in
Kubernetes, while the `host_observer` observer requires to run the
OpenTelemetry Collector in privileged mode.

@theletterf
Copy link
Member

@mjingle Thanks SO much for starting this. This is necessary. Hope you'll find my suggestions useful.

@svrnm
Copy link
Member

svrnm commented Jan 18, 2024

@mjingle any updates on this issue? Anything we can do to help you proceed?

@svrnm
Copy link
Member

svrnm commented Jan 29, 2024

@mjingle any updates for this PR?

@chalin
Copy link
Contributor

chalin commented Jan 31, 2024

All: I've rebased & resolved conflicts so that we can get a fresher view of this addition in the context of the updated website -- a lot has changed since this was submitted! :)

mjingle and others added 2 commits January 31, 2024 12:38
Co-authored-by: Patrice Chalin <[email protected]>
Co-authored-by: Juraci Paixão Kröhling <[email protected]>
@theletterf
Copy link
Member

@mjingle LMK if you need to discuss the draft or need more direct support — my comments are not meant to be blockers. Would be happy to commit to your fork if you want.

@cartermp
Copy link
Contributor

Closing old PR (and some unfortunate conflicts, too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants