Make CA valid 1 hour in the past

[champtar]

What type of PR is this?

/kind feature

What this PR does / why we need it:

When running kubeadm / installing k8s early during boot, the CA certificate can be generated before time is synchronised and time is jumped backward.
Make notBefore 1 hour in the past to accept small clock jump.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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]

Hi @champtar. Thanks for your PR.

I'm waiting for a kubernetes 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.

[alexzielenski]

/sig auth
/remove-sig api-machinery

[champtar]

Should we add area/kubeadm and sig/cluster-lifecycle ? The only usage of NewSelfSignedCACert() outside of tests is cmd/kubeadm/app/util/pkiutil/pki_helpers.go

[dims]

/ok-to-test

[dims]

/assign @neolit123

[neolit123]

for context, we had a related duscussion in the past
#76714

IMO, if we are changing notBefore for CA certs, with one hour padding we should also do it for other certs. @champtar WDYT?

Should we add area/kubeadm and sig/cluster-lifecycle ? The only usage of NewSelfSignedCACert() outside of tests is cmd/kubeadm/app/util/pkiutil/pki_helpers.go

the k/k usage may only be by kubeadm, but there could be other external direct users of client go.

[champtar]

for context, we had a related duscussion in the past #76714

IMO, if we are changing notBefore for CA certs, with one hour padding we should also do it for other certs. @champtar WDYT?

This is the only place in k/k where NotBefore == time.Now()

$ git grep 'CreateCertificate(' | grep -v test
cmd/kubeadm/app/util/pkiutil/pki_helpers.go:	certDERBytes, err := x509.CreateCertificate(cryptorand.Reader, &certTmpl, caCert, key.Public(), caKey)
pkg/controller/certificates/authority/authority.go:	der, err := x509.CreateCertificate(rand.Reader, tmpl, ca.Certificate, cr.PublicKey, ca.PrivateKey)
staging/src/k8s.io/client-go/util/cert/cert.go:	certDERBytes, err := x509.CreateCertificate(cryptorand.Reader, &tmpl, &tmpl, key.Public(), key)
staging/src/k8s.io/client-go/util/cert/cert.go:	caDERBytes, err := x509.CreateCertificate(cryptorand.Reader, &caTemplate, &caTemplate, &caKey.PublicKey, caKey)
staging/src/k8s.io/client-go/util/cert/cert.go:	derBytes, err := x509.CreateCertificate(cryptorand.Reader, &template, caCertificate, &priv.PublicKey, caKey)
vendor/go.etcd.io/etcd/client/pkg/v3/transport/listener.go:	derBytes, err := x509.CreateCertificate(rand.Reader, &tmpl, &tmpl, &priv.PublicKey, priv)

In the kubeadm case NotBefore == ca.NotBefore

kubernetes/cmd/kubeadm/app/util/pkiutil/pki_helpers.go

Line 664 in 1193ab6

NotBefore: caCert.NotBefore,

In pkg/controller/certificates/authority there is already some backdate logic

kubernetes/pkg/controller/certificates/authority/policies.go

Line 90 in 1193ab6

tmpl.NotBefore = now.Add(-p.Backdate)

In client-go in GenerateSelfSignedCertKeyWithFixtures() there is already -1hour (that's why I picked 1h)

kubernetes/staging/src/k8s.io/client-go/util/cert/cert.go

Lines 103 to 104 in 1193ab6

	func GenerateSelfSignedCertKeyWithFixtures(host string, alternateIPs []net.IP, alternateDNS []string, fixtureDirectory string) ([]byte, []byte, error) {
	validFrom := time.Now().Add(-time.Hour) // valid an hour earlier to avoid flakes due to clock skew

Should we add area/kubeadm and sig/cluster-lifecycle ? The only usage of NewSelfSignedCACert() outside of tests is cmd/kubeadm/app/util/pkiutil/pki_helpers.go

the k/k usage may only be by kubeadm, but there could be other external direct users of client go.

Good point, if we want to be safe I can introduce a new function that take NotBefore as parameter and not change the behavior for everyone, please tell me what is preferred

[neolit123]

thanks for the info.

In client-go in GenerateSelfSignedCertKeyWithFixtures() there is already -1hour (that's why I picked 1h)

+1 given we already have this in other places and it was discussed before.

Good point, if we want to be safe I can introduce a new function that take NotBefore as parameter and not change the behavior for everyone, please tell me what is preferred

unclear what is better. likely the -1h padding without opt-out is fine. i can lgtm, but approval is up to api machinery (owners of client go).

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 468ef00647a9d69239999ddfd3b63cb304100524

[neolit123]

/release-note-edit

client-go: make generated CA certificates valid 1 hour in the past (NewSelfSignedCACert). Applies to CA certificates and other certificates generated by kubeadm.

owners: please adjust the note if needed

[dims]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: champtar, dims

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:

• staging/src/k8s.io/client-go/util/cert/OWNERS [dims]

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

[aojea]

This function is exported from client-go, is this really safe to do?

[neolit123]

This function is exported from client-go, is this really safe to do?

i think the change is fine based on prior discussions.
#76714 (comment)
but as i noted earlier api machinery should take a look.

@champtar , your backports state "feature". we don't backport features per se. usually it has to be a blocking bug fix..

[champtar]

@champtar , your backports state "feature". we don't backport features per se. usually it has to be a blocking bug fix..

I just copied the kind/feature from this PR.

It's blocking for my use case because I want to start as fast as possible, so before time sync, and the install fails if there is a time jump just after certificate generation, and I don't have a workaround except this PR.
So it's a blocking bug fix for me, but for k8s is it a bug fix ?

[neolit123]

It's blocking for my use case because I want to start as fast as possible, so before time sync, and the install fails if there is a time jump just after certificate generation, and I don't have a workaround except this PR.

a slightly involved workaround for kubeadm would be to sign your own CA/certs/keys.

So it's a blocking bug fix for me, but for k8s is it a bug fix ?

i think the client-go owners should take a look at this PR and the backports.

EDIT: you can ping #sig-api-machinery on k8s slack.

[champtar]

It's blocking for my use case because I want to start as fast as possible, so before time sync, and the install fails if there is a time jump just after certificate generation, and I don't have a workaround except this PR.

a slightly involved workaround for kubeadm would be to sign your own CA/certs/keys.

openssl doesn't provide an easy way to put the not before time in the past, my simplest workaround involved using faketime + openssl and some certificates templates, but that's seemed a lot compared to a 1 line change

So it's a blocking bug fix for me, but for k8s is it a bug fix ?

i think the client-go owners should take a look at this PR and the backports.

EDIT: you can ping #sig-api-machinery on k8s slack.

Will do, thanks

[aojea]

/assign @enj @deads2k @jpbetz

This is exposed and used in more places that kubeadm , it will be better if both sig-auth and sig-apimachinery sign on this and on the backports to be completely sure and have consensus

[champtar]

Slack discussion: https://kubernetes.slack.com/archives/C0EG7JC6T/p1687815949656259

[neolit123]

/release-note-none

[k8s-ci-robot]

@neolit123: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none".

In response to this:

/release-note-none

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.

[neolit123]

/release-note-edit

NONE