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

[all][meta] Make existing sensitive configuration fields opaque #17273

Closed
40 of 45 tasks
Tracked by #6851
mx-psi opened this issue Dec 27, 2022 · 18 comments
Closed
40 of 45 tasks
Tracked by #6851

[all][meta] Make existing sensitive configuration fields opaque #17273

mx-psi opened this issue Dec 27, 2022 · 18 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mx-psi
Copy link
Member

mx-psi commented Dec 27, 2022

Overview

As part of open-telemetry/opentelemetry-collector#6851 and with the goal of creating a system to query the Collector's configuration, the configopaque.String type alias has been added to the core Collector library to be used on fields that contain sensitive information.

To ensure that no sensitive information is leaked on existing components' configuration, we need to audit their configuration schema and change the type on sensitive fields to use configopaque.String.

This issue intends to list all components where such a change is needed.

To generate the initial list, I searched for instances of "Token", "Key", "Password" and "Secret" on any file named config.go on this repository.

How to make the change

Changing a field type is a breaking change and should be noted as such on the changelog. Codeowners of a given component can choose to make this change with or without a deprecation, depending on how many users a component has as a Go module:

My expectation is that for most fields we can do this without deprecation since usage of the Go API is minimal/nonexistent.

List of subtasks

Receivers

  • [receiver/aerospike] Use configopaque for password field
  • [receiver/awsfirehose] Change the type of Config.AccessKey to be configopaque.String #23829
  • [receiver/bigip] Use configopaque for password field
  • [receiver/cloudfoundry] Change the type of Config.UAA.Password to be configopaque.String #23832
  • [receiver/couchdb] Use configopaque for password field
  • [receiver/elasticsearch] Use configopaque for password field
  • [receiver/jmx] Use configopaque for password, keystore_password, truststore_password fields
  • [receiver/mongodbatlas] Use configopaque for private_key and secret fields
  • [receiver/mongodb] Use configopaque for password field
  • [receiver/mysql] Use configopaque for password field
  • [receiver/nsxt] Use configopaque for password field
  • [receiver/podman] Use configopaque for ssh_passphrase field
  • [receiver/postgresql] Use configopaque for password field
  • [receiver/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
  • [receiver/rabbitmq] Use configopaque for password field
  • [receiver/redis] Use configopaque for password field
  • [receiver/riak] Use configopaque for password field
  • [receiver/saphana] Use configopaque for password field
  • [receiver/snmp] Use configopaque for auth_password and privacy_password fields
  • [receiver/snowflake] Use configopaque for password field
  • [receiver/solace] Use configopaque for password field
  • [receiver/vcenter] Use configopaque for password field

Processors

Exporters

Extensions

  • [extension/asapauth] Use configopaque for private_key field
  • [extension/basicauth] Use configopaque for client_auth::password field
  • [extension/bearertokenauth] Use configopaque for token field
  • [extension/oauth2clientauth] Use configopaque for client_secret field
@mx-psi mx-psi added enhancement New feature or request help wanted Extra attention is needed labels Dec 27, 2022
@mx-psi

This comment was marked as resolved.

@mx-psi mx-psi changed the title [WIP][all][meta] Make existing sensitive configuration fields opaque [all][meta] Make existing sensitive configuration fields opaque Dec 28, 2022
@mx-psi mx-psi added the good first issue Good for newcomers label Dec 28, 2022
@mx-psi
Copy link
Member Author

mx-psi commented Dec 28, 2022

This issue is big, so if you want to work on it, please pick a few components at a time and list them in a comment so that others can also work on it, thanks!

@dyladan
Copy link
Member

dyladan commented Dec 28, 2022

i'll do the dynatrace one

@atoulme
Copy link
Contributor

atoulme commented Dec 28, 2022

I’ll do the splunkhec and signalfx ones. Humio is deprecated, maybe we can pass on it.

@mx-psi
Copy link
Member Author

mx-psi commented Dec 29, 2022

@atoulme @dyladan I created child issues for those components and assigned them to you. I also marked humio as done since it's deprecated (thanks!)

@dyladan
Copy link
Member

dyladan commented Dec 30, 2022

Looks like dynatrace was already done by @codeboten in 62da158 #17077

@gbbr
Copy link
Member

gbbr commented Jan 2, 2023

Did processor/resourcedetectionprocessor: #17314 / #17315

@gbbr
Copy link
Member

gbbr commented Jan 2, 2023

Did extension/asapauthextension: #17316 / #17317

@gbbr
Copy link
Member

gbbr commented Jan 2, 2023

Did the rest of the extensions here: #17318. I hope it's ok for them to be part of the same PR. It was becoming tedious to do them individually.

gbbr added a commit to gbbr/opentelemetry-collector-contrib that referenced this issue Jan 3, 2023
This change updates sensitive information described in open-telemetry#17273 for all
receivers:

- [x] [receiver/aerospike] Use configopaque for password field
- [x] [receiver/awsfirehose] Use configopaque for access_key field
- [x] [receiver/bigip] Use configopaque for password field
- [x] [receiver/cloudfoundry] Use configopaque for password field
- [x] [receiver/couchdb] Use configopaque for password field
- [x] [receiver/elasticsearch] Use configopaque for password field
- [x] [receiver/jmx] Use configopaque for password, keystore_password, truststore_password fields
- [x] [receiver/mongodbatlas] Use configopaque for private_key and secret fields
- [x] [receiver/mongodb] Use configopaque for password field
- [x] [receiver/mysql] Use configopaque for password field
- [x] [receiver/nsxt] Use configopaque for password field
- [x] [receiver/podman] Use configopaque for ssh_passphrase field
- [x] [receiver/postgresql] Use configopaque for password field
- [x] [receiver/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
- [x] [receiver/rabbitmq] Use configopaque for password field
- [x] [receiver/redis] Use configopaque for password field
- [x] [receiver/riak] Use configopaque for password field
- [x] [receiver/saphana] Use configopaque for password field
- [x] [receiver/snmp] Use configopaque for auth_password and privacy_password fields
- [x] [receiver/snowflake] Use configopaque for password field
- [x] [receiver/solace] Use configopaque for password field
- [x] [receiver/vcenter] Use configopaque for password field
@gbbr
Copy link
Member

gbbr commented Jan 3, 2023

Migrated all receivers in #17353

gbbr added a commit to gbbr/opentelemetry-collector-contrib that referenced this issue Jan 3, 2023
…String

This change migrates the remaining exporters from open-telemetry#17273 to use
configopaque.String:

- [x] [exporter/alibabacloudlogservice] Use configopaque for access_key_secret field
- [x] [exporter/azuredataexplorer] Use configopaque for application_key field
- [x] [exporter/azuremonitor] Use configopaque for instrumentation_key field
- [x] [exporter/coralogix] Use configopaque for private_key field
- [x] [exporter/elasticsearch] Use configopaque for api_key and password fields
- [x] [exporter/influxdb] Use configopaque for token and password fields
- [x] [exporter/instana] Use configopaque for agent_key field
- [x] [exporter/logicmonitor] Use configopaque for apitoken::access_key fields
- [x] [exporter/logzio] Use configopaque for account_token field
- [x] [exporter/mezmo] Use configopaque for ingest_key field
- [x] [exporter/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
- [x] [exporter/sapm] Use configopaque for access_token field
- [x] [exporter/tencentcloudlogservice] Use configopaque for secret_key field
@gbbr
Copy link
Member

gbbr commented Jan 3, 2023

...and the exporters are here: #17354.

I think that should have covered everything now.

gbbr added a commit to gbbr/opentelemetry-collector-contrib that referenced this issue Jan 3, 2023
…String

This change migrates the remaining exporters from open-telemetry#17273 to use
configopaque.String:

- [x] [exporter/alibabacloudlogservice] Use configopaque for access_key_secret field
- [x] [exporter/azuredataexplorer] Use configopaque for application_key field
- [x] [exporter/azuremonitor] Use configopaque for instrumentation_key field
- [x] [exporter/coralogix] Use configopaque for private_key field
- [x] [exporter/elasticsearch] Use configopaque for api_key and password fields
- [x] [exporter/influxdb] Use configopaque for token and password fields
- [x] [exporter/instana] Use configopaque for agent_key field
- [x] [exporter/logicmonitor] Use configopaque for apitoken::access_key fields
- [x] [exporter/logzio] Use configopaque for account_token field
- [x] [exporter/mezmo] Use configopaque for ingest_key field
- [x] [exporter/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
- [x] [exporter/sapm] Use configopaque for access_token field
- [x] [exporter/tencentcloudlogservice] Use configopaque for secret_key field
gbbr added a commit to gbbr/opentelemetry-collector-contrib that referenced this issue Jan 3, 2023
This change updates sensitive information described in open-telemetry#17273 for all
receivers:

- [x] [receiver/aerospike] Use configopaque for password field
- [x] [receiver/awsfirehose] Use configopaque for access_key field
- [x] [receiver/bigip] Use configopaque for password field
- [x] [receiver/cloudfoundry] Use configopaque for password field
- [x] [receiver/couchdb] Use configopaque for password field
- [x] [receiver/elasticsearch] Use configopaque for password field
- [x] [receiver/jmx] Use configopaque for password, keystore_password, truststore_password fields
- [x] [receiver/mongodbatlas] Use configopaque for private_key and secret fields
- [x] [receiver/mongodb] Use configopaque for password field
- [x] [receiver/mysql] Use configopaque for password field
- [x] [receiver/nsxt] Use configopaque for password field
- [x] [receiver/podman] Use configopaque for ssh_passphrase field
- [x] [receiver/postgresql] Use configopaque for password field
- [x] [receiver/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
- [x] [receiver/rabbitmq] Use configopaque for password field
- [x] [receiver/redis] Use configopaque for password field
- [x] [receiver/riak] Use configopaque for password field
- [x] [receiver/saphana] Use configopaque for password field
- [x] [receiver/snmp] Use configopaque for auth_password and privacy_password fields
- [x] [receiver/snowflake] Use configopaque for password field
- [x] [receiver/solace] Use configopaque for password field
- [x] [receiver/vcenter] Use configopaque for password field
gbbr added a commit to gbbr/opentelemetry-collector-contrib that referenced this issue Jan 3, 2023
@MovieStoreGuy
Copy link
Contributor

Hey @mx-psi ,

After reviewing #17354, I am not certain that this would be considered a breaking change considering from the user's perspective nothing has changed.
From a development perspective, I don't think this is a breaking change either considering that it is possible to cast back to the original string with no impact.

Is there anything I am missing that would cause this to be a breaking change?

@mx-psi
Copy link
Member Author

mx-psi commented Jan 10, 2023

Is there anything I am missing that would cause this to be a breaking change?

This will not affect end-users of Collector binaries built using the Collector builder (either official or custom), but there exist programs written before changing the type that will fail to build after changing the type because of a type mismatch. This is a breaking change, and semi-official Go tooling like apidiff would identify it as such.

I agree it is not a breaking change that would cause much pain (it's Go API only, and easy to fix), which is why I argue above that it's fine to do this in one step without deprecation, but (unless this causes significant confusion), I don't see why we should not label it as breaking when it is.

codeboten pushed a commit that referenced this issue Jan 10, 2023
…String (#17354)

This change migrates the remaining exporters from #17273 to use
configopaque.String:

- [x] [exporter/alibabacloudlogservice] Use configopaque for access_key_secret field
- [x] [exporter/azuredataexplorer] Use configopaque for application_key field
- [x] [exporter/azuremonitor] Use configopaque for instrumentation_key field
- [x] [exporter/coralogix] Use configopaque for private_key field
- [x] [exporter/elasticsearch] Use configopaque for api_key and password fields
- [x] [exporter/influxdb] Use configopaque for token and password fields
- [x] [exporter/instana] Use configopaque for agent_key field
- [x] [exporter/logicmonitor] Use configopaque for apitoken::access_key fields
- [x] [exporter/logzio] Use configopaque for account_token field
- [x] [exporter/mezmo] Use configopaque for ingest_key field
- [x] [exporter/pulsar] Use configopaque for auth::Token::Token and auth::athenz::private_key fields
- [x] [exporter/sapm] Use configopaque for access_token field
- [x] [exporter/tencentcloudlogservice] Use configopaque for secret_key field
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…que.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…que.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…opaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…Password}` fields to be of `configopaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…igopaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…igopaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
… to be `configopaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
mackjmr added a commit to mackjmr/opentelemetry-collector-contrib that referenced this issue Jul 10, 2023
…opaque.String`

**Description:**
Split out from: open-telemetry#17353

**Link to tracking Issue:** open-telemetry#17273
jpkrohling pushed a commit that referenced this issue Jul 11, 2023
…e `configopaque.String` (#23832)

**Description:**
Split out from: #17353

**Link to tracking Issue:**
#17273
jpkrohling pushed a commit that referenced this issue Jul 12, 2023
…opaque.String` (#24063)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273
fyuan1316 pushed a commit to fyuan1316/opentelemetry-collector-contrib that referenced this issue Jul 13, 2023
codeboten pushed a commit that referenced this issue Jul 18, 2023
…Password}` fields to be of `configopaque.String` (#24064)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273
codeboten pushed a commit that referenced this issue Jul 18, 2023
…igopaque.String` (#24065)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273
codeboten pushed a commit that referenced this issue Jul 18, 2023
…opaque.String` (#24067)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273
mx-psi pushed a commit that referenced this issue Jul 19, 2023
…,TruststorePassword}` to be `configopaque.String` (#23864)

**Description:**
Split out from: #17353

**Link to tracking Issue:**
#17273
dmitryax pushed a commit that referenced this issue Jul 20, 2023
…que.String` (#24062)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273
dmitryax pushed a commit that referenced this issue Jul 20, 2023
…aque.String` (#24060)

**Description:**
Split out from: #17353

**Link to tracking Issue:** #17273

Co-authored-by: Alex Boten <aboten@lightstep.com>
dmitryax pushed a commit that referenced this issue Jul 20, 2023
…onfigopaque.String` (#23829)

**Description:**
Split out from: #17353

**Link to tracking Issue:**
#17273
@mackjmr
Copy link
Member

mackjmr commented Jul 20, 2023

All the receivers have been updated @mx-psi, I think this issue is good to close!

@mx-psi
Copy link
Member Author

mx-psi commented Jul 21, 2023

Thanks @mackjmr!! 🚀 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants