open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.01k stars 2.33k forks source link

New component: confmap.Provider for credential encryption #35550

Open shazlehu opened 2 weeks ago

shazlehu commented 2 weeks ago

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?

Code Owner(s)

No response

Sponsor (optional)

No response

Additional context

No response

mx-psi commented 2 weeks ago

@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 commented 1 week ago

@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 commented 1 week ago

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

tigrannajaryan commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.

djaglowski commented 4 days ago

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 commented 4 days ago

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 commented 4 days ago

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 commented 4 days ago

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

djaglowski commented 4 days ago

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