Skip to content

Commit 595adc4

Browse files
liggittliggitt
committed
Validate unknown and duplicate usages in CSR v1
1 parent 14cb921 commit 595adc4

File tree

2 files changed

+167
-1
lines changed

2 files changed

+167
-1
lines changed

‎pkg/apis/certificates/validation/validation.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ type certificateValidationOptions struct {
7474
allowEmptyConditionType bool
7575
// allow arbitrary content in status.certificate
7676
allowArbitraryCertificate bool
77+
// allow usages values outside the known set
78+
allowUnknownUsages bool
79+
// allow duplicate usages values
80+
allowDuplicateUsages bool
7781
}
7882

7983
// validateCSR validates the signature and formatting of a base64-wrapped,
@@ -140,6 +144,34 @@ func ValidateCertificateSigningRequestCreate(csr *certificates.CertificateSignin
140144
return validateCertificateSigningRequest(csr, opts)
141145
}
142146

147+
var (
148+
allValidUsages = sets.NewString(
149+
string(certificates.UsageSigning),
150+
string(certificates.UsageDigitalSignature),
151+
string(certificates.UsageContentCommitment),
152+
string(certificates.UsageKeyEncipherment),
153+
string(certificates.UsageKeyAgreement),
154+
string(certificates.UsageDataEncipherment),
155+
string(certificates.UsageCertSign),
156+
string(certificates.UsageCRLSign),
157+
string(certificates.UsageEncipherOnly),
158+
string(certificates.UsageDecipherOnly),
159+
string(certificates.UsageAny),
160+
string(certificates.UsageServerAuth),
161+
string(certificates.UsageClientAuth),
162+
string(certificates.UsageCodeSigning),
163+
string(certificates.UsageEmailProtection),
164+
string(certificates.UsageSMIME),
165+
string(certificates.UsageIPsecEndSystem),
166+
string(certificates.UsageIPsecTunnel),
167+
string(certificates.UsageIPsecUser),
168+
string(certificates.UsageTimestamping),
169+
string(certificates.UsageOCSPSigning),
170+
string(certificates.UsageMicrosoftSGC),
171+
string(certificates.UsageNetscapeSGC),
172+
)
173+
)
174+
143175
func validateCertificateSigningRequest(csr *certificates.CertificateSigningRequest, opts certificateValidationOptions) field.ErrorList {
144176
isNamespaced := false
145177
allErrs := apivalidation.ValidateObjectMeta(&csr.ObjectMeta, isNamespaced, ValidateCertificateRequestName, field.NewPath("metadata"))
@@ -152,6 +184,22 @@ func validateCertificateSigningRequest(csr *certificates.CertificateSigningReque
152184
if len(csr.Spec.Usages) == 0 {
153185
allErrs = append(allErrs, field.Required(specPath.Child("usages"), "usages must be provided"))
154186
}
187+
if !opts.allowUnknownUsages {
188+
for i, usage := range csr.Spec.Usages {
189+
if !allValidUsages.Has(string(usage)) {
190+
allErrs = append(allErrs, field.NotSupported(specPath.Child("usages").Index(i), usage, allValidUsages.List()))
191+
}
192+
}
193+
}
194+
if !opts.allowDuplicateUsages {
195+
seen := make(map[certificates.KeyUsage]bool, len(csr.Spec.Usages))
196+
for i, usage := range csr.Spec.Usages {
197+
if seen[usage] {
198+
allErrs = append(allErrs, field.Duplicate(specPath.Child("usages").Index(i), usage))
199+
}
200+
seen[usage] = true
201+
}
202+
}
155203
if !opts.allowLegacySignerName && csr.Spec.SignerName == certificates.LegacyUnknownSignerName {
156204
allErrs = append(allErrs, field.Invalid(specPath.Child("signerName"), csr.Spec.SignerName, "the legacy signerName is not allowed via this API version"))
157205
} else {
@@ -377,6 +425,8 @@ func getValidationOptions(version schema.GroupVersion, newCSR, oldCSR *certifica
377425
allowDuplicateConditionTypes: allowDuplicateConditionTypes(version, oldCSR),
378426
allowEmptyConditionType: allowEmptyConditionType(version, oldCSR),
379427
allowArbitraryCertificate: allowArbitraryCertificate(version, newCSR, oldCSR),
428+
allowDuplicateUsages: allowDuplicateUsages(version, oldCSR),
429+
allowUnknownUsages: allowUnknownUsages(version, oldCSR),
380430
}
381431
}
382432

@@ -465,3 +515,45 @@ func allowArbitraryCertificate(version schema.GroupVersion, newCSR, oldCSR *cert
465515
return false
466516
}
467517
}
518+
519+
func allowUnknownUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
520+
switch {
521+
case version == certificatesv1beta1.SchemeGroupVersion:
522+
return true // compatibility with v1beta1
523+
case oldCSR != nil && hasUnknownUsage(oldCSR.Spec.Usages):
524+
return true // compatibility with existing data
525+
default:
526+
return false
527+
}
528+
}
529+
530+
func hasUnknownUsage(usages []certificates.KeyUsage) bool {
531+
for _, usage := range usages {
532+
if !allValidUsages.Has(string(usage)) {
533+
return true
534+
}
535+
}
536+
return false
537+
}
538+
539+
func allowDuplicateUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
540+
switch {
541+
case version == certificatesv1beta1.SchemeGroupVersion:
542+
return true // compatibility with v1beta1
543+
case oldCSR != nil && hasDuplicateUsage(oldCSR.Spec.Usages):
544+
return true // compatibility with existing data
545+
default:
546+
return false
547+
}
548+
}
549+
550+
func hasDuplicateUsage(usages []certificates.KeyUsage) bool {
551+
seen := make(map[certificates.KeyUsage]bool, len(usages))
552+
for _, usage := range usages {
553+
if seen[usage] {
554+
return true
555+
}
556+
seen[usage] = true
557+
}
558+
return false
559+
}

‎pkg/apis/certificates/validation/validation_test.go‎

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -263,12 +263,53 @@ func TestValidateCertificateSigningRequestCreate(t *testing.T) {
263263
},
264264
errs: field.ErrorList{},
265265
},
266+
"missing usages": {
267+
csr: capi.CertificateSigningRequest{
268+
ObjectMeta: validObjectMeta,
269+
Spec: capi.CertificateSigningRequestSpec{
270+
Usages: []capi.KeyUsage{},
271+
Request: newCSRPEM(t),
272+
SignerName: validSignerName,
273+
},
274+
},
275+
errs: field.ErrorList{
276+
field.Required(specPath.Child("usages"), "usages must be provided"),
277+
},
278+
},
279+
"unknown and duplicate usages - v1beta1": {
280+
gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1beta1"},
281+
csr: capi.CertificateSigningRequest{
282+
ObjectMeta: validObjectMeta,
283+
Spec: capi.CertificateSigningRequestSpec{
284+
Usages: []capi.KeyUsage{"unknown", "unknown"},
285+
Request: newCSRPEM(t),
286+
SignerName: validSignerName,
287+
},
288+
},
289+
errs: field.ErrorList{},
290+
},
291+
"unknown and duplicate usages - v1": {
292+
gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1"},
293+
csr: capi.CertificateSigningRequest{
294+
ObjectMeta: validObjectMeta,
295+
Spec: capi.CertificateSigningRequestSpec{
296+
Usages: []capi.KeyUsage{"unknown", "unknown"},
297+
Request: newCSRPEM(t),
298+
SignerName: validSignerName,
299+
},
300+
},
301+
errs: field.ErrorList{
302+
field.NotSupported(specPath.Child("usages").Index(0), capi.KeyUsage("unknown"), allValidUsages.List()),
303+
field.NotSupported(specPath.Child("usages").Index(1), capi.KeyUsage("unknown"), allValidUsages.List()),
304+
field.Duplicate(specPath.Child("usages").Index(1), capi.KeyUsage("unknown")),
305+
},
306+
},
266307
}
267308
for name, test := range tests {
268309
t.Run(name, func(t *testing.T) {
269310
el := ValidateCertificateSigningRequestCreate(&test.csr, test.gv)
270311
if !reflect.DeepEqual(el, test.errs) {
271-
t.Errorf("returned and expected errors did not match - expected %v but got %v", test.errs.ToAggregate(), el.ToAggregate())
312+
t.Errorf("returned and expected errors did not match - expected\n%v\nbut got\n%v", test.errs.ToAggregate(), el.ToAggregate())
272313
}
273314
})
274315
}
@@ -331,6 +372,8 @@ func Test_getValidationOptions(t *testing.T) {
331372
allowDuplicateConditionTypes: true,
332373
allowEmptyConditionType: true,
333374
allowArbitraryCertificate: true,
375+
allowUnknownUsages: true,
376+
allowDuplicateUsages: true,
334377
},
335378
},
336379
{
@@ -352,6 +395,8 @@ func Test_getValidationOptions(t *testing.T) {
352395
allowDuplicateConditionTypes: true,
353396
allowEmptyConditionType: true,
354397
allowArbitraryCertificate: true,
398+
allowUnknownUsages: true,
399+
allowDuplicateUsages: true,
355400
},
356401
},
357402
{
@@ -424,6 +469,22 @@ func Test_getValidationOptions(t *testing.T) {
424469
allowArbitraryCertificate: true,
425470
},
426471
},
472+
{
473+
name: "v1 compatible update, existing unknown usages",
474+
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
475+
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"unknown"}}},
476+
want: certificateValidationOptions{
477+
allowUnknownUsages: true,
478+
},
479+
},
480+
{
481+
name: "v1 compatible update, existing duplicate usages",
482+
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
483+
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"any", "any"}}},
484+
want: certificateValidationOptions{
485+
allowDuplicateUsages: true,
486+
},
487+
},
427488
}
428489
for _, tt := range tests {
429490
t.Run(tt.name, func(t *testing.T) {
@@ -587,6 +648,19 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
587648
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{Certificate: invalidCertificateNoPEM}},
588649
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{Certificate: invalidCertificateNoPEM}},
589650
},
651+
{
652+
name: "finalizer change with duplicate and unknown usages",
653+
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: capi.CertificateSigningRequestSpec{
654+
Usages: []capi.KeyUsage{"unknown", "unknown"},
655+
Request: newCSRPEM(t),
656+
SignerName: validSignerName,
657+
}},
658+
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: capi.CertificateSigningRequestSpec{
659+
Usages: []capi.KeyUsage{"unknown", "unknown"},
660+
Request: newCSRPEM(t),
661+
SignerName: validSignerName,
662+
}},
663+
},
590664
{
591665
name: "add Approved condition",
592666
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{

0 commit comments

Comments
 (0)