certificate_manager: Check that template differs from current cert before rotation #69991

agunnerson-ibm · 2018-10-18T19:44:17Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

With the current behavior, when kubelet starts, a templateChanged
event is always fired off because it only checks if getLastRequest
matches getTemplate. The last request only exists in memory and thus
is initially nil and can't ever match the current template during
startup.

This causes kubelet to request the signing of a new CSR every time it's
restarted. This commit changes the behavior so that templateChanged is
only fired off if the currently template doesn't match both the current
certificate and the last template.

Which issue(s) this PR fixes:

Fixes #69471

Release note:

When `--rotate-server-certificates` is enabled, kubelet will no longer request a new certificate on startup if the current certificate on disk is satisfactory.

fedebongio · 2018-10-18T20:54:43Z

/cc @caesarxuchao

liggitt · 2018-10-19T22:37:21Z

/ok-to-test

liggitt · 2018-10-23T05:13:07Z

there's a gofmt error that needs fixing to make the bot happy:

-func (m* manager) certSatisfiesTemplate() bool {
+func (m *manager) certSatisfiesTemplate() bool {
 	m.certAccessLock.RLock()
 	defer m.certAccessLock.RUnlock()
 	if m.cert == nil {

Run ./hack/update-gofmt.sh

liggitt · 2018-10-23T05:14:36Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

-			glog.V(2).Infof("Current certificate CN (%s) does not match requested CN (%s), rotating now", m.cert.Leaf.Subject.CommonName, template.Subject.CommonName)
-			return time.Now()
+			glog.V(2).Infof("Current certificate CN (%s) does not match requested CN (%s)", m.cert.Leaf.Subject.CommonName, template.Subject.CommonName)
+			return false
 		}


also check sets.NewString(m.cert.Leaf.Subject.Organizations...).HasAll(template.Subject.Organizations...)

liggitt · 2018-10-23T05:17:00Z

/test pull-kubernetes-e2e-gke

liggitt · 2018-10-23T05:17:44Z

thanks for the PR, just a couple comments. exercising the logic in a unit test would be good as well

awly · 2018-10-23T20:18:50Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

+		return time.Now()
+	}
+
+	m.certAccessLock.RLock()


certSatisfiesTemplate already locks the mutex, this will deadlock.
Recommend splitting certSatisfiesTemplate into two functions: unlocked one and a locking wrapper. Call the unlocked one here and the locked one in manager.Start

agunnerson-ibm · 2018-10-24T17:35:54Z

Thanks everyone for the comments! I've updated this PR to address them.

Changes:

Fixed all gofmt errors
Added check to ensure that the organizations in the x509 subject of the cert match the template
Split certSatisfiesTemplate into certSatisfiesTemplate and certSatisfiesTemplateLocked and use the latter if certAccessLock is already locked
Added unit tests for certSatisfiesTemplate

agunnerson-ibm · 2018-10-24T18:39:22Z

Fixed the lint issue reported by pull-kubernetes-verify.

awly

Two small nits, LGTM otherwise

awly · 2018-10-24T20:17:43Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go

+			}
+
+			if m.certSatisfiesTemplate() != tc.shouldSatisfy {
+				if tc.shouldSatisfy {


Don't wrap errors, store certSatisfiesTemplate result in a var and do a simpler error print:

t.Errorf("cert: %+v, template: %+v, certSatisfiesTemplate returned %v, want %v", m.cert, tc.template, got, tc.shouldSatisfy)

Thanks, I'll fix that.

awly · 2018-10-24T20:18:06Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go

+			m := manager{
+				cert:        tlsCert,
+				getTemplate: func() *x509.CertificateRequest { return tc.template },
+				usages:      []certificates.KeyUsage{},


Is this needed?

Nope, I'll remove it.

…fore rotation With the current behavior, when kubelet starts, a `templateChanged` event is always fired off because it only checks if `getLastRequest` matches `getTemplate`. The last request only exists in memory and thus is initially `nil` and can't ever match the current template during startup. This causes kubelet to request the signing of a new CSR every time it's restarted. This commit changes the behavior so that `templateChanged` is only fired off if the currently template doesn't match both the current certificate and the last template. Fixes kubernetes#69471 Signed-off-by: Andrew Gunnerson <andrew.gunnerson@us.ibm.com>

agunnerson-ibm · 2018-10-24T21:16:32Z

The latest commit should address @awly's comments above.

awly · 2018-10-24T21:23:05Z

/lgtm

liggitt · 2018-10-24T21:25:40Z

/approve

k8s-ci-robot · 2018-10-24T21:25:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agunnerson-ibm, liggitt

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/certificate/OWNERS~~ [liggitt]

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

liggitt · 2018-10-24T21:25:56Z

thanks for putting this together

fejta-bot · 2018-10-25T02:01:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. labels Oct 18, 2018

k8s-ci-robot requested review from mikedanese and smarterclayton October 18, 2018 19:44

agunnerson-ibm mentioned this pull request Oct 18, 2018

Kubelet generates a new CSR on start even if it has a valid certificate on disk #69471

Closed

k8s-ci-robot requested a review from caesarxuchao October 18, 2018 20:54

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 19, 2018

liggitt reviewed Oct 23, 2018

View reviewed changes

mikedanese assigned awly Oct 23, 2018

liggitt added this to the v1.13 milestone Oct 23, 2018

liggitt added sig/auth Categorizes an issue or PR as relevant to SIG Auth. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Oct 23, 2018

awly reviewed Oct 23, 2018

View reviewed changes

agunnerson-ibm force-pushed the issue-69471 branch from 0b5c749 to c58b569 Compare October 24, 2018 17:35

k8s-ci-robot 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 Oct 24, 2018

agunnerson-ibm force-pushed the issue-69471 branch from c58b569 to c11f21e Compare October 24, 2018 18:38

awly reviewed Oct 24, 2018

View reviewed changes

agunnerson-ibm force-pushed the issue-69471 branch from c11f21e to b9ab65d Compare October 24, 2018 21:16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2018

k8s-ci-robot merged commit d96f235 into kubernetes:master Oct 25, 2018

agunnerson-ibm deleted the issue-69471 branch October 25, 2018 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

certificate_manager: Check that template differs from current cert before rotation #69991

certificate_manager: Check that template differs from current cert before rotation #69991

agunnerson-ibm commented Oct 18, 2018

fedebongio commented Oct 18, 2018

liggitt commented Oct 19, 2018

liggitt commented Oct 23, 2018

liggitt Oct 23, 2018

liggitt commented Oct 23, 2018

liggitt commented Oct 23, 2018

awly Oct 23, 2018

agunnerson-ibm commented Oct 24, 2018

agunnerson-ibm commented Oct 24, 2018

awly left a comment

awly Oct 24, 2018

agunnerson-ibm Oct 24, 2018

awly Oct 24, 2018

agunnerson-ibm Oct 24, 2018

agunnerson-ibm commented Oct 24, 2018

awly commented Oct 24, 2018

liggitt commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

liggitt commented Oct 24, 2018

fejta-bot commented Oct 25, 2018

certificate_manager: Check that template differs from current cert before rotation #69991

certificate_manager: Check that template differs from current cert before rotation #69991

Conversation

agunnerson-ibm commented Oct 18, 2018

fedebongio commented Oct 18, 2018

liggitt commented Oct 19, 2018

liggitt commented Oct 23, 2018

liggitt Oct 23, 2018

Choose a reason for hiding this comment

liggitt commented Oct 23, 2018

liggitt commented Oct 23, 2018

awly Oct 23, 2018

Choose a reason for hiding this comment

agunnerson-ibm commented Oct 24, 2018

agunnerson-ibm commented Oct 24, 2018

awly left a comment

Choose a reason for hiding this comment

awly Oct 24, 2018

Choose a reason for hiding this comment

agunnerson-ibm Oct 24, 2018

Choose a reason for hiding this comment

awly Oct 24, 2018

Choose a reason for hiding this comment

agunnerson-ibm Oct 24, 2018

Choose a reason for hiding this comment

agunnerson-ibm commented Oct 24, 2018

awly commented Oct 24, 2018

liggitt commented Oct 24, 2018

k8s-ci-robot commented Oct 24, 2018

liggitt commented Oct 24, 2018

fejta-bot commented Oct 25, 2018