default a CSR's allowed usage to key encipherment and digital signing #39698

Merged
merged 2 commits into from Jan 12, 2017

Projects

None yet

8 participants

@mikedanese
Member

Some pretty safe and sane defaults.

@liggitt

@mikedanese mikedanese requested a review from pipejakob Jan 10, 2017
@k8s-reviewable

This change is Reviewable

@mikedanese mikedanese assigned pipejakob and unassigned lavalamp Jan 11, 2017
@@ -0,0 +1,31 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+ )
+}
+func SetDefaults_CertificateSigningRequestSpec(obj *CertificateSigningRequestSpec) {
+ if len(obj.Usages) == 0 {
@liggitt
liggitt Jan 11, 2017 Member

Maybe only if nil? An explicitly specified empty list seems like an invalid CSR

@liggitt
liggitt Jan 11, 2017 Member

thanks, validation check for nil usages would probably be helpful

@liggitt
Member
liggitt commented Jan 11, 2017

Did kubelet CSR usage get updated to request a client cert usage yet?

@jcbsmpsn jcbsmpsn was assigned by mikedanese Jan 11, 2017
- allErrs = append(allErrs, field.Invalid(field.NewPath("request"), csr.Spec.Request, fmt.Sprintf("%v", err)))
+ allErrs = append(allErrs, field.Invalid(specPath.Child("request"), csr.Spec.Request, fmt.Sprintf("%v", err)))
+ }
+ if csr.Spec.Usages == nil {
@liggitt
liggitt Jan 11, 2017 Member

with defaulting, should never be nil, and clients that don't specify should never be empty. I was thinking a zero-length check here should make it invalid

@mikedanese
mikedanese Jan 11, 2017 Member

Why wouldn't we just default the zero length array then if it is invalid?

@liggitt
liggitt Jan 11, 2017 Member

old clients that don't know about the field send nil, so we default them.

new clients that know about the field and explicitly specify an empty list should get an error

@mikedanese
mikedanese Jan 11, 2017 Member

I understand now. Fixed.

mikedanese added some commits Jan 10, 2017
@mikedanese mikedanese default a CSR's allowed usage to key encipherment and digital signing 06077ac
@mikedanese mikedanese autogenerated
5bbd4cf
@liggitt
Member
liggitt commented Jan 12, 2017

LGTM

@mikedanese mikedanese added the lgtm label Jan 12, 2017
@k8s-merge-robot
Collaborator

Automatic merge from submit-queue (batch tested with PRs 39803, 39698, 39537, 39478)

@k8s-merge-robot k8s-merge-robot merged commit 0abdcfb into kubernetes:master Jan 12, 2017

14 checks passed

Jenkins Bazel Build Build succeeded.
Details
Jenkins CRI GCE Node e2e Build succeeded.
Details
Jenkins GCE Node e2e Build succeeded.
Details
Jenkins GCE e2e Build succeeded.
Details
Jenkins GCE etcd3 e2e Build succeeded.
Details
Jenkins GCI GCE e2e Build succeeded.
Details
Jenkins GCI GKE smoke e2e Build succeeded.
Details
Jenkins GKE smoke e2e Build succeeded.
Details
Jenkins Kubemark GCE e2e Build succeeded.
Details
Jenkins kops AWS e2e Build succeeded.
Details
Jenkins unit/integration Build succeeded.
Details
Jenkins verification Build succeeded.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mikedanese authorized
Details
@mikedanese mikedanese deleted the mikedanese:default-csr branch Jan 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment