Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

Add support for Vault UpstreamAuthority plugin - K8s Auth #415

Merged
merged 24 commits into from
Sep 15, 2023
Merged

Add support for Vault UpstreamAuthority plugin - K8s Auth #415

merged 24 commits into from
Sep 15, 2023

Conversation

LaithLite
Copy link
Contributor

This PR enables the usage of Vault as an upstream authority with the usage of Kubernetes Authentication.

This relates to #413

@faisal-memon
Copy link
Contributor

@LaithLite Thanks for submitting, this will be a useful feature for a lot of people

@faisal-memon faisal-memon added this to the 0.12.0 milestone Aug 3, 2023
@LaithLite
Copy link
Contributor Author

With regards to ca_cert_path should the helm chart user:

  • specify their own certificates, volume (via spire-server.extraVolumes and volume mount via spire-server.extraVolumeMounts to make this option work

OR

  • Make assumptions and generate templating that creates the volume & volumeMount (which mounts a k8s secret/certificate resource) that then refers to a pre-made secret/cert resource by the helm chart user?

@faisal-memon
Copy link
Contributor

faisal-memon commented Aug 3, 2023

With regards to ca_cert_path should the helm chart user:

  • specify their own certificates, volume (via spire-server.extraVolumes and volume mount via spire-server.extraVolumeMounts to make this option work

OR

  • Make assumptions and generate templating that creates the volume & volumeMount (which mounts a k8s secret/certificate resource) that then refers to a pre-made secret/cert resource by the helm chart user?

I like the 2nd option of those 2. Can also go a step further and pass in the ca cert by a variable and create the secret. We do this with external database password. The first option seems like less work so if you prefer to do that, i think thats fine too.

@kfox1111
Copy link
Contributor

kfox1111 commented Aug 3, 2023

With regards to ca_cert_path should the helm chart user:

  • specify their own certificates, volume (via spire-server.extraVolumes and volume mount via spire-server.extraVolumeMounts to make this option work

OR

  • Make assumptions and generate templating that creates the volume & volumeMount (which mounts a k8s secret/certificate resource) that then refers to a pre-made secret/cert resource by the helm chart user?

I like the 2nd option of those 2. Can also go a step further and pass in the ca cert by a variable and create the secret. We do this with external database password. The first option seems like less work so if you prefer do that, i think thats fine too.

That's messy I think. How do we support the case where the user already has loaded in the cert/secret by some means? (For example: https://external-secrets.io/latest/). We could support both specifying them in values and an external reference I suppose.

@faisal-memon
Copy link
Contributor

That's messy I think. How do we support the case where the user already has loaded in the cert/secret by some means? (For example: https://external-secrets.io/latest/). We could support both specifying them in values and an external reference I suppose.

Valid point @kfox1111.

@faisal-memon
Copy link
Contributor

@LaithLite we discussed on the sync meeting today and agreed on option 2 as the better option. Did you need any further guidance on this?

@marcofranssen
Copy link
Contributor

As an example of implementing this you might follow the same pattern as service accounts do to have a similar API for the end users of the chart.

https://github.com/spiffe/helm-charts/blob/main/charts/spire/charts/spire-server/templates/serviceaccount.yaml

@LaithLite
Copy link
Contributor Author

LaithLite commented Aug 10, 2023

@LaithLite we discussed on the sync meeting today and agreed on option 2 as the better option. Did you need any further guidance on this?

Hi, thanks for the feedback, I think I still need a bit more clarity as I am unsure of the suggested implementation.

With regards to caCertPath, would the underlying volume associated with it be similar to the service account's setup as follows:

volumes:
{{- with .Values.upstreamAuthority.vault.caCertVolume -}}
  - name: upstream-vault-ca
    {{ toYaml  . | nindent 4}}
{{- end }}

So it allows the use of externalSecrets or any other k8s volume resource, although now we provide another helm configuration here.

The alternative could be making it similar to the upstreamAuthority.disk.secret implementation, where you have defaults that mount a secret volume with a default name, otherwise the user can override and generate their own yaml/config similar to the yaml I have defined above.

{{- if eq (.Values.upstreamAuthority.disk.enabled | toString) "true" }}
- name: upstream-ca
secret:
secretName: {{ include "spire-server.upstream-ca-secret" . }}
{{- end }}

{{- with .Values.upstreamAuthority.disk }}
{{- if and (eq (.enabled | toString) "true") (eq (.secret.create | toString) "true") }}
apiVersion: v1
kind: Secret
metadata:
name: {{ include "spire-server.upstream-ca-secret" $root }}
namespace: {{ include "spire-server.namespace" $root }}
labels:
{{- include "spire-server.labels" $root | nindent 4 }}
data:
{{- with .secret.data }}
tls.crt: {{ .certificate | b64enc }}
tls.key: {{ .key | b64enc }}
{{- if ne .bundle ""}}
bundle.crt: {{ .bundle | b64enc }}

@faisal-memon
Copy link
Contributor

Hi @LaithLite The secret option seems a little cleaner to me than exposing a volume. @kfox1111 @marcofranssen Any thoughts on this one?

@kfox1111
Copy link
Contributor

Was looking for a precedent here... We have https://github.com/spiffe/helm-charts/blob/main/charts/spire/charts/spire-server/values.yaml#L425-L430 already in tree.

That supports both secret and configmap, since some engines may put the ca pem in a configmap (safe as they are public keys only)

@faisal-memon faisal-memon modified the milestones: 0.12.0, 0.13.0 Aug 17, 2023
@marcofranssen
Copy link
Contributor

@LaithLite are you planning on moving this forward? Should one of our team pull it forward? Please let us know.

@LaithLite
Copy link
Contributor Author

@LaithLite are you planning on moving this forward? Should one of our team pull it forward? Please let us know.

Apologies I have been busy, I am intending to make this PR more active and will move this forward!

@kfox1111
Copy link
Contributor

Apologies I have been busy, I am intending to make this PR more active and will move this forward!

No worries. We just wanted to make sure you weren't stuck.

@kfox1111
Copy link
Contributor

Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

Thanks for the progress on this one. I have a few inline questions and suggestions. Let me know your thoughts on those.

LaithLite and others added 5 commits September 4, 2023 18:04
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
@kfox1111
Copy link
Contributor

Other then the docs needing to get fixed, it looks good to me. Thanks!

…lt-upstream

Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Signed-off-by: Laith Majeed <laithm98@hotmail.com>
Copy link
Contributor

@kfox1111 kfox1111 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@faisal-memon faisal-memon left a comment

Choose a reason for hiding this comment

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

Thanks @LaithLite for this contribution.

Copy link
Contributor

@marcofranssen marcofranssen left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@marcofranssen marcofranssen merged commit 38f0af4 into spiffe:main Sep 15, 2023
22 checks passed
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
marcofranssen added a commit that referenced this pull request Sep 15, 2023
* 38f0af4 Add support for Vault UpstreamAuthority plugin - K8s Auth (#415)
* 1aac2d4 Bump docker/login-action from 2 to 3
* 1f90867 Allow configuration of priorityClassName on spire-server statefulset (#480)
* 9ad2ed5 option to configure agent sds (#479)
* 693ce08 Remove ## values section from chart readms
* 65d5695 Migrate to readme-generator for helm maintained by bitnami (#431)
* dcc60a2 fix(charts/spire/spire-agent): podmonitor templating (#478)
* 48adb88 Bump actions/checkout from 3.6.0 to 4.0.0
* d1f52d6 Bump sigstore/cosign-installer from 3.1.1 to 3.1.2 (#473)
* 5273f4e Switch mysql and postgresql tests to HA Production configs (#471)
* e81a59a ingress-nginx production tests and spiffe-oidc-discovery-provider example (#136)
* b05175e Bump actions/checkout from 3.5.3 to 3.6.0
* 51cba5b Add customPlugins and unsupportedBuiltInPlugins sections to spire-server (#198)
* f4ee2c2 Bump github.com/onsi/ginkgo/v2 from 2.11.0 to 2.12.0 in /tests (#468)
* c817dd2 support datastore password secret created by external resources (#464)
* 71ac5af Split steps in check-versions wf for easier debugging (#467)
* d91403a Scan for updates to new images (#466)
* 7a5456e Bump helm.sh/helm/v3 from 3.11.3 to 3.12.3 in /tests (#462)
* cbe0001 Federation test (#423)

Signed-off-by: Marco Franssen <marco.franssen@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants