search

helm / helm-www

The Helm website for docs, blog and project info.
https://helm.sh
MIT License
201 stars 505 forks source link

Document solution to problem of random passwords change on upgrade (from helm/charts) #1259

Open scottrigby opened 6 years ago

scottrigby commented 6 years ago

I was replying in helm/charts#3591 and realized this is a much bigger problem that deserves a charts issue to decide whether we should agree on and document a policy for this across charts.

Can we address this at one of the next charts meetings?

Related:

unguiculus commented 6 years ago

When I became a maintainer I learned it was best practice to auto-generate passwords and have told folks to follow this pattern. I never really questioned this but am not really happy about it either. I appreciate you brought up the discussion.

Auto-generating passwords is kind of nice. It makes it easy to get started and is certainly better than a hard-coded password. But for a real-world installation, I wouldn't rely on this. So, we need at least better documentation.

Thinking about it again, wouldn't it be better not to provide a default password at all? We could use the required function forcing the user to explicitly set a password. For CI, a custom values file could provide a password.

unguiculus commented 6 years ago

cc @kubernetes/charts-maintainers

alexvicegrab commented 6 years ago

This is indeed a problem. Currently I'm overcoming this by specifying a value with --set flag to override sensitive information (e.g. passwords) during the upgrade process.

I tend to save charts on git repositories but without the password. Ideally I neither want to: 1) Cause bloat, by saving one copy of each chart values.yaml for each (e.g.) PostGres chart I install, nor: 2) Cause security issues by saving passwords in Git repositories 3) Cause high complexity (what I currently do) by having to retrieve the passwords from the relevant Helm deployment secrets and resupplying them in the update command.

A way to automate 3 in a K8S/Helm way would go a long way.

alexvicegrab commented 6 years ago

e.g. it could be useful to add an annotation that prevents updating of a secret like this:

"helm.sh/resource-policy": keep

Except this only works for preventing deletion.

geekuillaume commented 6 years ago

I had the same problem. You can create a secret only on install by using a pre-install hook. The secret will be left untouched on upgrades. The only problem is that it's not managed by Helm and so not deleted when you delete the release.

michaelfig commented 6 years ago

I think the issue is not feeding back secrets into a chart upgrade, nor a problem with generating passwords. I think the issue is that if a chart is generating a random password, it should not overwrite an existing secret.

This seems like it should use the following wrapper around the secret definition:

{{- if or .Values.somePassword .Release.IsInstall}}
apiVersion: v1
kind: Secret
...
data:
  {{- if .Values.somePassword }}
  some-password: {{ .Values.somePassword | b64enc | quote }}
  {{- else }}
  some-password: {{ randAlphaNum 10 | b64enc | quote }}
  {{- end }}
{{- end}}

Thoughts?

alexvicegrab commented 6 years ago

@michaelfig I like this, either we explicitly specify the password, or we autogenerate it during an install.

Does this work? (i.e. Have you tried running this to check that it does not entirely delete the secret during an update when not finding the secret template?)

alexvicegrab commented 6 years ago

Tried it myself just now, alas Helm will simply wipe out the secret.

achempion commented 6 years ago

The service is writing password to the secrets. Maybe it will be better to check first for existence and use old if a secret has a value already.

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

alexvicegrab commented 6 years ago

Is there any progress on setting up a way for random secrets to not be reset implicitly during upgrades?

bacongobbler commented 6 years ago

From the dev call, one possible solution to try would be through an install hook that writes out a configmap.

bacongobbler commented 6 years ago

That or checking for the presence of .Release.IsUpgrade.

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

yurrriq commented 6 years ago

Any update?

arianitu commented 6 years ago

This is pretty annoying, especially with things like Jenkins and Grafana, you're constantly looking up the new password on upgrades.

giacomoguiulfo commented 6 years ago

@arianitu Since Grafana is usually a not mission-critical component, a workaround is to add an annotation to the deployment's spec.template that looks like this: checksum/config: {{ include (print $.Template.BasePath "/secret.yaml") . | sha256sum }} to upgrade the Grafana pods every time the secret changes (when the password is re-generated). In my experience, this makes the pod be in sync with the secret most of the time.

pastukhov commented 6 years ago

Hi all! My secrets keep deleting when chart upgrade, even "helm.sh/resource-policy": keep is set. My yaml:

{{ if .Release.IsInstall }}
{{- $graylogPasswordSecret := randAlphaNum 128 }}
{{- $graylogAdminPassword := randAlphaNum 64 }}
---
apiVersion: v1
kind: Secret
metadata:
  annotations:
    "helm.sh/resource-policy": keep
  name: {{ template "graylog.fullname" . }}-secrets
  labels:
    configVersion: v1
    app: {{ template "graylog.name" . }}
    chart: {{ template "graylog.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
type: Opaque
data:
  GRAYLOG_PASSWORD_SECRET: {{ $graylogPasswordSecret | b64enc | quote }}
  GRAYLOG_ROOT_PASSWORD_SHA2: {{ $graylogAdminPassword | sha256sum | b64enc | quote }}
  GRAYLOG_ADMIN_PASSWORD: {{ $graylogAdminPassword | b64enc | quote }}
{{ end }}

Any advice pls!

pastukhov commented 6 years ago

Solve this quiz with pre-install hook that check if secret exist https://gist.github.com/pastukhov/9e894a71e5793cc56a5096a3dc199f5e

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

dtshepherd commented 5 years ago

/remove stale

chrisaige commented 5 years ago

Any progress?

squillace commented 5 years ago

I'm curious, @bacongobbler, whether any of the work in Helm 3 will resolve or at least provide a direct workaround to this. thots?

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

manics commented 5 years ago

This issue is still relevant

desaintmartin commented 5 years ago

There has been some discussion here : https://github.com/helm/charts/pull/10400

hedayat commented 5 years ago

My opinion: The problem is NOT only for randomly generated passwords in helm charts. Even for not random passwords, you need to always pass proper --set params on helm upgrades for passwords, which is very inconvenient and you should always remember the passwords of a specific release. This is also a problem for automatic upgrade tools: they should store passwords somewhere so that they can reuse them for upgrades, while they are already stored by k8s. So, it created redundancy, and also they should secure the passwords they store.

bacongobbler commented 5 years ago

I'm curious, @bacongobbler, whether any of the work in Helm 3 will resolve or at least provide a direct workaround to this. thots?

Hey @squillace, this is just an issue that nobody has documented a working convention in chart development. Helm 3 won't solve this issue necessarily.

Has anyone tried the suggestions made here? If so, does that solve the issue?

michaelfig commented 5 years ago

@bacongobbler I have a working, backward-compatible convention that relies on Helm PR helm/charts#5290

As per the documentation in the PR, you can just do the following to an existing chart, and it will just work, even for people who upgrade an existing installation:

apiVersion: v1
kind: Secret
metadata:
  name: {{ template "helm-random-secret.fullname" . }}
{{- if not .Values.somePassword }}
  annotations:
    "helm.sh/resource-policy": no-upgrade-existing
{{- end }}
  labels:
    app: {{ template "helm-random-secret.name" . }}
    chart: {{ template "helm-random-secret.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
data:
  {{- if .Values.somePassword }}
  some-password: {{ .Values.somePassword | b64enc | quote }}
  {{- else }}
  some-password: {{ randAlphaNum 10 | b64enc | quote }}
  {{- end }}

The semantics are:

# Use a specific password.
# If omitted, will use existing password otherwise generate a new random one.
# somePassword: "my Pas$w0rd"

Thoughts?

bacongobbler commented 5 years ago

Dropped a comment in that PR. Let's continue the discussion on that particular feature over there.

npdeep commented 5 years ago

One approach that has been working for us is setting a default value for postgresqlPassword but hiding that value using the secrets plugin so you don't commit the real password to github.

cgetzen commented 5 years ago

If we got something similar to .Release.Time (maybe .Release.Init) that was set only on install (and not upgrades), I think a lot of use-cases would be satisfied. You would be able to create pseudo-random values based on it that do not change on upgrade. It'd be even better if https://github.com/Masterminds/sprig/blob/master/docs/strings.md#randalphanum-randalpha-randnumeric-and-randascii could take a seed value.

vsliouniaev commented 5 years ago

The install time would have to be secret to safely use a deterministic PRNG for a password, which seems awkward. (it would also be subject to brute force attacks)

How about a Helm feature that generates some entropy and stores it in a secret for each release at first install time, and then deriving passwords from that?

cgetzen commented 5 years ago

Good point. I think there are two cases that I would hope could be addressed together: generating random values for secrets, and generating random values that need to be placed in labels, annotations, etc - such as ID's. Instead of a .Release.Init, would a .Release.RandomSeedVal be appropriate for both of these scenarios?

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

michaelfig commented 5 years ago

Still relevant. Please review Helm PR helm/charts#5290.

danielezer commented 5 years ago

+1

cgetzen commented 5 years ago

I think https://github.com/helm/helm/pull/5290 is a step in the right direction but doesn't cover all cases. I would like to contribute, but think it would be a waste to work off of master. Are things too in-flux right now / if not, what branch should I work off of? /cc @adamreese

michaelfig commented 5 years ago

@cgetzen, if you want to take https://github.com/michaelfig/helm/tree/no-upgrade-existing you are welcome to... I'm not working on it any further. Feel free to adopt it in your own repo, and publish it as a new PR against Helm.

cgetzen commented 5 years ago

Thanks. The changes I'm hoping to incorporate (generating some amount of entropy that can be referenced from the Release) would probably be clobbered by Helm V3. /cc @bacongobbler

naseemkullah commented 5 years ago

put a general note that one should pre generate a secret and call it a day, or do the checksum thing

desaintmartin commented 5 years ago

To copy what has been previously said elsewhere, here is my personal opinion:

Although random password generation is a very nice way to get started, it should be clearly advertised as DEMONSTRATION PURPOSE only and no complex handle should be done on the chart part in order to keep the chart as simple as possible to manage complexity and understandability. Or to be completely disabled. Some software support having an empty password and that may be an even simpler way to get started.

If there is a native kube/helm feature to store/remember key/values, why not, but handling it through black magic chart tricks seems insane from a pure maintenance and complexity perspective and should be avoided.

tl; dr: I would be in favor of dropping all complex work done in charts for handling passwords generated by the chart, in order to keep complexity low.

cerw commented 5 years ago

Can someone please explain to my, what is the offical best way of passing the DB User/pass/hostname to chart that uses stable/postgrsql as dependencies? example GO application, that needs to connect to it. this is basic stuff but I can not find one answer. I know PSQL stores it in Secret but how shoudl I use it in my custom chart?

thanks

agronholm commented 5 years ago

@desaintmartin It has been suggested to generate secrets as a helm install hook in this issue. Is that not working for you?

desaintmartin commented 5 years ago

It's only my opinion, but I don't really see the point of adding complexity to charts just to generate a password (that, if you do production, should anyway be generated beforehand and stored somewhere else, and if you don't, you don't really care).

danielezer commented 5 years ago

@agronholm but that would make the secret not managed by helm, which will make it harder to make sure its deleted when release is deleted

agronholm commented 5 years ago

@danielezer I see – I thought that adding the appropriate labels for Helm would do the trick.

danielezer commented 5 years ago

@agronholm I can't make it work with helm.sh/hook-delete-policy for secrets because the options are:

  1. hook-succeeded that would delete the secret before it can ever be used
  2. hook-failed which won't help as well
  3. before-hook-creation which will keep the secret after deleting a release.
llech commented 5 years ago

If Helm has no concept for managing secrets with generated random passwords, why encourage people to use helm for generating secrets with random passwords?

Is there any reasonable productive usage for functions like randAlphaNum other than generating random passwords?

You can find numerous tutorials where passwords are auto-generated and following them will bring you to dead end sooner or later...

tinyzimmer commented 5 years ago

I just ran into this also. I was hoping for some sort of functionality along the lines of the ansible ssm parameter store module where you can set overwrite_value: never. That way as long as the parameter exists, if you invoke with a new randomly generated value, it will not be replaced.

I wonder if similar logic could be applied to secrets, but I don't know if that's a k8s scoped thing or a helm scoped thing.