modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates #111061

pacoxu · 2022-07-11T03:02:37Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

xref #109077

set it to optional in this release, but the default will use it
in the next release, we can set the default not to include it.

Special notes for your reviewer:

API validation of CSR create and update requests
Controller tolerating CSR objects with/without that usage
Kubelets requesting CSRs properly without that usage if given a non-RSA key

kubernetes/pkg/controller/certificates/signer/signer.go

Lines 277 to 293 in 14e8db0

    
           func validAPIServerClientUsages(usages []capi.KeyUsage) error { 
        
           	hasClientAuth := false 
        
           	for _, u := range usages { 
        
           		switch u { 
        
           		// these usages are optional 
        
           		case capi.UsageDigitalSignature, capi.UsageKeyEncipherment: 
        
           		case capi.UsageClientAuth: 
        
           			hasClientAuth = true 
        
           		default: 
        
           			return fmt.Errorf("invalid usage for client certificate: %s", u) 
        
           		} 
        
           	} 
        
           	if !hasClientAuth { 
        
           		return fmt.Errorf("missing required usage for client certificate: %s", capi.UsageClientAuth) 
        
           	} 
        
           	return nil 
        
           }

The controller already tolerates it.

Does this PR introduce a user-facing change?

Modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates without key encipherment

k8s-ci-robot · 2022-07-11T03:02:45Z

@pacoxu: 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.

liggitt

one test nit, then lgtm

pkg/apis/certificates/v1beta1/defaults_test.go

…ges for kubelet client and serving certificates Signed-off-by: Paco Xu <paco.xu@daocloud.io>

liggitt · 2022-08-02T21:18:00Z

/lgtm
/approve

k8s-ci-robot · 2022-08-02T21:18:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, pacoxu

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:

~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controller/certificates/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 · 2022-08-02T21:18:39Z

once this merges, if you don't mind, can you open the PR we'll merge in 1.26 relaxing the API defaulting?

pacoxu · 2022-08-02T21:19:48Z

once this merges, if you don't mind, can you open the PR we'll merge in 1.26 relaxing the API defaulting?

of course.

Thanks for your detailed review and guide.

pacoxu · 2022-08-03T00:19:12Z

/retest

pacoxu · 2022-08-03T09:08:51Z

I opened #111660 for v1.26.

sftim · 2022-08-10T11:44:53Z

Could we update the changelog entry to mention that this change is in connection with CertificateSigningRequest?

pacoxu · 2022-08-10T11:54:28Z

Could we update the changelog entry to mention that this change is in connection with CertificateSigningRequest?

Modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates without key encipherment

I tried to change it like above to mention the sign request approval.

This commit modifies the existing CSr approver to tolerate a set of usages for kubelet client and serving certificates without key encipherment to accomodate recent upstream changes around key encipherment. Releated upstream PR's: kubernetes/kubernetes#111061 kubernetes/kubernetes#111660

This commit modifies the existing CSR approval process to accept a set of CSR usages with and without key encipherment. This is required after the recent upstream changes where kubelet may request a CSR without the key encipherment usage when given a non-RSA key. Releated upstream PR's: kubernetes/kubernetes#111061 kubernetes/kubernetes#111660

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 11, 2022

k8s-ci-robot requested review from smarterclayton and wojtek-t July 11, 2022 03:03

k8s-ci-robot added area/kubelet sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 11, 2022

pacoxu marked this pull request as draft July 11, 2022 03:35

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2022

pacoxu force-pushed the key-encipherment-optional branch 2 times, most recently from e56f450 to a80b502 Compare July 11, 2022 04:07

pacoxu marked this pull request as ready for review July 11, 2022 04:07

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2022

k8s-ci-robot requested review from mikedanese and thockin July 11, 2022 04:08

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Jul 11, 2022

pacoxu changed the title ~~Set Key encipherment optional~~ [WIP]Set Key encipherment optional Jul 11, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2022

pacoxu force-pushed the key-encipherment-optional branch 2 times, most recently from 6f2ae41 to fb06fd2 Compare July 14, 2022 09:36

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2022

pacoxu force-pushed the key-encipherment-optional branch from fb06fd2 to bed76fa Compare July 14, 2022 15:07

pacoxu requested a review from liggitt August 2, 2022 15:02

liggitt reviewed Aug 2, 2022

View reviewed changes

pkg/apis/certificates/v1beta1/defaults_test.go Outdated Show resolved Hide resolved

modify the signing/approving controller to tolerate either set of usa…

e6176c2

…ges for kubelet client and serving certificates Signed-off-by: Paco Xu <paco.xu@daocloud.io>

pacoxu force-pushed the key-encipherment-optional branch from bab84a7 to e6176c2 Compare August 2, 2022 21:12

pacoxu requested a review from liggitt August 2, 2022 21:13

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2022

k8s-ci-robot merged commit cb41d50 into kubernetes:master Aug 3, 2022

SIG Node PR Triage automation moved this from Triage to Done Aug 3, 2022

pacoxu mentioned this pull request Aug 3, 2022

Key encipherment usage v1.27 #111660

Merged

pacoxu mentioned this pull request Dec 27, 2022

only add key encipherment usage for RSA private keys kubernetes/cloud-provider-gcp#442

Closed

dims mentioned this pull request Apr 20, 2023

KeyUsage array content changed in 1.27 for a non-RSA key #117506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates #111061

modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates #111061

pacoxu commented Jul 11, 2022 •

edited

k8s-ci-robot commented Jul 11, 2022

liggitt left a comment

liggitt commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

liggitt commented Aug 2, 2022

pacoxu commented Aug 2, 2022

pacoxu commented Aug 3, 2022

pacoxu commented Aug 3, 2022

sftim commented Aug 10, 2022

pacoxu commented Aug 10, 2022

	func validAPIServerClientUsages(usages []capi.KeyUsage) error {
	hasClientAuth := false
	for _, u := range usages {
	switch u {
	// these usages are optional
	case capi.UsageDigitalSignature, capi.UsageKeyEncipherment:
	case capi.UsageClientAuth:
	hasClientAuth = true
	default:
	return fmt.Errorf("invalid usage for client certificate: %s", u)
	}
	}
	if !hasClientAuth {
	return fmt.Errorf("missing required usage for client certificate: %s", capi.UsageClientAuth)
	}
	return nil
	}

modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates #111061

modify the signing/approving controller to tolerate either set of usages for kubelet client and serving certificates #111061

Conversation

pacoxu commented Jul 11, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Jul 11, 2022

liggitt left a comment

Choose a reason for hiding this comment

liggitt commented Aug 2, 2022

k8s-ci-robot commented Aug 2, 2022

liggitt commented Aug 2, 2022

pacoxu commented Aug 2, 2022

pacoxu commented Aug 3, 2022

pacoxu commented Aug 3, 2022

sftim commented Aug 10, 2022

pacoxu commented Aug 10, 2022

pacoxu commented Jul 11, 2022 •

edited