Add renew certificates subcommand #147

pytimer · 2019-12-22T07:27:29Z

The etcdadm certs renew support renew certificates with local CA, it will renew the etcd certificates exclude CA.

Now the etcdadm create etcd certificates default expiration time is one year. The cluster not work if time is more than one year. I use kubeadm create Kubernetes cluster and it also have the same question, however kubeadm have renew command to help us make certificates rotation, so i think it is better if etcdadm have this renew command.'

I also found other people have this question, Ref: #56, so i send a PR to add a renew subcommand for implement it.

I'm not sure if missing something, if i missing, please tell me.

k8s-ci-robot · 2019-12-22T07:27:37Z

Hi @pytimer. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

chuckha

I think I'd want a little more documentation about how to actually use this. Where do I run it? On the node with the CA? Do I run it on every node? Can I run it on any node? Do I have to copy certs over to other nodes after I run this?

pytimer · 2020-01-14T16:35:38Z

This command should be run on the certificate expirated node with CA. Before run this command, you should copy CA to the node, and then run renew command, it will renew the etcd certificates on the node.

chuckha · 2020-01-14T17:04:02Z

This command should be run on the certificate expirated node with CA. Before run this command, you should copy CA to the node, and then run renew command, it will renew the etcd certificates on the node.

My questions were mostly to help guide some documentation fore this feature. Perhaps a new section in the README? Or maybe even in the command help?

pytimer · 2020-01-15T10:51:41Z

@chuckha I update the README.md in this pr, you can review it. Thanks.

chuckha

Thanks for adding this. I update a suggestion to potentially clear up what certs are being renewed. I don't think we need to write down how to copy certs around as it's already listed.

Worked for me, but I'm surprised at the lack of cluster level health check/info through etcdadm.

chuckha · 2020-01-15T17:41:17Z

cmd/certs.go

@@ -0,0 +1,46 @@
+package cmd


New file needs license header

I will add license header late.

chuckha · 2020-01-15T19:03:51Z

README.md

@@ -60,6 +60,22 @@ On the machine being removed, run
 etcdadm reset
 ```

+### Renewal certificates


This took me a second to understand. We aren't renewing the CA, we're renewing the certs generated with the CA.

this should be a little more friendly, how about something like this:

Certificates are set to expire after <1 year?> by default.
To renew certificates, first ensure every node in the cluster has the CA root certificate. Then, on each node in, run etcdadm certs renew.

+1 to @chuckha's suggestion.

I think it might help to explicitly mention that there are two certificates: peer and server.

I will update the document.

Thank you @pytimer!

chuckha · 2020-01-15T19:04:59Z

/ok-to-test

dlipovetsky · 2020-01-15T22:48:40Z

certs/certs.go

+// GetDefaultCertList returns all of the certificates etcdadm requires to function.
+func GetDefaultCertList() []string {
+	return []string{
+		//constants.EtcdCACertAndKeyBaseName,


Remove commented code?

OK, I will remove it.

dlipovetsky · 2020-01-15T22:52:05Z

certs/certs.go

+	}
+}
+
+// RenewUsingLocalCA executes certificate renewal using local certificate authorities for generating new certs.


Suggested change

// RenewUsingLocalCA executes certificate renewal using local certificate authorities for generating new certs.

// RenewUsingLocalCA replaces certificates with new certificates signed by the CA.

dlipovetsky · 2020-01-15T22:57:48Z

certs/certs.go

+func GetDefaultCertList() []string {
+	return []string{
+		//constants.EtcdCACertAndKeyBaseName,
+		constants.EtcdServerCertAndKeyBaseName,


How does replacing a certificate affect the client that uses the certificates?

etcd peer and server certificates: etcd will check and, if needed, reload the certificate on every connection.

etcdctl client certificate: this is not a long-running process, and presumably etcdctl reads certificates when it starts. If it fails, the user can always rerun it.

**kube-apiserver: I'm not sure if it will reload its etcd client certificate. Does it need to be restarted? **

Etcd should restart as far as I know.

etcdctl every time use the new certificate s , so I think renew not effect it.

If the kube-apisever client crt changed, I think kube-apisever maybe need to restart. I'm not tests this case, so i'm not sure. I will tests it.

Thank you!

If kube-apiserver does need to be restarted, let's document that. Perhaps even write a message to the user.

dlipovetsky · 2020-01-15T23:26:07Z

certs/certs.go

+	}
+
+	// extract the certificate config
+	cfg := certToConfig(cert)


For etcdadm certs renew this means that the source of truth is the certificate on disk.. However, for etcdadm init and etcdadm join, the source of truth is etcdadm.

I think that the source of truth should be consistent across all commands.

In fact, if etcdadm certs renew uses etcdadm as the source of truth, then it can just create new certificates using the existing functions (CreateEtcdServerCertAndKeyFiles, CreateEtcdPeerCertAndKeyFiles, CreateEtcdctlClientCertAndKeyFiles, and CreateAPIServerEtcdClientCertAndKeyFiles).

WDYT?

You mean that maybe the disk certificates not the source of truth?

I understand your said, But if we use createXXX to recreate the new certificates, is it include CA certificate?

You mean that maybe the disk certificates not the source of truth?

That's right; I believe that the certificates on disk should not be the source of truth.

When etcdadm creates certificates the first time, it determines their properties. When it renews them, it should set those properties the same way. It should not pay attention to the properties in the certificates on disk.

I understand your said, But if we use createXXX to recreate the new certificates, is it include CA certificate?

I believe each of the functions I mentioned reads the CA from disk.

Do not believe the certificates on disk is right, but if we recreate certificates, this command name still call renew, or change to etcdadm certs recreate ?

Great question! I think renew is correct, based on the RFC:

This subcomponent is used to describe the following elements related to certificate renewal. Certificate renewal means the issuance of a new certificate to the subscriber without changing the subscriber or other participant's public key or any other information in the certificate:
-- https://tools.ietf.org/html/rfc3647 *

However, if I'm reading the definition correctly, etcdadm must generate a new certificate using its current key on disk. That means etcdadm needs to read the key from disk, but not the certificate.

(I continue to believe that etcdadm should set the certificate properties, rather than read them from the certificate on disk.)

* I found this via https://security.stackexchange.com/questions/150907/x-509-certificate-renew-vs-rekey

I think this renew implementation more like rekey, but it also use certificate properties. If my understanding correct, you mean that etcdadm not use old certificate properties on the disk?

I agree; this PR right now implements "rekey" according to the RFC.

I don't know whether we want "renew" or "rekey;" I need to learn the trade-offs and use cases.

I decided to check what kubeadm does. Looks like kubeadm does in fact read the properties of the existing certificates:

Note: alpha certs renew uses the existing certificates as the authoritative source for attributes (Common Name, Organization, SAN, etc.) instead of the kubeadm-config ConfigMap. It is strongly recommended to keep them both in sync.
-- https://v1-17.docs.kubernetes.io/docs/tasks/administer-cluster/kubeadm/kubeadm-certs/#renew-certificates-with-the-kubernetes-certificates-api

I wonder why that is.

I see kubeadm read certificate properties, i think maybe etcdadm/kubeadm init or join can add custom SANs, if we don't read properties, maybe we missing some SANs.

i think maybe etcdadm/kubeadm init or join can add custom SANs, if we don't read properties, maybe we missing some SANs.

That's a great point.

dlipovetsky · 2020-02-08T04:39:50Z

Thanks for helping me understand your approach @pytimer . I'm ok with etcdadm reading the properties from the certs on disk.

The only question left is #147 (comment). Otherwise this looks good to merge.

pytimer · 2020-02-08T06:27:05Z

The only question left is #147 (comment). Otherwise this looks good to merge.

Sorry, because of have some things, i have no locally Kubernetes cluster before, i will testing and reply it coming soon.

pytimer · 2020-02-09T04:58:48Z

@dlipovetsky I testing kube-apiserver case about #147 (comment) , kube-apiserver needs to restart and load new etcd client certificates.

I update document and code, warning users to pay attention. The code is: https://github.com/kubernetes-sigs/etcdadm/pull/147/files#diff-1094222d3259d85ad53cc70f8bcc0816R54

dlipovetsky · 2020-02-09T22:03:45Z

cmd/certs.go

+					log.Fatalf("Certificates %s can't be renewed.\n", name)
+				}
+			}
+			log.Println("Your etcd certificates has renew successfully!")


Suggested change

log.Println("Your etcd certificates has renew successfully!")

log.Println("The etcd certificates have been renewed successfully!")

dlipovetsky · 2020-02-09T22:09:43Z

cmd/certs.go

+				}
+			}
+			log.Println("Your etcd certificates has renew successfully!")
+			log.Warnln("After renew etcd certificates, you need to restart client(kube-apiserver) which use apiserver-etcd-client certificate.")


Suggested change

log.Warnln("After renew etcd certificates, you need to restart client(kube-apiserver) which use apiserver-etcd-client certificate.")

log.Warnln("If kube-apiserver is running, restart it so that it uses the renewed etcd client certificate.")

dlipovetsky · 2020-02-09T22:39:56Z

Worked for me, but I'm surprised at the lack of cluster level health check/info through etcdadm.

@chuckha What did you want etcdadm to do?

Also, what do you think of the PR as it is right now?

dlipovetsky · 2020-02-10T01:20:43Z

@dlipovetsky I testing kube-apiserver case about #147 (comment) , kube-apiserver needs to restart and load new etcd client certificates.

I update document and code, warning users to pay attention. The code is: https://github.com/kubernetes-sigs/etcdadm/pull/147/files#diff-1094222d3259d85ad53cc70f8bcc0816R54

I know I said earlier that kube-apisever might need to be restarted to reload its etcd client cert. But is that what you see in the logs? The reason I ask is: it looks like kube-apiserver should reload the certificates automatically. That's because kube-apiserver uses the etcd client's TLS config, which implements certificate reloading.

pytimer · 2020-02-10T05:17:37Z

I know I said earlier that kube-apisever might need to be restarted to reload its etcd client cert. But is that what you see in the logs? The reason I ask is: it looks like kube-apiserver should reload the certificates automatically. That's because kube-apiserver uses the etcd client's TLS config, which implements certificate reloading.

I see kube-apiserver log print error, remote error: tls: bad certificate. I'm not sure kube-apiserver occur this error because of renew certificates, i will continue tests. If someone know, please point out.

dlipovetsky · 2020-02-10T17:58:14Z

I see kube-apiserver log print error, remote error: tls: bad certificate. I'm not sure kube-apiserver occur this error because of renew certificates, i will continue tests. If someone know, please point out.

I'll look this week, too. It's not a blocking question, but it's important to know. Depending on what we find, we may want to open a kube-apiserver issue.

Make PKI library richer, including in-memory keypairs

fejta-bot · 2020-09-09T07:41:24Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-10-09T08:23:56Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

Avatat · 2020-10-09T08:51:05Z

I'm sorry. I will check this feature next week.

fejta-bot · 2020-11-08T09:06:10Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-11-08T09:06:17Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

Make PKI library richer, including in-memory keypairs

ntaylor1781 · 2021-01-27T20:27:04Z

Can we reopen this? This is a needed feature. I think #179 is a great long term goal, but it will be able to utilize this work

dlipovetsky · 2021-02-01T17:42:40Z

/reopen

k8s-ci-robot · 2021-02-01T17:42:47Z

@dlipovetsky: Reopened this PR.

In response to this:

/reopen

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.

k8s-ci-robot · 2021-02-01T17:42:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pytimer
To complete the pull request process, please assign justinsb after the PR has been reviewed.
You can assign the PR to them by writing /assign @justinsb in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

ntaylor1781 · 2021-02-01T17:45:07Z

A comment on kube-apiserver. It will pick up the changes to the ETCD client cert, and automatically use it on new connections.

pytimer · 2021-02-02T01:18:45Z

A comment on kube-apiserver. It will pick up the changes to the ETCD client cert, and automatically use it on new connections.

I use k8s v1.10.0, found kube-apiserver can not automatically use the new client cert, need to restart it, so i add the warning comment when certificates renewed.

ntaylor1781 · 2021-02-02T16:27:15Z

Good point. The functionality was added in the etcd client version 3.2.0, though there was a bug where it did not work properly when addressed with an IP until 3.2.19. K8S version 1.11 and 1.10 should work if addressed with a domain. Version 1.12 and up will always reload.

fejta-bot · 2021-03-04T16:36:29Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

k8s-ci-robot · 2021-03-04T16:36:39Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.

ntaylor1781 · 2021-03-22T15:11:37Z

Can we re-open this and get it reviewed?

Avatat · 2021-05-17T09:21:05Z

Finally, I tried it and the renew feature works good!

The only problem I have is CSR mode:

# etcdadm certs renew --csr-only
INFO[0000] [renew] Renew etcd server certificate.       
FATA[0000] [renew] Error renew certificate server: failed to write new csr server: server CSR existed but it could not be loaded properly: couldn't load the certificate request /etc/etcd/pki/server.csr: failed to read file: open /etc/etcd/pki/server.csr: no such file or directory

But there is no server.csr file:

# ls
apiserver-etcd-client.crt  apiserver-etcd-client.key  ca.crt  ca.key  etcdctl-etcd-client.crt  etcdctl-etcd-client.key	peer.crt  peer.key  server.crt	server.key

If you want to use this feature, clone @pytimer repo, build from cert branch, and use built binary:

git clone -b cert https://github.com/pytimer/etcdadm.git
cd etcdadm
make container-build
./etcdadm certs

sfgroups-k8s · 2021-06-24T12:32:08Z

Hi,
Any plan to include this cert renewal code https://github.com/pytimer/etcdadm/tree/cert to this etcdadm code?

Thanks

This is continuing the work from kubernetes-retired#147. Last concern was due to issues with CSR generation. This was due to trying to write the CSR and key to the certificate directory. When calling TryLoadCSRAndKeyFromDisk it registered the key existed, and tried to load both. This caused the error stating it couldn't load the CSR. As with kubeadm, this needed a directory to output the csr specifically. I added a new argument of csr-dir (with a default of the local directory). I split the RenewUsingLocalCA function into two functions. One for the CSRs, and one for the full certificates. The new RenewCSRUsingLocalCA function uses the new csr-dir path to output the CSRs. This ensures the key, from the cert being used, is not overwritten/cause the process to fail.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 22, 2019

k8s-ci-robot requested review from dlipovetsky and justinsb December 22, 2019 07:27

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 22, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 22, 2019

chuckha reviewed Jan 14, 2020

View reviewed changes

pytimer force-pushed the cert branch from 87bda94 to 1882cf5 Compare January 15, 2020 10:50

chuckha reviewed Jan 15, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 15, 2020

dlipovetsky reviewed Jan 15, 2020

View reviewed changes

pytimer force-pushed the cert branch from 1882cf5 to 219587f Compare January 16, 2020 02:31

pytimer force-pushed the cert branch from 219587f to e17272f Compare February 9, 2020 05:23

dlipovetsky reviewed Feb 9, 2020

View reviewed changes

justinsb added a commit to justinsb/etcdadm that referenced this pull request Aug 28, 2020

Merge pull request kubernetes-retired#147 from justinsb/more_certs

fe0670e

Make PKI library richer, including in-memory keypairs

justinsb added a commit to justinsb/etcdadm that referenced this pull request Aug 29, 2020

Merge pull request kubernetes-retired#147 from justinsb/more_certs

09b3cd3

Make PKI library richer, including in-memory keypairs

justinsb added a commit to justinsb/etcdadm that referenced this pull request Aug 29, 2020

Merge pull request kubernetes-retired#147 from justinsb/more_certs

de2c9fc

Make PKI library richer, including in-memory keypairs

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 9, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 9, 2020

k8s-ci-robot closed this Nov 8, 2020

justinsb added a commit to justinsb/etcdadm that referenced this pull request Dec 4, 2020

Merge pull request kubernetes-retired#147 from justinsb/more_certs

84aaec3

Make PKI library richer, including in-memory keypairs

k8s-ci-robot reopened this Feb 1, 2021

k8s-ci-robot closed this Mar 4, 2021

This was referenced Dec 2, 2021

Cert renewal #272

Closed

Finish cert renewal #273

Closed

	// RenewUsingLocalCA executes certificate renewal using local certificate authorities for generating new certs.
	// RenewUsingLocalCA replaces certificates with new certificates signed by the CA.

	log.Println("Your etcd certificates has renew successfully!")
	log.Println("The etcd certificates have been renewed successfully!")

	log.Warnln("After renew etcd certificates, you need to restart client(kube-apiserver) which use apiserver-etcd-client certificate.")
	log.Warnln("If kube-apiserver is running, restart it so that it uses the renewed etcd client certificate.")

Add renew certificates subcommand #147

Add renew certificates subcommand #147

Conversation

pytimer commented Dec 22, 2019

k8s-ci-robot commented Dec 22, 2019

chuckha left a comment

Choose a reason for hiding this comment

pytimer commented Jan 14, 2020

chuckha commented Jan 14, 2020

pytimer commented Jan 15, 2020

chuckha left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha commented Jan 15, 2020

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

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

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

Choose a reason for hiding this comment

pytimer Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlipovetsky commented Feb 8, 2020

pytimer commented Feb 8, 2020

pytimer commented Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

dlipovetsky Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

dlipovetsky commented Feb 9, 2020 • edited Loading

dlipovetsky commented Feb 10, 2020

pytimer commented Feb 10, 2020

dlipovetsky commented Feb 10, 2020

fejta-bot commented Sep 9, 2020

fejta-bot commented Oct 9, 2020

Avatat commented Oct 9, 2020

fejta-bot commented Nov 8, 2020

k8s-ci-robot commented Nov 8, 2020

ntaylor1781 commented Jan 27, 2021

dlipovetsky commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

ntaylor1781 commented Feb 1, 2021

pytimer commented Feb 2, 2021 • edited Loading

ntaylor1781 commented Feb 2, 2021

fejta-bot commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

ntaylor1781 commented Mar 22, 2021

Avatat commented May 17, 2021

sfgroups-k8s commented Jun 24, 2021

pytimer Jan 18, 2020 •

edited

Loading

pytimer commented Feb 9, 2020 •

edited

Loading

dlipovetsky Feb 9, 2020 •

edited

Loading

dlipovetsky commented Feb 9, 2020 •

edited

Loading

pytimer commented Feb 2, 2021 •

edited

Loading