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

Seed secrets (proxy.secretToken, etc) so they don't have to be manually generated #1993

Merged
merged 10 commits into from
Jan 27, 2021

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Jan 16, 2021

Wouldn't it be great if we could automatically seed the passwords like proxy.secretToken, hub.cookieSecret, and hub.config.CryptKeeper.keys, so that the user doesn't have to generate and set them manually?

The reason we haven't before is that we had no mechanism to re-use previous generated passwords, but that changed with Helm 3.1 that introduced the lookup function which we can make use of following #1994.

The strategy in this PR is to...

  1. Use an explicitly provided password (backward compatebility!)
  2. Use an existing password in k8s Secret (helm3's new lookup function!)
  3. Use a new randomly generated password

With this, we close #1910.

Action points

  • Update documentation to reflect we don't have to set proxy.secretToken any more
  • Update schema.yaml if there are such entries
  • CI: Provide some extra --set flags to the upgrade steps were we pass proxy.secretToken explicitly so it won't keep failing until next release.
  • Provide feedback to users in a NOTES.txt note about no longer needing to pass an explicit value for the secrets we now can autogenerate.

Notes for changelog

Note that users should make the upgrade once and then remove what they previously set explicitly in order to avoid a key rotation.

Bonus feature for another PR

  • Support setting "rotate" or similar to generate a new secret.

@consideRatio consideRatio marked this pull request as draft January 16, 2021 15:01
@consideRatio consideRatio force-pushed the pr/seed-secrets branch 6 times, most recently from 776bafd to dbe8403 Compare January 16, 2021 15:30
@consideRatio consideRatio marked this pull request as ready for review January 20, 2021 18:57
@consideRatio consideRatio changed the title Seed secrets so they don't have to be manually generated Seed secrets (proxy.secretToken, etc) so they don't have to be manually generated Jan 20, 2021
@manics
Copy link
Member

manics commented Jan 21, 2021

I like it, I'll test it locally if I have time.

I guess we'll need to update the Z2JH docs too?

@consideRatio consideRatio force-pushed the pr/seed-secrets branch 2 times, most recently from ddf3e1a to abdd456 Compare January 21, 2021 16:42
Copy link
Member Author

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR ready for review / merge!


```
helm upgrade --cleanup-on-fail jhub jupyterhub/jupyterhub -f config.yaml
```
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an open issue to improve the debugging section, so instead of finding an example with diagnosis etc that wasn't correct any more, I opted to delete the section.

<!---
Don't put an example here! People will just copy paste that & that's a
security issue.
-->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were significant differences here, so I ended up updating some leading paragraphs as well.

@@ -372,6 +372,11 @@ def camelCaseify(s):
c.Spawner.debug = True


# load seeded secrets
c.JupyterHub.proxy_auth_token = get_secret_value("JupyterHub.proxy_auth_token")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: the change here is that these secrets are no longer optional in the chart, and cannot possibly be left unspecified or null, is that correct? The previous logic allowed these to be unspecified, in which case the existing default behavior would occur, but explicitly setting them to null or empty values is not the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct that we will now always provide a value to these three configuration options, if you search for "assert hack" you will find an assertion I make about that.

Any falsy value for proxy.secretToken, hub.cookieSecret, hub.config.CryptKeeper.keys would lead to automatic generation of a new passwords.

Is this indirectly influencing the behavior of JupyterHub?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_secret_value("JupyterHub.proxy_auth_token") returns the hub k8s Secret's key's value, which in turn is set by a named template, which in turn has priority logic to:

  1. return any explicitly set value
  2. return the previous k8s Secret's value
  3. return autogenerated value

@yuvipanda
Copy link
Collaborator

This is great!

Resetting / rotating these values is important when you believe they're compromised. Access to proxy secret basically gives you full access to every user's traffic, and access to cookie secret is terrible too. Rotating cookie secret logs everyone out (I think) and that's also very useful.

So as long as these can be set explicitly, I'm <3. But, great that it isn't required!

@consideRatio
Copy link
Member Author

consideRatio commented Jan 25, 2021

Thanks @minrk and @yuvipanda for your feedback!!

Resetting / rotating these values is important when you believe they're compromised.

While it is a bit of a feature creep of this PR, do you have a suggestion on what API to use in order to do this?

Passing -, reset, or rotate as a signal to reset and generate a new secret? A more complicated solution would be to pass reset-<time> or similar which would only reset if it was changed rather than over and over. Doing this, one would not need to first do an upgrade passing - and then a subsequent upgrade passing a blank string again.

@yuvipanda
Copy link
Collaborator

While it is a bit of a feature creep of this PR, do you have a suggestion on what API to use in order to do this?

If these properties are set explicitly, they should take precedence over the autogenerated values. I think that's the best way to do this. So if we are only autogenerating values when the fields are unset, that's good enough :)

@consideRatio
Copy link
Member Author

If these properties are set explicitly, they should take precedence over the autogenerated values. I think that's the best way to do this. So if we are only autogenerating values when the fields are unset, that's good enough :)

Okay yeah then we are good to go - that is what this PR currently does.

@consideRatio
Copy link
Member Author

I added a randHex function and made the generated length 64 hex characters, which aligns with documentation in JupyterHub for cookie secret and how we previously instructed users to generate their passwords.

That was the last itch I had with regards to this PR.

@minrk minrk merged commit 0a2a397 into jupyterhub:master Jan 27, 2021
@minrk
Copy link
Member

minrk commented Jan 27, 2021

Awesome! So great to have this resolved.

consideRatio pushed a commit to jupyterhub/helm-chart that referenced this pull request Jan 27, 2021
@consideRatio
Copy link
Member Author

consideRatio commented Jan 27, 2021

Wieeee thanks for review / merge @minrk @yuvipanda @manics! 🎉 ❤️

minrk added a commit that referenced this pull request Jan 29, 2021
yuvipanda added a commit to yuvipanda/datahub-old-fork that referenced this pull request May 5, 2021
With jupyterhub/zero-to-jupyterhub-k8s#1993,
these are no longer required to be explicitly set. We remove
both just for staging hubs - doing so for production
hubs will cause everyone to be logged out (cookieSecret) and
temporarily disconnected (proxy.secretToken) so we aren't
doing that yet
{{- $result := "" }}
{{- range $i := until 100 }}
{{- if lt (len $result) . }}
{{- $rand_list := randAlphaNum . | splitList "" -}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those curious - randAlphaNum is cryptographically safe too - http://masterminds.github.io/sprig/strings.html.

@yuvipanda
Copy link
Collaborator

Also, you can rotate these secrets with k -n <namespace> delete secret hub and then doing a redeploy. New keys!

yuvipanda added a commit to yuvipanda/datahub-old-fork that referenced this pull request May 5, 2021
Follow-up to berkeley-dsep-infra/datahub#2381.
I verified that this doesn't *actually* rotate the secrets
themeselves - just removes them from git. This just reduces
the amount of secret data we store here.

See jupyterhub/zero-to-jupyterhub-k8s#1993
for more info on what is happening here.
yuvipanda added a commit to yuvipanda/pilot-hubs that referenced this pull request May 11, 2021
Thanks to
jupyterhub/zero-to-jupyterhub-k8s#1993,
proxy.secretToken is automatically generated :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate passwords etc. into a dedicated k8s Secret that can be re-used
4 participants