Skip to content

Refactor secrets and auto-generate password + ssh_key#203

Merged
arm4b merged 16 commits intoStackStorm:masterfrom
cognifloyd:refactor-secrets
Jul 6, 2021
Merged

Refactor secrets and auto-generate password + ssh_key#203
arm4b merged 16 commits intoStackStorm:masterfrom
cognifloyd:refactor-secrets

Conversation

@cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Jun 19, 2021

Refactor secrets values to implement #14 and #16. Since these changes overlap, I combined them into one PR.

BREAKING CHANGE:
Move secrets.st2.{username,password,ssh_key,datastore_crypto_key} values into st2.* (implements #14).

Plus, auto-generate password and ssh_key secrets (implements #16), so that the stackstorm-ha installation is a bit more secure by default. On upgrade, use lookup to pull the current secrets from k8s APIs.

Resolves: #14
Resolves: #16
Closes: #62

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Jun 19, 2021
working_directory: ~/stackstorm-ha
docker:
- image: lachlanevenson/k8s-helm:v3.3.4
- image: lachlanevenson/k8s-helm:v3.4.2
Copy link
Member Author

Choose a reason for hiding this comment

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

@cognifloyd
Copy link
Member Author

rebased on master

Comment on lines 26 to 31
# Password, used to login to StackStorm system (default: auto-generated)
{{- if .Release.IsUpgrade }}
password: {{ index (lookup "v1" "Secret" .Release.Namespace $name).data "password" }}
{{ else }}
password: {{ default (randAlphaNum 12) .Values.st2.password | b64enc | quote }}
{{ end }}
Copy link
Member

@arm4b arm4b Jun 27, 2021

Choose a reason for hiding this comment

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

I hadn't chance to explore deeper the new lookup functionality.

How this thing will behave if user needs to override the previous password set? (whether it's auto-generated or custom)

Any edge cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. We might need a flag like resetPassword which would make it skip the lookup. Something like this:

{{- if and .Release.IsUpgrade (not .Values.st2.resetPassword) }}

Wdyt?

Copy link
Member Author

@cognifloyd cognifloyd Jun 27, 2021

Choose a reason for hiding this comment

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

One edge case might be trying to update the password while upgrading from an existing install without setting that flag. I can't think of any other edge cases. You?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

Choose a reason for hiding this comment

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

@arm4b
Copy link
Member

arm4b commented Jun 28, 2021

Thanks! Looks great at a high level.

Based on the CI run (https://app.circleci.com/pipelines/github/StackStorm/stackstorm-ha/1251/workflows/dbc5fbc0-7557-4eb1-987e-a6d480366c3a/jobs/4191), the st2 password is not showed up in the NOTES when doing the initial Helm chart install:

2. Login with the following credentials:
username: st2admin
password:

This is a high-profile feature that'll affect everyone and so a few more cycles of testing won't hurt, especially around the corner cases we mentioned.

@cognifloyd
Copy link
Member Author

Yeah. That's very sad - the lookup works on upgrade but not install.
Also, lookup always returns nil when using helm template to debug because helm is not supposed to contact the cluster during template.
I'll do a bit more research to see if there's a good way to grab the auto-generated value in multiple places. If you come across anything please let me know.

Allow kubernetes/kubectl to reveal the password instead of embedding
it in NOTEs.txt (which is available in `helm get notes <release name>`).

This works for both values provided in values.yaml and anything
that is auto-generated. Helm templates all of the files at once,
before applying the resources, so we cannot lookup in NOTES what
was generated in another file.
@cognifloyd
Copy link
Member Author

Helm templates all of the files at once, before applying the resources, so we cannot lookup in NOTES what was generated in another file. For that matter, during the same helm run, we can't lookup anything generated in one template file from another template file.

Also, NOTES is stored with the other release information. By default helm 3 uses secret storage for that, but also allows using configmap and postgresql storage backends. It makes sense to minimize the number of places that expose the plain secrets, without regard to how the cluster is [mis-]configured. So, I think using kubectl to retrieve the password instead of embedding it directly in the NOTES marginally improves security for the default st2 ha installation.

So, I changed the NOTES to explain how to retrieve the password with kubectl. Wdyt?

@ericreeves
Copy link
Contributor

ericreeves commented Jun 30, 2021

I agree that it's a step forward to not just print the password in the NOTES. Providing the kubectl command to retrieve it is far safer and nearly as simple from a user experience perspective.

Printing passwords at every upgrade when they weren't requested is a bad thing. They could get caught in log files and terminal output all over the place, and this is the admin password!

Great idea for a PR, and I think the implementation looks nice!

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Thanks @cognifloyd!

WIll run a few tests though before merging.
Would appreciate if @ericreeves you could help with more manual testing around possible edge cases.

@arm4b arm4b merged commit 1bee860 into StackStorm:master Jul 6, 2021
@cognifloyd cognifloyd removed the RFR label Jul 10, 2021
@cognifloyd cognifloyd deleted the refactor-secrets branch November 11, 2021 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change feature security size/L PR that changes 100-499 lines. Requires some effort to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Autogenerate secrets if no Helm value provided, remove insecure defaults Rearrange "secrets" in Helm values

3 participants