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

[confmap/provider] feat: Add confmap.Provider that supports AES decryption of values #35549

Conversation

shazlehu
Copy link
Contributor

@shazlehu shazlehu commented Oct 2, 2024

Description:

This package provides a confmap.Provider implementation for symmetric AES encryption of credentials (and other sensitive values) in configurations. It relies on the environment variable OTEL_CREDENTIAL_PROVIDER with the value of the AES key, base64 encoded. 16, 24, or 32 byte keys are supported, selecting AES-128, AES-192, or AES-256 respectively.

An AES 32-byte (AES-256) key can be generated using the following command:

openssl rand -base64 32

Configurations can now use placeholders with the following pattern ${credential:<encrypted & base64-encoded value>}. The value will be decrypted using the AES key provided in the environment variable OTEL_CREDENTIAL_PROVIDER

For example:

export OTEL_CREDENTIAL_PROVIDER="GQi+Y8HwOYzs8lAOjHUqB7vXlN8bVU2k0TAKtzwJzac="
password: ${aes:RsEf6cTWrssi8tlssfs1AJs2bRMrVm2Ce5TaWPY=}

will resolve to:

password: '1'

Since AES is a symmetric encryption algorithm, the same key must be used to encrypt and decrypt the values. If the key is exchanged between the collector and a server, it should be done over a secure connection.

When the collector persists its configuration to disk, storing the key in the environment prevents compromising secrets in the configuration. It still presents a vulnerability if the attacker has access to the collector's memory or the environment's configuration, but increases security over plaintext configurations.

Testing:

Unit tests with 93.0% coverage, built agent and configured with ${credential:<encrypted value>} values.

Documentation:

README.md reflecting this PR description.

Issue:

#35550

@shazlehu shazlehu force-pushed the samhazlehurst/bpop-911-add-credential-encryption-provider-to-otel-contrib branch from 23a9b47 to 2a52146 Compare October 14, 2024 16:23
@shazlehu shazlehu marked this pull request as ready for review October 14, 2024 16:31
@shazlehu shazlehu requested a review from a team as a code owner October 14, 2024 16:31
@shazlehu shazlehu force-pushed the samhazlehurst/bpop-911-add-credential-encryption-provider-to-otel-contrib branch from 2a52146 to 406dae2 Compare October 14, 2024 16:31
@shazlehu shazlehu changed the title [confmap/provider] feat: Add credential provider that supports AES decryption of values [confmap/provider] feat: Add confmap.Provider that supports AES decryption of values Oct 14, 2024
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me.

Since this would add a new component, you'll need to add or update some boilerplate files. https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/31123/files appears to be a fairly recent example.

  • metadata.yaml and make generate
  • make goporto
  • Update .github/* files
  • Probably a make gotidy as well

confmap/provider/aesprovider/package_test.go Outdated Show resolved Hide resolved
confmap/provider/aesprovider/provider.go Outdated Show resolved Hide resolved
@shazlehu shazlehu force-pushed the samhazlehurst/bpop-911-add-credential-encryption-provider-to-otel-contrib branch 2 times, most recently from 8458df7 to ff652ed Compare October 16, 2024 14:30
@shazlehu shazlehu requested a review from djaglowski October 17, 2024 13:56
@djaglowski
Copy link
Member

LGTM. Needs another make gotidy

@shazlehu shazlehu force-pushed the samhazlehurst/bpop-911-add-credential-encryption-provider-to-otel-contrib branch from 0fcf07b to 0001bec Compare October 22, 2024 13:46
@djaglowski djaglowski merged commit b0cbb61 into open-telemetry:main Oct 22, 2024
158 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 22, 2024
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…ption of values (open-telemetry#35549)

**Description:**


This package provides a `confmap.Provider` implementation for symmetric
AES encryption of credentials (and other sensitive values) in
configurations. It relies on the environment variable
`OTEL_CREDENTIAL_PROVIDER` with the value of the AES key, base64
encoded. 16, 24, or 32 byte keys are supported, selecting AES-128,
AES-192, or AES-256 respectively.

An AES 32-byte (AES-256) key can be generated using the following
command:

```shell
openssl rand -base64 32
```

Configurations can now use placeholders with the following pattern
`${credential:<encrypted & base64-encoded value>}`. The value will be
decrypted using the AES key provided in the environment variable
`OTEL_CREDENTIAL_PROVIDER`

> For example:
> 
> ```shell
> export
OTEL_CREDENTIAL_PROVIDER="GQi+Y8HwOYzs8lAOjHUqB7vXlN8bVU2k0TAKtzwJzac="
> ```
> 
> ```yaml
> password: ${aes:RsEf6cTWrssi8tlssfs1AJs2bRMrVm2Ce5TaWPY=}
> ```
> 
> will resolve to:
> ```yaml
> password: '1'
> ```

Since AES is a symmetric encryption algorithm, the same key must be used
to encrypt and decrypt the values. If the key is exchanged between the
collector and a server, it should be done over a secure connection.

When the collector persists its configuration to disk, storing the key
in the environment prevents compromising secrets in the configuration.
It still presents a vulnerability if the attacker has access to the
collector's memory or the environment's configuration, but increases
security over plaintext configurations.


**Testing:**

Unit tests with 93.0% coverage, built agent and configured with
`${credential:<encrypted value>}` values.

**Documentation:**

`README.md` reflecting this PR description.

**Issue:**


open-telemetry#35550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants