[stable/concourse] Refactor values and make secrets optional #3254

william-tran · 2018-01-07T16:56:05Z

Addresses #3208 with the original intent of the issue, to eliminate config map as a source of duplication and make secrets in helm values optional. I could take another stab at it using configmap and secrets supplying an envFrom, but I find this approach simpler.

This PR includes the commits in #3203 assuming that'll be merged first. Only the latest commit in this PR needs to be considered.

william-tran · 2018-01-10T18:29:32Z

/assign @viglesiasce @nexeck @mattfarina

k8s-ci-robot · 2018-01-10T18:29:33Z

@william-tran: GitHub didn't allow me to assign the following users: nexeck.

Note that only kubernetes members can be assigned.

In response to this:

/assign @viglesiasce @nexeck @mattfarina

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

viglesiasce · 2018-01-10T18:35:37Z

/assign

mattfarina · 2018-01-12T14:46:52Z

stable/concourse/README.md


-To run Concourse securely you'll need [3 private keys](https://concourse.ci/binaries.html#generating-keys). For your convenience, this chart provides some [default keys](concourse-keys), but it is recommended that you generate your own keys by running:
+For your convenience, this chart provides some default values for secrets, but it is recommended that you generate and manage these secrets outside the Helm chart. To do this, set `secrets.create` to `false`, create files for each secret value, and turn it all into a k8s secret. First, perform the following to create the manditory secret values:


@viglesiasce any idea how we run automated testing on this if mandatory secret values need to be manually created?

It would be nice if you could pass in a secret to use that was created outside the chart. If that didn't happen the chart could generate a secret.

Right now automated testing would only cover the case where secrets.create: true, as in the values defaults, see https://github.com/kubernetes/charts/pull/3254/files#diff-95cc28250f44d26990fee96ad2fbd63dR465 and below (diff not rendered automatically by github)

mattfarina · 2018-01-12T14:49:33Z

stable/concourse/concourse-keys/host_key

-FS4HAoGAGy+56+zvdROjJy9A2Mn/4BvWrsu4RSQILBJ6Hb4TpF46p2fn0rwqyhOj
-Occs+xkdEsI9w5phXzIEeOq2LqvWHDPxtdLpxOrrmx4AftAWdM8S1+OqTpzHihK1
-y1ZOrWfvON+XjWFFAEej/CpQZkNUkTzjTtSC0dnfAveZlasQHdI=
-----END RSA PRIVATE KEY-----


FYI, templates can generate keys like this. See https://masterminds.github.io/sprig/crypto.html

Cool, though I don't see a function that can generate both the public key and private key, or extract a public key given a private key.

viglesiasce · 2018-01-13T00:44:33Z

/ok-to-test

viglesiasce · 2018-01-13T00:57:50Z

Not sure why but in the test run and when running this locally the pods stayed in ImagePullBackOff for minutes. Going to retest in a bit as maybe its just a temporary Docker Hub issue.

william-tran · 2018-01-13T15:51:07Z

/test all

william-tran · 2018-01-13T16:16:31Z

I think that was a blip, reran tests and it was ready in 30 seconds

ilyasotkov · 2018-01-16T09:58:15Z

You should definitely add a notice to README that manually created secrets must NOT have a trailing newline (\n).

Otherwise, running kubectl create secret generic my-release-concourse --from-file=. will lead to secrets and eventually container environmental variables like CONCOURSE_GITHUB_AUTH_CLIENT_ID having a newline at the end when there really shouldn't be one.

Just spend a few hours debugging this and wouldn't wish it upon anyone :)

william-tran · 2018-01-16T18:31:12Z

I'm so sorry @ilyasotkov, I hope f748073 addresses that.

ilyasotkov · 2018-01-17T04:38:13Z

Also noticed, this block in templates/secrets.yaml should have postgres-password, postgresql-user keys too:

  {{ if .Values.postgresql.enabled }}
  postgresql-user: {{ .Values.postgresql.postgresUser | b64enc | quote }}
  {{ else }}
  postgresql-uri: {{ .Values.secrets.postgresql.uri | b64enc | quote }}
  {{ end }}

viglesiasce · 2018-01-25T00:16:12Z

@william-tran do you have a sec to look at the isse that @ilyasotkov found wrt postgres-password and postgres-user

william-tran · 2018-01-26T18:23:49Z

@ilyasotkov I don't think the postgres-password should be in these secrets as it needs to end up in the secrets for the postgres chart dependency as well, see https://github.com/autonomic-ai/charts/blob/ce0404a6a87c738baad46794c3ea630a93288429/stable/concourse/templates/web-deployment.yaml#L49

If you are using the postgres chart dependency, there's no way to keep that password out of your helm values. But now I question whether the postgres username needs to be a secret, or whether that can just go into the values. This was the way it was set up before I added the conditional postgres dependency and kept this structure, with the username in these secrets and the password in the postgres chart's secrets. But the postgres chart doesn't treat the username as a secret: https://github.com/kubernetes/charts/blob/master/stable/postgresql/templates/deployment.yaml#L34

So I'll further simplify this.

william-tran · 2018-01-26T18:37:39Z

@viglesiasce I've addressed @ilyasotkov's concern in the latest commit e8a9a96

ilyasotkov · 2018-01-26T21:13:05Z

@william-tran you are right about postgres-password being handled by the postgres subchart, there seems to be no way to keep it out of release values as of now, though the subchart will generate a password for us by default if we don’t put one in our release values.

Also I think some massive confusion happened on my end there as I recommended you add the postresql-user key which was literally right there 😝

I believe this line in the README confused me

You'll also need to create/copy secret values for postgres, and any other optional values. See templates/secrets.yaml for possible values

as it lead me to believe both postgresql-user and postgres-password should be part of the manually created secret for a concourse release.

Now that you removed postgresql-user from the concourse secret, the README should state that only postgresql-uri is a file we need to create for the secret and only if we don’t enable the postgres subchart.

william-tran · 2018-01-27T23:08:03Z

Thanks for proofreading, now that I'm looking through the README there's lots of table entries that need updating. I'll try to get around to this in the next few days.

madmod

I think there may be some lifecycle issues with how Vault periodic tokens are being used right now.

Also using client certs is independent of other auth backends and can actually have separate pairs used for the TCP connection and identity verification. (Which has valid use cases.)

Some of my suggestions may fall outside of the scope of this PR so please let me know if you would like me to create separate issues. See my other comments for details.

madmod · 2018-01-30T05:19:37Z

stable/concourse/README.md


-    ## initial periodic token issued for concourse
-    ## ref: https://www.vaultproject.io/docs/concepts/tokens.html#periodic-tokens


I think it might be best to leave this comment as it specifies the kind of token needed. For example if you configured it with a non renewable token it would fail after a period of time and if you use (gasp) a root token it would be a security horror.

madmod · 2018-01-30T05:28:52Z

stable/concourse/templates/secrets.yaml

+  {{- end }}
+  {{- if .Values.credentialManager.vault.enabled }}
+  vault-ca-cert: {{ default "" .Values.secrets.credentialManager.vault.caCert | b64enc | quote }}
+  vault-client-token: {{ default "" .Values.secrets.credentialManager.vault.clientToken | b64enc | quote }}


I think it may be incorrect to persist the client token in this way as concourse will need to periodically renew it and redeploying a stale token would cause a failure. (Same for a pod failure or scaling) I haven't verified this so maybe Concourse is smart enough to deal with it and has the latest token stored elsewhere. Vault best practice is to either use an App Role or even better a service account with the Vault Kubernetes auth backend. That has the downside of longer lived credentials, but the advantage of being able to deal with long periods of downtime without requiring manually created tokens. I'm not sure if concourse has a way to save the refreshed tokens but a persistent volume might make more sense.

maybe Concourse is smart enough to deal with it and has the latest token stored elsewhere.

@madmod I'm not a vault user, but from my reading of https://www.vaultproject.io/docs/concepts/tokens.html#periodic-tokens as long as there isn't downtime longer than the token period, the token value provided would still be valid. So maybe the word initial should be dropped from the comment

## initial periodic token issued for concourse

Because this value shouldn't change, unless you have enough downtime that the token expires.

madmod · 2018-01-30T05:31:51Z

stable/concourse/templates/web-deployment.yaml

-                configMapKeyRef:
-                  name: {{ template "concourse.concourse.fullname" . }}
-                  key: vault-auth-backend
+              value: {{ default "" .Values.credentialManager.vault.authBackend | quote }}


Maybe this should be required instead of an empty default to avoid undefined behavior?

in the comments for credentialManager.vault.authBackend:

## vault authentication backend, leave this blank if using an initial periodic token ## currently supported backends: token, approle, cert. ## # authBackend:

Concourse seems to deal with env vars that are set as empty strings for other features, so unless we can demonstrate otherwise, I'd be hesitant to start wrapping everything in if statements. See also https://concourse.ci/creds.html#vault

madmod · 2018-01-30T05:54:09Z

stable/concourse/values.yaml

+      ## provide the client key for authenticating with the [TLS](https://www.vaultproject.io/docs/auth/cert.html) backend
+      ## the value will be written to /concourse-vault/client.key
+      ## make sure to also set authBackend to `cert`
+      # clientKey: |-


Some Vault deployments may have the TCP listener configured to require a client cert and client key alongside any other form of authentication so it is incorrect to make this a choice between the three. It must be client token or app role and/or client cert and client key. Source

It is also possible to use one cert/key pair for the initial connection and another for the auth backend. (Untested but I'm pretty certain based on the documentation.)

An example use case could be having few shared client key pairs granting permission to talk with Vault at all (TCP listener) and a separate client key pair claiming the identity of the client. (TLS auth backend) This would allow for auditing of traffic with TLS decryption but securely prove the identity of the client in a way that could have a chain of trust subject to certificate revocation lists. (A common use case is for short lived ssh certs given to engineers by other engineers. Each engineer is an intermediate CA provided by Vault so provenance of credentials is easy to track.)

Maybe it would be better to treat this like caCert instead by having it be independent from the auth settings?

unguiculus

Whenever possible, please add defaults to values.yaml instead of using the default function.

william-tran · 2018-01-30T19:40:23Z

@unguiculus I'll address the use of default "" in a later commit.

@madmod The documentation for the vault initial periodic token has moved to the bottom of values.yaml as I've separated out values stored as secrets into their own section.

@viglesiasce How can I become at least a reviewer for this chart? I have a close relationship with the Concourse team so I'd probably be a good candidate to help drive adoption via this chart. Would you sponsor my membership?

Provide a way to opt out of managing secrets in helm values. The configmap introduced needless repetition, mapping values directly to environment variables is a simpler approach.

Also fix encryption key values key in secrets, and rename credentialManager.vault.caCert to credentialManager.vault.useCaCert for clarity

Use a helper to produce more informative errors when secret values are required

Add rbac support for secret access from web pods to to team specific namespaces. Upgrade to Concourse 3.9.0

If the worker runs out of disk space, logs will stop tee-ing to /concourse-work-dir/.liveness_probe because it's on the same persistent volume, and the condition won't be detected.

Contributors, reviewers and approvers of PRs to this chart who are also kubernetes github org members.

william-tran · 2018-02-21T20:36:29Z

@unguiculus have I addressed your concerns around the use of default?

The maintainers have OWNER on the top level of the repo already

Changes made to default

viglesiasce · 2018-02-24T00:18:21Z

/lgtm

Thanks @william-tran!

k8s-ci-robot · 2018-02-24T00:18:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: viglesiasce, william-tran

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [viglesiasce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Make secrets optional, eliminate configmap. Provide a way to opt out of managing secrets in helm values. The configmap introduced needless repetition, mapping values directly to environment variables is a simpler approach. * Don't store postgresql user as a secret * Add missing documentation for values introduced in this and previous PRs Also fix encryption key values key in secrets, and rename credentialManager.vault.caCert to credentialManager.vault.useCaCert for clarity * [stable/concourse] bump postgresql dependency to v0.8.11 * Remove redundant default template function calls * Flatten secret values Use a helper to produce more informative errors when secret values are required * Add support for using k8s secrets as a credential manager. Add rbac support for secret access from web pods to to team specific namespaces. Upgrade to Concourse 3.9.0 * Write the liveness probe log to /tmp instead of /concourse-work-dir If the worker runs out of disk space, logs will stop tee-ing to /concourse-work-dir/.liveness_probe because it's on the same persistent volume, and the condition won't be detected. * Add OWNERS file Contributors, reviewers and approvers of PRs to this chart who are also kubernetes github org members. * fix typos * see if btrfs works in e2e test * Stick with naive FS driver since its the most compatible * Fix helm#3407, make docs reflect actual value * Remove core maintainers from local OWNERS The maintainers have OWNER on the top level of the repo already

* Make secrets optional, eliminate configmap. Provide a way to opt out of managing secrets in helm values. The configmap introduced needless repetition, mapping values directly to environment variables is a simpler approach. * Don't store postgresql user as a secret * Add missing documentation for values introduced in this and previous PRs Also fix encryption key values key in secrets, and rename credentialManager.vault.caCert to credentialManager.vault.useCaCert for clarity * [stable/concourse] bump postgresql dependency to v0.8.11 * Remove redundant default template function calls * Flatten secret values Use a helper to produce more informative errors when secret values are required * Add support for using k8s secrets as a credential manager. Add rbac support for secret access from web pods to to team specific namespaces. Upgrade to Concourse 3.9.0 * Write the liveness probe log to /tmp instead of /concourse-work-dir If the worker runs out of disk space, logs will stop tee-ing to /concourse-work-dir/.liveness_probe because it's on the same persistent volume, and the condition won't be detected. * Add OWNERS file Contributors, reviewers and approvers of PRs to this chart who are also kubernetes github org members. * fix typos * see if btrfs works in e2e test * Stick with naive FS driver since its the most compatible * Fix helm#3407, make docs reflect actual value * Remove core maintainers from local OWNERS The maintainers have OWNER on the top level of the repo already Signed-off-by: voron <av@arilot.com>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 7, 2018

william-tran force-pushed the concourse-optional-secrets branch from 4a5bf24 to a8a3cc1 Compare January 7, 2018 17:21

k8s-ci-robot assigned mattfarina and viglesiasce Jan 10, 2018

mattfarina reviewed Jan 12, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 13, 2018

william-tran force-pushed the concourse-optional-secrets branch 2 times, most recently from 013c591 to ce0404a Compare January 16, 2018 20:32

mattfarina removed their assignment Jan 17, 2018

william-tran mentioned this pull request Jan 30, 2018

[stable/concourse] Fix typo in Vault cert paths. #3465

Merged

madmod suggested changes Jan 30, 2018

View reviewed changes

unguiculus previously requested changes Jan 30, 2018

View reviewed changes

Will Tran and others added 12 commits February 21, 2018 15:33

Make secrets optional, eliminate configmap.

986d30d

Provide a way to opt out of managing secrets in helm values. The configmap introduced needless repetition, mapping values directly to environment variables is a simpler approach.

Don't store postgresql user as a secret

35eeef0

Add missing documentation for values introduced in this and previous PRs

1d47368

Also fix encryption key values key in secrets, and rename credentialManager.vault.caCert to credentialManager.vault.useCaCert for clarity

Remove redundant default template function calls

5e2a459

[stable/concourse] bump postgresql dependency to v0.8.11

73f6db1

Flatten secret values

21b1bd6

Use a helper to produce more informative errors when secret values are required

Add support for using k8s secrets as a credential manager.

86649cc

Add rbac support for secret access from web pods to to team specific namespaces. Upgrade to Concourse 3.9.0

Write the liveness probe log to /tmp instead of /concourse-work-dir

0a538d6

If the worker runs out of disk space, logs will stop tee-ing to /concourse-work-dir/.liveness_probe because it's on the same persistent volume, and the condition won't be detected.

Add OWNERS file

aec02ee

Contributors, reviewers and approvers of PRs to this chart who are also kubernetes github org members.

fix typos

02621b1

see if btrfs works in e2e test

eadbfc6

Stick with naive FS driver since its the most compatible

d5e35e3

william-tran force-pushed the concourse-optional-secrets branch from 5eff1f3 to d5e35e3 Compare February 21, 2018 20:35

william-tran mentioned this pull request Feb 21, 2018

[stable/concourse] Documented default for concourse.allowSelfSignedCertificates is incorrect #3407

Closed

Will Tran and others added 2 commits February 21, 2018 15:54

Fix helm#3407, make docs reflect actual value

af74ab7

Remove core maintainers from local OWNERS

75757e1

The maintainers have OWNER on the top level of the repo already

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2018

k8s-ci-robot merged commit b84124f into helm:master Feb 24, 2018

This was referenced Feb 24, 2018

[stable/concourse] Clean up env var mappings using envFrom #3208

Closed

stable/concourse: trunc of service name causes failures when the helm generated name is too long #2928

Closed

fix typo in ./stable/concourse/values.yaml #3689

Closed

william-tran mentioned this pull request Sep 13, 2018

[stable/concourse] Upgrade to Concourse 4 #6951

Closed

william-tran mentioned this pull request Jan 15, 2019

Allow AWS role based authentication to Secrets Manager and Systems Manager #10237

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/concourse] Refactor values and make secrets optional #3254

[stable/concourse] Refactor values and make secrets optional #3254

william-tran commented Jan 7, 2018 •

edited

william-tran commented Jan 10, 2018

k8s-ci-robot commented Jan 10, 2018

viglesiasce commented Jan 10, 2018

mattfarina Jan 12, 2018

william-tran Jan 12, 2018

mattfarina Jan 12, 2018

william-tran Jan 12, 2018

viglesiasce commented Jan 13, 2018

viglesiasce commented Jan 13, 2018

william-tran commented Jan 13, 2018

william-tran commented Jan 13, 2018

ilyasotkov commented Jan 16, 2018 •

edited

william-tran commented Jan 16, 2018

ilyasotkov commented Jan 17, 2018 •

edited

viglesiasce commented Jan 25, 2018

william-tran commented Jan 26, 2018

william-tran commented Jan 26, 2018

ilyasotkov commented Jan 26, 2018 •

edited

william-tran commented Jan 27, 2018

madmod left a comment

madmod Jan 30, 2018

madmod Jan 30, 2018

william-tran Feb 13, 2018 •

edited

madmod Jan 30, 2018

william-tran Feb 13, 2018

madmod Jan 30, 2018

unguiculus left a comment

william-tran commented Jan 30, 2018

william-tran commented Feb 21, 2018

viglesiasce commented Feb 24, 2018

k8s-ci-robot commented Feb 24, 2018


		To run Concourse securely you'll need [3 private keys](https://concourse.ci/binaries.html#generating-keys). For your convenience, this chart provides some [default keys](concourse-keys), but it is recommended that you generate your own keys by running:
		For your convenience, this chart provides some default values for secrets, but it is recommended that you generate and manage these secrets outside the Helm chart. To do this, set `secrets.create` to `false`, create files for each secret value, and turn it all into a k8s secret. First, perform the following to create the manditory secret values:


		## initial periodic token issued for concourse
		## ref: https://www.vaultproject.io/docs/concepts/tokens.html#periodic-tokens

[stable/concourse] Refactor values and make secrets optional #3254

[stable/concourse] Refactor values and make secrets optional #3254

Conversation

william-tran commented Jan 7, 2018 • edited

william-tran commented Jan 10, 2018

k8s-ci-robot commented Jan 10, 2018

viglesiasce commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viglesiasce commented Jan 13, 2018

viglesiasce commented Jan 13, 2018

william-tran commented Jan 13, 2018

william-tran commented Jan 13, 2018

ilyasotkov commented Jan 16, 2018 • edited

william-tran commented Jan 16, 2018

ilyasotkov commented Jan 17, 2018 • edited

viglesiasce commented Jan 25, 2018

william-tran commented Jan 26, 2018

william-tran commented Jan 26, 2018

ilyasotkov commented Jan 26, 2018 • edited

william-tran commented Jan 27, 2018

madmod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

william-tran Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unguiculus left a comment

Choose a reason for hiding this comment

william-tran commented Jan 30, 2018

william-tran commented Feb 21, 2018

viglesiasce commented Feb 24, 2018

k8s-ci-robot commented Feb 24, 2018

william-tran commented Jan 7, 2018 •

edited

ilyasotkov commented Jan 16, 2018 •

edited

ilyasotkov commented Jan 17, 2018 •

edited

ilyasotkov commented Jan 26, 2018 •

edited

william-tran Feb 13, 2018 •

edited