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

Talk about key rotation downtime and "rotationPolicy: Always" #521

Merged
merged 5 commits into from
Jul 29, 2021

Conversation

maelvls
Copy link
Member

@maelvls maelvls commented Apr 12, 2021

Questions around possible downtime while rotating the private key secret were asked multiple times on the Kubernetes Slack: 1, 2. This PR is about mentioning that there is no expected downtime.

Preview: https://deploy-preview-521--cert-manager.netlify.app/docs/usage/certificate/#rotation-private-key

Related:

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2021
@maelvls maelvls marked this pull request as draft April 12, 2021 16:43
@jetstack-bot jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2021
@maelvls maelvls force-pushed the document-key-rotation branch from 672af3e to c21dd5c Compare April 12, 2021 17:31
@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2021
@maelvls maelvls force-pushed the document-key-rotation branch 5 times, most recently from cb8764f to e47a736 Compare April 12, 2021 17:56
@maelvls
Copy link
Member Author

maelvls commented Apr 12, 2021

I needed to use {id="key-usages"} to avoid breaking people's links anchored to the "Key Usages" section. But our old version of hugo does not support setting custom id anchors. As discussed in #507, upgrading to the latest Hugo version isn't trivial either.

Update: using {#id} anchors instead of {id="id"} seems to have done the trick.

@maelvls maelvls marked this pull request as ready for review April 12, 2021 18:13
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2021
@maelvls
Copy link
Member Author

maelvls commented Apr 12, 2021

/hold

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2021
@maelvls maelvls force-pushed the document-key-rotation branch from 419dd65 to 880e3df Compare April 12, 2021 18:28
@maelvls maelvls changed the title Talk about key rotation downtime Talk about key rotation downtime and "rotationPolicy: Always" May 6, 2021
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @maelvls

I spotted a few typos and made a few suggestions.
Also suggest going through all the changes in the PR and ensuring that references to cert-manager or Kubernetes resource types are capitalized. E.g. Certificate, Secret etc....I didn't highlight them all below.

content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
@maelvls
Copy link
Member Author

maelvls commented May 7, 2021

TODO: add a note that says "changing Never to Always does not immediately trigger a rotation of the private key. done

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2021
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels May 26, 2021
@maelvls maelvls force-pushed the document-key-rotation branch from 62a8bf1 to ea657a9 Compare May 26, 2021 15:53
@maelvls maelvls force-pushed the document-key-rotation branch from ee43f0c to d7b0205 Compare May 26, 2021 19:07
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels May 26, 2021
@maelvls
Copy link
Member Author

maelvls commented Jul 9, 2021

/assign @wallrj

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2021
@maelvls maelvls force-pushed the document-key-rotation branch from ef9e2e6 to d27f858 Compare July 27, 2021 09:08
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2021
maelvls and others added 3 commits July 29, 2021 12:43
Note that I added an anchor ID using the syntax

  ## Header {#id}

That's to prevent anchor ID breakage, i.e., people have already copied
this anchor ID and expect the ID to still work.

Signed-off-by: Maël Valais <mael@vls.dev>
Two bits of information have also been added as part of this change: we
now explain that the app is expected to auto-reload the cert, and we
also added a word on the Venafi issuer not allowing reusing private keys
depending on the configuration.

Signed-off-by: Maël Valais <mael@vls.dev>
Co-authored-by: Richard Wall <richard.wall@jetstack.io>
@maelvls maelvls force-pushed the document-key-rotation branch from d27f858 to 550fc84 Compare July 29, 2021 10:44
@JoshVanL
Copy link
Contributor

This good to me, reads really well :)

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

I spotted a few more typos.

content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
Comment on lines 191 to 194
Setting the `rotationPolicy: Always` or change the Certificate's spec won't
rotate the private key immediately. In order to get the private key secret
rotated, the certificate objects must be reissued. A certificate object is
reissued with either:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Setting the `rotationPolicy: Always` or change the Certificate's spec won't
rotate the private key immediately. In order to get the private key secret
rotated, the certificate objects must be reissued. A certificate object is
reissued with either:
Setting the `rotationPolicy: Always` or changing other fields in the Certificate spec won't
rotate the private key immediately. In order to rotate the private key, the certificate objects must be reissued. A certificate object is
reissued under the following circumstances:

Copy link
Member

Choose a reason for hiding this comment

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

It's not showing the diff for some reason. s/or change the/or changing the/.

And maybe it would read better as:

Changing the Certificate spec does not trigger the renewal process, so if you change the value to rotationPolicy: Always and you want the private key to be rotated immediately, you will have to manually trigger the renewal by:

Copy link
Member

Choose a reason for hiding this comment

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

Also:

Changing the Certificate spec does not trigger the renewal process,

This statement in isolation is not true, and may cause confusion. The majority of fields will cause a re-issuance (e.g. changing dnsNames, commonName, usages etc). Changing the rotationPolicy (or any other fields that control how a renewal happens) won't cause a re-issuance immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

In #521 (comment), Richard proposed to remove the line:

The certificate object is reissued when a change to the certificate object's spec is made.

I think I misunderstood Richard's suggestion back in May. I should have understood Richard's comment as:

The certificate object is reissued when a change to one of the certificate object's fields commonName, dnsNames, ipAddresses, uris, emailAddresses, subject, isCA, usages, duration or issuerRef is made.

(as per RequestMatchesSpec)

Apologies for my misunderstanding.

I re-added the line about reissuing on spec change in ea1f25b.

content/en/docs/usage/certificate.md Outdated Show resolved Hide resolved
Comment on lines 241 to 243
👉 It is recommended you configure `rotationPolicy: Always` on your certificate
resources. It is considered to be a good practice to rotate private keys when a
certificate is renewed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
👉 It is recommended you configure `rotationPolicy: Always` on your certificate
resources. It is considered to be a good practice to rotate private keys when a
certificate is renewed.
👉 We recommend that you configure `rotationPolicy: Always` on your certificate
resources. It is considered to be a good practice to rotate private keys when a
certificate is renewed.

Can we link to a reference backing up this claim good practice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot find any good reference to support this claim. By reading some discussions:

People seem to suggest that:

  1. RSA/ECDSA private keys can't (mostly) be guessed, which means there is less of a need for renewing them (unlike passwords that are way shorter),
  2. But at the same time, an RSA/ECDSA private keys may be leaked through some old backup, leading to a possible man-in-the-middle situation.

Looking around, I found that:

  • certbot renews the private key while renewing the cert by default, but there is nothing explaining why;

  • NIST's Guidelines for the Selection, Configuration, and Use of Transport Layer Security (TLS) Implementations does not indicate when to renew the private key; the only bit that talks about what to do with a server private key is this:

    The security of the server’s private key is critical to the security of TLS. If the server’s private key is weak or can be obtained by a third party, the third party can masquerade as the server to
    all clients.

  • NIST's Recommendation for Key Management has a nice table but no claim on when the private key should be renewweed:
    image

  • f5-bigip suggests to renew the private key (https://support.f5.com/csp/article/K14620):

    Note: When renewing an SSL certificate from a CA, F5 recommends that you generate a new certificate signing request (CSR) and private key. Although some CAs allow you to renew a certificate by using the existing CSR on file, this method is less secure as it retains the existing private key. To generate a new CSR, follow the procedure for Creating an SSL CSR and private key.

@SgtCoDFish Do you know a place where that would be written? Or should I rather remove this claim and say that "the cert-manager team suggests users renewing the private key along with the certificate"?

Copy link
Member

Choose a reason for hiding this comment

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

I think one big reason why you would want to, is that if you were to renew a certificate using the same private key, and then after the initial certificates expiry learn that the private key had actually been leaked, the new certificate would be valid to use with the old private key, thus the 'rotation' didn't really provide any more protection and really just extended the lifetime of the existing certificate.

Copy link
Member

Choose a reason for hiding this comment

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

James is spot on. I don't have like an authoritative source to hand for why you'd want to rotate as much as possible, but certainly I think it's best practice.

The longer a key is used, the more likely it is to be exposed. Keys should be renewed "as often as possible" for me; not only for security reasons but also operational reasons to show that it can be done, so that if it has to be done in an emergency the org has confidence that they can carry it out.

Copy link
Member Author

@maelvls maelvls Jul 29, 2021

Choose a reason for hiding this comment

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

I tried my best to formulate a paragraph that "justifies" our point of view. Please correct me if any of this is wrong. 🔥

👉 We recommend that you configure rotationPolicy: Always on your Certificate resources.

Rotating both the certificate and the private key simultaneously prevents the risk of issuing a certificate associated with an exposed private key.

Another benefit to renewing the private key regularly is to let you be confident that the private key rotation can be done in case of an emergency.

More generally, it is a good practice to be rotating the keys as often as possible, reducing the risk associated with compromised keys.

@JoshVanL
Copy link
Contributor

/hold
Since there are more review comments

Signed-off-by: Maël Valais <mael@vls.dev>
Co-authored-by: Richard Wall <richard.wall@jetstack.io>
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls, wallrj

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:

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

Signed-off-by: Maël Valais <mael@vls.dev>
@maelvls maelvls force-pushed the document-key-rotation branch from d887ba8 to ea1f25b Compare July 29, 2021 15:56
@jetstack-bot jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@SgtCoDFish
Copy link
Member

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@maelvls
Copy link
Member Author

maelvls commented Jul 29, 2021

/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 29, 2021
@jetstack-bot jetstack-bot merged commit e66746c into cert-manager:master Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants