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.

Sign up for GitHub

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

Jump to bottom

chart: generate a CA cert with certmanager for hubble tls generation #24505

Closed
wants to merge 2 commits into from
Closed

chart: generate a CA cert with certmanager for hubble tls generation #24505

wants to merge 2 commits into from

Conversation

vflaux
Copy link

@vflaux vflaux commented Mar 21, 2023 •
edited

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

When hubble tls auto method is set to certmanager with no issuer ref, generate the CA certificate using a self-signed issuer and use this CA for the hubble issuer.

Previous behavior was to generate the CA certificate using helm. It was confusing with the method set to certmanager as we could expect for all cert to be created with certmanager.
The generated secret containing the CA by helm was also missing the "tls.crt" key.

Fixes: #24500

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 21, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 21, 2023
@vflaux vflaux changed the title chart: fix hubble tls generation with certmanager chart: generate a CA cert with certmanager for hubble tls generation Mar 22, 2023
@vflaux vflaux force-pushed the fix_chart_tls_certmanager branch 4 times, most recently from 4115708 to 35a1725 Compare March 27, 2023 14:24
@vflaux
chart: fix hubble tls generation with certmanager
65404d2 
When hubble tls auto method is set to certmanager with no issuer ref,
generate the CA certificate using a self-signed issuer and use this CA
for the hubble issuer.
Add missing server auth usage for hubble server certificate and client
auth usage for hubble-relay.

Previous behavior was to generate the CA certificate using helm which
was confusing with the method set to certmanager.

Fixes: cilium#24500

Signed-off-by: Valentin Flaux <valentin_flaux@connect-tech.sncf>
@vflaux vflaux marked this pull request as ready for review March 27, 2023 14:31
@vflaux vflaux requested review from a team as code owners March 27, 2023 14:31
@kaworu kaworu self-requested a review March 28, 2023 09:30
@tklauser tklauser removed their request for review March 28, 2023 09:31
meyskens
meyskens requested changes Mar 28, 2023
View reviewed changes
Copy link
Member

@meyskens meyskens 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 this! Just a few small details to note here :)

install/kubernetes/cilium/values.yaml
@@ -996,6 +996,10 @@ hubble:
# -- certmanager issuer used when hubble.tls.auto.method=certmanager.
# If not specified, a CA issuer will be created.
certManagerIssuerRef: {}
# -- Generated CA certificate validity duration in days for the Issuer.
# Only used if certManagerIssuerRef is not specified.
issuerCertValidityDuration: 26280
Copy link
Member

Choose a reason for hiding this comment

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

This also needs to be set in the values.yaml.tmpl file for our helm chart and doc generating tool to work

Copy link
Member

Choose a reason for hiding this comment

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

more info about that is at https://github.com/cilium/cilium/blob/9be3b287363bbb2c1ef3160aa5e4bb2c236574fa/Documentation/contributing/development/dev_setup.rst#making-changes-to-the-helm-chart (seems to not yet be released to our docs site)

install/kubernetes/cilium/templates/hubble/tls-certmanager/hubble-issuer.yaml
spec:
commonName: Cilium CA
duration: {{ printf "%dh0m0s" (mul .Values.hubble.tls.auto.issuerCertValidityDuration 24) }}
isCA: true
Copy link
Member

Choose a reason for hiding this comment

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

This will generate an RSA key by default, within Cilium we are already using ECDSA.

cilium/examples/kubernetes/servicemesh/ca-issuer.yaml

Line 17 in e63d79b

algorithm: ECDSA

We might consider doing the same here

install/kubernetes/cilium/templates/hubble/tls-certmanager/hubble-issuer.yaml Outdated
ca.crt: {{ .ca.Cert | b64enc }}
ca.key: {{ .ca.Key | b64enc }}
spec:
commonName: Cilium CA
Copy link
Member

Choose a reason for hiding this comment

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

We might want to add Hubble here too as Cilium uses certificates for cluster mesh, service mesh, network policy... if a user sees a certificate signed by this it helps to day that it is Cilium Hubble

@maintainer-s-little-helper
Copy link

Commit 5587e8efacfb99cc977c2494be05f7da443bf180 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 28, 2023
@vflaux
fixup! chart: fix hubble tls generation with certmanager
63d9843 
Signed-off-by: Valentin Flaux <valentin_flaux@connect-tech.sncf>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 28, 2023
youngnick
youngnick approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Looks like @meyskens' requested changes are done, LGTM now.

@giorio94 giorio94 mentioned this pull request Mar 31, 2023
Merged
@giorio94
Copy link
Member

giorio94 commented Apr 6, 2023

Hey @vflaux, thanks for your contribution!

This is an heads up that we are merging #24666, which addresses #24500 in a different way. Essentially, that PR makes it mandatory to specify the issuer configuration when using cert-manager. That targets both hubble and clustermesh related configurations, and encourages the best practice of using the same CA for all components. I personally believe it also reduces the overall confusing during operations, explicitly demanding users to select which issuer type to use, since Cilium should not be concerned with the generation of the CA as much as possible. And users can just create a self-signed issuer with whatever deployment method they are using (e.g., flux2) if they are not interested in establishing a clustermesh.

Given the above, I'm closing this PR as superseded. Feel free to comment or open an issue and tag me if you feel something is still missing in this regard.

@giorio94 giorio94 closed this Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meyskens meyskens meyskens requested changes

@youngnick youngnick youngnick approved these changes

@sayboras sayboras Awaiting requested review from sayboras sayboras was automatically assigned from cilium/helm

@kaworu kaworu Awaiting requested review from kaworu

Assignees
No one assigned
Labels
dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. kind/community-contribution This was a contribution made by a community member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chart: hubble tls certificate generation with certmanager fails
4 participants
@vflaux @giorio94 @meyskens @youngnick