openobserve / openobserve-helm-chart

Helm charts for OpenObserve
Apache License 2.0
31 stars 29 forks source link

Remove secrets from values.yaml #45

Closed julienkosinski closed 5 months ago

julienkosinski commented 8 months ago

Hello,

Because values.yaml can be committed in a DevOps repository and also because secrets handled securely are encrypted, I believe values.yaml should not contain secret values but only references to Kubernetes secrets by name when needed.

What do you think about it?

Thank you very much.

prabhatsharma commented 8 months ago

I agree with you in principle. Ease of using a single values.yaml file for setting up entire application is unbeatable though. Most people would favor a single values.yaml and would want to get up and running as soon as possible. There are certainly a category of people who would favor security higher than the ease of use.

One of the primary reasons why people like OpenObserve is it's overall simplicity compared to all the alternative solutions.

Any thoughts on how we can satisfy needs of both the user groups?

julienkosinski commented 8 months ago

@prabhatsharma Well, I thought about it more, and, I understand the need for simplicity.

As I was learning how to secure a Kubernetes cluster I discovered that FluxCD can use a values.yaml as a secret so that it can be encrypted.

So, for those like me who need security, consider FluxCD and this solution: https://github.com/fluxcd/flux2/discussions/3136#discussioncomment-3717631 (Maybe ArgoCD can do the same, but I have not explored it.)

Thank you for your time.

joneshf commented 7 months ago

I agree with you in principle. Ease of using a single values.yaml file for setting up entire application is unbeatable though. Most people would favor a single values.yaml and would want to get up and running as soon as possible. There are certainly a category of people who would favor security higher than the ease of use.

One of the primary reasons why people like OpenObserve is it's overall simplicity compared to all the alternative solutions.

Any thoughts on how we can satisfy needs of both the user groups?

As someone trialing OpenObserve, the fact that credentials are expected to be in plain-text has the opposite effect on me. It doesn't make me excited to continue going, or pleased that it's simpler than the alternatives (which it is!). It makes me question what other things that are commonly thought to be best practices are being ignored. Nevermind the fact that the credentials in question are for the root account (assuming you followed the quick start).

The solution, is exactly what you said: satisfy the needs of both groups. It's fairly common for Helm charts to accept both a plain-text value (as it currently is) and a Kubernetes Secret (as @julienkosinski mentioned). The chart then does the work to inject the value correctly, rather than putting the onus on the consumers of the chart to expand their infrastructure to support this workflow.


Taking a step back, if the idea is to make it easier on folks to get started with OpenObserve, then having the exporters not require injecting plain-text credentials in all cases seems like it would meet that idea in a lower-friction way. If someone is self-hosting OpenObserve, they're likely also self-hosting OpenTelemetry, and likely in the same cluster. Requiring credentials for cluster-local traffic of observability data feels like (and is to me) more friction to getting started than copy/pasting base64 encoded root account credentials.

julienkosinski commented 7 months ago

@joneshf I reopen the issue. Supporting both plain text and secret reference now that you say it, just seems the way to go. Generating and referencing needed secrets, embedding in the chart needed/most advised dependencies (it's now done with PostgreSQL if I recall and could be done with OpenTelemetry) would seems, to me, to be the best default behaviour for a frictionless experience.

prabhatsharma commented 7 months ago

The problem with K8s secrets is that, at the end of the day, they are again manifest files—in principle, no different from values.yaml. That means that you are essentially trading one file/problem for another.

A better way to do it is to use sealed secrets or an external secrets manager. Let's discuss 2 examples:

  1. Hashicorp vault—In order to access the secret in the Hashicorp vault, you need to authenticate to the vault, which is again an issue. Where would you keep the secret to authenticate to vault.
  2. AWS secrets manager: This is the only model I think is a good one if used together with AWS IAM roles, which does not require handling one secret to fetch another.

People should treat their manifest files with secrets as SECRETS. For the more paranoid ones, they should use an external secrets manager, preferably one that follows the model of AWS secrets manager.

We certainly need to do our part of putting secrets in k8s secrets, though, for the above to work. We will do it soon.

Keeping multiple files is not the solution, though, as it does not improve anything.

Self hosting opentelemetry is not really the right word since users always need to host the agents in their environment. OpenObserve has a good model where for ingestion it does not use password, but uses ingestion token which does not have permission to do anything more than ingest data. OpenObserve provides copy/paste scripts to make this happen with minimal friction. Not having any authentication will lead to people making mistakes which is not acceptable. Not sure if you have seen the history of Elastcisearch clusters being open to the whole world with all the data they have. ES did not use to have authentication in standard open source release, which caused people to make these mistakes.

In general, increasing security reduces usability and vice versa. Good tooling can help avoid this, but this rule applies in general.

image
julienkosinski commented 7 months ago

Yes. As I understand it, k8s secret objet is the correct object type to use for every secrets. However, indeed, by default, using these objects do not provide any kind of security. It's just in the right box so that you know what is sensitive and what is not.

I do not think at all that secret encryption is within the scope of the helm, just maybe that the designed box should be used for all secrets (but still, I feel fine as it is now that I found good ways to secure my usage).

I agree about your comment regarding athentication @prabhatsharma. Thank you for the very detailed answer.

MathiasPius commented 6 months ago

People should treat their manifest files with secrets as SECRETS.

There's some nuance here. As mentioned by @julienkosinski previously, tools like FluxCD in conjunction with SOPS allows you to either fully or partially encrypt your secrets and safely store them in version control like Git.

The problem with the OpenObserve approach of not having the chart support secretKeyRef in place of a plaintext password which is what every other helm chart I've seen use, is that you have to have both your secret and non-secret data co-mingling in the same file.

By supporting secretKeyRef the actual secret data could be stored in a separate encrypted yaml file containing only the secret data, allowing you to modify everything else in your deployment without having to worry about leaking credentials due to decrypting/encrypting the data.

For what it's worth, updating the helm charts to support secretKeyRef in a completely backwards-compatible way would be extremely easy, and I'd be happy put in the time to do so in a pull request.

prabhatsharma commented 5 months ago

Added option to specify externalSecret https://github.com/openobserve/openobserve-helm-chart/blob/8662c8301cf53ca1de006bda35323d1cd7577b71/charts/openobserve/values.yaml#L74

julienkosinski commented 5 months ago

Thank you very much @prabhatsharma! :D