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

Add support to TLSSetting to not only read from file path, but from memory #7313

Closed
Cainuse opened this issue Mar 4, 2023 · 4 comments
Closed

Comments

@Cainuse
Copy link

Cainuse commented Mar 4, 2023

Is your feature request related to a problem? Please describe.
https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/configtls.go#L42

The current TLSSettting struct only reads CA cert, client/server cert and private key from file which is inconvenient in some situations. For instance, when deploying applications in a containerized environment, it may not be feasible to read from a file due to security and operational constraints.

Describe the solution you'd like
A clear and concise description of what you want to happen.
I would like to propose adding the ability to read the TLSSetting struct from memory in the OpenTelemetry library. This would enable applications to securely and conveniently provide the necessary TLS configuration settings without the need for an external file.

This could simply be adding a couple of new fields within the TLSSetting struct like the following

type TLSSetting struct {
	// Path to the CA cert. For a client this verifies the server certificate.
	// For a server this verifies client certificates. If empty uses system root CA.
	// (optional)
	CAFile string \`mapstructure:"ca_file"\`

        // Value of the CA cert(optional)
        // For a server this verifies client certificates. 
        CA       string \`mapstructure:"ca"\`

	// Path to the TLS cert to use for TLS required connections. (optional)
	CertFile string \`mapstructure:"cert_file"\`

        // Value of the CA cert (optional)
        // Value of the TLS cert to use for TLS required connections.
        Cert       string \`mapstructure:"cert"\`

	// Path to the TLS key to use for TLS required connections. (optional)
	KeyFile string \`mapstructure:"key_file"\`

        // Value of the private key (optional)
        // Value of the TLS private key to use for TLS required connections.
        Key       string \`mapstructure:"key"\`

	// MinVersion sets the minimum TLS version that is acceptable.
	// If not set, TLS 1.2 will be used. (optional)
	MinVersion string \`mapstructure:"min_version"\`

	// MaxVersion sets the maximum TLS version that is acceptable.
	// If not set, refer to crypto/tls for defaults. (optional)
	MaxVersion string \`mapstructure:"max_version"\`

	// ReloadInterval specifies the duration after which the certificate will be reloaded
	// If not set, it will never be reloaded (optional)
	ReloadInterval time.Duration \`mapstructure:"reload_interval"\`
}

Alternatives Considered
We could work around this issue by reading from memory first and write to local files and then have TLSSetting pick up the generated files, but this is not a secure approach as if the private key doesn't get cleaned up property it could still be accessible by malicious parties.

@andrzej-stencel
Copy link
Member

andrzej-stencel commented Mar 15, 2023

Do I understand correctly that this would mean I could configure a component that uses TLS (e.g. Prometheus exporter) like this, using environment variables?

exporters:
  prometheus:
    tls:
      cert: ${TLS_CERT}

and pass the certificate content as environment variable TLS_CERT? Sounds like a good idea 👍

@erikbaranowski
Copy link
Contributor

I have created a prototype for this exact use case and would be willing to submit a similar PR if this proposal is accepted to move forward.

Prototype: grafana#2

@atoulme
Copy link
Contributor

atoulme commented Dec 14, 2023

It looks like this was resolved, ok to close?

@mx-psi
Copy link
Member

mx-psi commented Jan 23, 2024

I think we can close this indeed

@mx-psi mx-psi closed this as completed Jan 23, 2024
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

No branches or pull requests

5 participants