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

New component: confmap.Provider for credential encryption #35550

Open
3 tasks
shazlehu opened this issue Oct 2, 2024 · 14 comments
Open
3 tasks

New component: confmap.Provider for credential encryption #35550

shazlehu opened this issue Oct 2, 2024 · 14 comments
Labels
Sponsor Needed New component seeking sponsor

Comments

@shazlehu
Copy link
Contributor

shazlehu commented Oct 2, 2024

The purpose and use-cases of the new component

Many receivers and exporters require credentials that deployers want to keep confidential, particularly when collectors are deployed to systems where users have elevated access. When the collector persists its configuration to disk, storing credentials in plain text is a potential vulnerability. This provider will allow configurations to use AES-encrypted values by decrypting configuration values using a key stored as an environment variable. This 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.

Example configuration for the component

${credential:<value>} values must be replaced with valid, AES encrypted & base64-encoded values.

receivers:
    plugin/pgp:
        parameters:
            postgresql_log_path:
                - /var/log/postgresql/postgresql*.log
                - /var/lib/pgsql/data/log/postgresql*.log
                - /var/lib/pgsql/*/data/log/postgresql*.log
            start_at: end
        path: ${OIQ_OTEL_COLLECTOR_HOME}/plugins/postgresql_logs.yaml
    postgresql/pg:
        collection_interval: 1m0s
        endpoint: localhost:5432
        metrics: null
        password: ${credential:RsEf6cTWrssi8tlsqMeg3SDhDBlGCHiJFC7bUwl7w/P4uths/mA9}
        tls:
            insecure: true
        transport: tcp
        username: sam
processors:
    resourcedetection/pg:
        detectors:
            - system
        system:
            hostname_sources:
                - os
    transform/google:
        error_mode: ignore
        metric_statements:
            - context: resource
              statements:
                - set(attributes["cloud.region"], "us-east1") where (attributes["cloud.region"] == nil) and (attributes["cloud.availability_zone"] == nil)
exporters:
    googlecloud/google:
        credentials: ${credential:RsEf6cTWrssi8tlsqMeg3SDhDBlGCHiJFC7bUwl7w/P4uths/mA9}
        log:
            resource_filters:
                - regex: .*
        metric: null
        project: fake-project-id
        sending_queue:
            enabled: false
        timeout: 5s
service:
    pipelines:
        logs/pg__google-0:
            receivers:
                - plugin/pgp
            processors:
                - resourcedetection/pg
                - transform/google
            exporters:
                - googlecloud/google
    telemetry:
        metrics:
            address: localhost:8888

Telemetry data types supported

All

Is this a vendor-specific component?

  • This is a vendor-specific component
  • If this is a vendor-specific component, I am a member of the OpenTelemetry organization.
  • If this is a vendor-specific component, I am proposing to contribute and support it as a representative of the vendor.

Code Owner(s)

No response

Sponsor (optional)

No response

Additional context

No response

@shazlehu shazlehu added needs triage New item requiring triage Sponsor Needed New component seeking sponsor labels Oct 2, 2024
@mx-psi
Copy link
Member

mx-psi commented Oct 4, 2024

@shazlehu If you are passing the key through an environment variable, what is the security benefit of doing this instead of just using the env provider?

@shazlehu
Copy link
Contributor Author

shazlehu commented Oct 9, 2024

@mx-psi While I agree the security advantage over the env provider is relatively small, it's not entirely negligible. With this provider, there’s an added layer of security, as it requires discovery of the environment, config, and proper decoding & decryption, whereas the env provider would only need discovery of the environment (and potentially the config).

However, the real benefit this provider adds is in dealing with large, dynamic configs, such as those managed through OpAMP.

With the env provider, if we had ten secrets, we'd need to manage and reference ten environment variables. Adding additional secrets to a configuration would require new environment variables. Also, if the credentials were modified on the OpAMP server, the values of the environment variables would need to be modified.

With this proposed provider, collectors could be deployed with the two proposed variables (key, key type) and changing a collector's configuration wouldn't require a change to the environment or a redeploy of the collector.

@mx-psi
Copy link
Member

mx-psi commented Oct 9, 2024

cc @evan-bradley @atoulme @tigrannajaryan @BinaryFissionGames any thoughts on the OpAMP use case?

@tigrannajaryan
Copy link
Member

When the collector persists its configuration to disk, storing credentials in plain text is a potential vulnerability.

Does this refer to Supervisor's persistence of config to disk? If I remember correctly we recently added the ability to redact config settings when OpAMP Supervisor persists the effective config.

Or is this about the user putting those credentials manually in a config file and supplying the config via --config? If OpAMP is used this should not be the case, the config comes from the server.

So, I am a bit confused about which particular scenario this refers to and what kind of attack exactly the proposed solution prevents.

@BinaryFissionGames
Copy link
Contributor

Does this refer to Supervisor's persistence of config to disk? If I remember correctly we recently added the ability to redact config settings when OpAMP Supervisor persists the effective config.

The config reported in the EffectiveConfig message should be redacted properly, but I think this issue is more about the config that's persisted to disk for the collector to read. It necessarily needs the secrets in the config for the collector to be able to read them.

If that config file were written with the wrong permissions, or a user had configured their groups wrong (once we implement run_as functionality, the config file will need to be readable by the user the collector is run as), the secrets could be read in the plaintext config file.

So, it seems like the concern is having those secrets written to a file. If that's the issue, the only way to solve it seems to be some encryption scheme. We need to persist those secrets in some format because the collector will need them to run.

There could be other ways to accomplish this goal (e.g. persisted RemoteConfig message is encrypted on-disk by supervisor, collector configuration is passed directly as yaml to the command line so the final config never persisted to disk).

@tigrannajaryan
Copy link
Member

I think this issue is more about the config that's persisted to disk for the collector to read. It necessarily needs the secrets in the config for the collector to be able to read them.

Ah, yes, that file. Makes sense.

For completeness, just a reminder that OpAMP has built-in support for connection settings to be provided via OpAMP protocol instead of being stored in the config. It is intended to be used for exporters, although we may be able to stretch it to use for receivers too.

However, the plan was that these credentials will be written to the config file by the Supervisor, so we are back to the same problem when the config file is persisted. If we could make opampextension aware of OtherConnectionSettings and be able to perform the setting substitution in the config than we could avoid writing the plaintext settings in the config file by the Supervisor. I am not sure it is easily doable. Can we even make opampextesnion a config provider so that it can modify the config? I don't think there is a way with the current Collector architecture.

@evan-bradley
Copy link
Contributor

Can we even make opampextesnion a config provider so that it can modify the config? I don't think there is a way with the current Collector architecture.

We could make an OpAMP confmap provider that would allow sending the configuration to the Collector through a socket, which would let us avoid writing any configuration to the disk.

@tigrannajaryan
Copy link
Member

Can we even make opampextesnion a config provider so that it can modify the config? I don't think there is a way with the current Collector architecture.

We could make an OpAMP confmap provider that would allow sending the configuration to the Collector through a socket, which would let us avoid writing any configuration to the disk.

Right. I think, we would need to combine it with opampextension since config providers don't have access to what extension have (e.g. NotifyConfig).

@evan-bradley
Copy link
Contributor

Yeah, they would have to implement complementary OpAMP capabilities. I think the provider would only implement AcceptsRemoteConfig, and everything else would be left to the extension.

Unfortunately any communication between the two would require some kind of global state that exists outside of the Collector framework, for example if we wanted to report remote config status in the extension after receiving it in the confmap provider. It would also be challenging to share a single connection between the two.

@atoulme atoulme removed the needs triage New item requiring triage label Oct 12, 2024
@djaglowski
Copy link
Member

I think the idea of using OpAMP to retrieve credentials at runtime is interesting but my understanding is that this presents a number of challenges. Confmap providers work on the config before there is any semantic understanding of it, and before any extensions can be created. I'm not opposed to the idea but unless there is a clear path forward, can this be a separate issue?


However, the real benefit this provider adds is in dealing with large, dynamic configs, such as those managed through OpAMP.

With the env provider, if we had ten secrets, we'd need to manage and reference ten environment variables. Adding additional secrets to a configuration would require new environment variables. Also, if the credentials were modified on the OpAMP server, the values of the environment variables would need to be modified.

This is the key benefit of what was originally proposed. I don't see it as a meaningful increase in security over the env provider, but it would provide equivalent security while also solving the immediate practical challenges noted by @shazlehu.

Are there any objections to moving this forward solely as a confmap provider? If not, I'm willing to sponsor the provider.

@mx-psi
Copy link
Member

mx-psi commented Oct 14, 2024

I have no objections to this, it seems to solve real practical issues even if security is not improved. One thing that I do wonder is how we plan to support different encryption schemes.

For a start, we should support a single scheme and key size, but we need to think about how to support more as time goes by and schemes get deprecated/people have different use cases. Concretely: do we just do this? Do we want a different provider per encryption scheme (so e.g. here instead of credential we name it aes128)?

@djaglowski
Copy link
Member

Do we want a different provider per encryption scheme (so e.g. here instead of credential we name it aes128)?

I'm in favor of this. At least some other encryption schemes are likely to require different inputs. We might as well just be specific for now. It also simplifies the minimum config for this one, since we'd only need one environment variable.

@shazlehu
Copy link
Contributor Author

Sounds like a good path forward. I'll revise the PR to be AES-specific. Thanks for the helpful input!

@djaglowski
Copy link
Member

If there are no objections before Wednesday, I'll mark this as an Accepted Component

djaglowski pushed a commit that referenced this issue Oct 22, 2024
…ption of values (#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:**


#35550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsor Needed New component seeking sponsor
Projects
None yet
Development

No branches or pull requests

7 participants