diff --git a/pkg/apis/certificates/validation/validation.go b/pkg/apis/certificates/validation/validation.go index 20d84a3395a..8de97049536 100644 --- a/pkg/apis/certificates/validation/validation.go +++ b/pkg/apis/certificates/validation/validation.go @@ -74,6 +74,10 @@ type certificateValidationOptions struct { allowEmptyConditionType bool // allow arbitrary content in status.certificate allowArbitraryCertificate bool + // allow usages values outside the known set + allowUnknownUsages bool + // allow duplicate usages values + allowDuplicateUsages bool } // validateCSR validates the signature and formatting of a base64-wrapped, @@ -140,6 +144,34 @@ func ValidateCertificateSigningRequestCreate(csr *certificates.CertificateSignin return validateCertificateSigningRequest(csr, opts) } +var ( + allValidUsages = sets.NewString( + string(certificates.UsageSigning), + string(certificates.UsageDigitalSignature), + string(certificates.UsageContentCommitment), + string(certificates.UsageKeyEncipherment), + string(certificates.UsageKeyAgreement), + string(certificates.UsageDataEncipherment), + string(certificates.UsageCertSign), + string(certificates.UsageCRLSign), + string(certificates.UsageEncipherOnly), + string(certificates.UsageDecipherOnly), + string(certificates.UsageAny), + string(certificates.UsageServerAuth), + string(certificates.UsageClientAuth), + string(certificates.UsageCodeSigning), + string(certificates.UsageEmailProtection), + string(certificates.UsageSMIME), + string(certificates.UsageIPsecEndSystem), + string(certificates.UsageIPsecTunnel), + string(certificates.UsageIPsecUser), + string(certificates.UsageTimestamping), + string(certificates.UsageOCSPSigning), + string(certificates.UsageMicrosoftSGC), + string(certificates.UsageNetscapeSGC), + ) +) + func validateCertificateSigningRequest(csr *certificates.CertificateSigningRequest, opts certificateValidationOptions) field.ErrorList { isNamespaced := false allErrs := apivalidation.ValidateObjectMeta(&csr.ObjectMeta, isNamespaced, ValidateCertificateRequestName, field.NewPath("metadata")) @@ -152,6 +184,22 @@ func validateCertificateSigningRequest(csr *certificates.CertificateSigningReque if len(csr.Spec.Usages) == 0 { allErrs = append(allErrs, field.Required(specPath.Child("usages"), "usages must be provided")) } + if !opts.allowUnknownUsages { + for i, usage := range csr.Spec.Usages { + if !allValidUsages.Has(string(usage)) { + allErrs = append(allErrs, field.NotSupported(specPath.Child("usages").Index(i), usage, allValidUsages.List())) + } + } + } + if !opts.allowDuplicateUsages { + seen := make(map[certificates.KeyUsage]bool, len(csr.Spec.Usages)) + for i, usage := range csr.Spec.Usages { + if seen[usage] { + allErrs = append(allErrs, field.Duplicate(specPath.Child("usages").Index(i), usage)) + } + seen[usage] = true + } + } if !opts.allowLegacySignerName && csr.Spec.SignerName == certificates.LegacyUnknownSignerName { allErrs = append(allErrs, field.Invalid(specPath.Child("signerName"), csr.Spec.SignerName, "the legacy signerName is not allowed via this API version")) } else { @@ -377,6 +425,8 @@ func getValidationOptions(version schema.GroupVersion, newCSR, oldCSR *certifica allowDuplicateConditionTypes: allowDuplicateConditionTypes(version, oldCSR), allowEmptyConditionType: allowEmptyConditionType(version, oldCSR), allowArbitraryCertificate: allowArbitraryCertificate(version, newCSR, oldCSR), + allowDuplicateUsages: allowDuplicateUsages(version, oldCSR), + allowUnknownUsages: allowUnknownUsages(version, oldCSR), } } @@ -465,3 +515,45 @@ func allowArbitraryCertificate(version schema.GroupVersion, newCSR, oldCSR *cert return false } } + +func allowUnknownUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool { + switch { + case version == certificatesv1beta1.SchemeGroupVersion: + return true // compatibility with v1beta1 + case oldCSR != nil && hasUnknownUsage(oldCSR.Spec.Usages): + return true // compatibility with existing data + default: + return false + } +} + +func hasUnknownUsage(usages []certificates.KeyUsage) bool { + for _, usage := range usages { + if !allValidUsages.Has(string(usage)) { + return true + } + } + return false +} + +func allowDuplicateUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool { + switch { + case version == certificatesv1beta1.SchemeGroupVersion: + return true // compatibility with v1beta1 + case oldCSR != nil && hasDuplicateUsage(oldCSR.Spec.Usages): + return true // compatibility with existing data + default: + return false + } +} + +func hasDuplicateUsage(usages []certificates.KeyUsage) bool { + seen := make(map[certificates.KeyUsage]bool, len(usages)) + for _, usage := range usages { + if seen[usage] { + return true + } + seen[usage] = true + } + return false +} diff --git a/pkg/apis/certificates/validation/validation_test.go b/pkg/apis/certificates/validation/validation_test.go index 78675b664e9..b95b31615f0 100644 --- a/pkg/apis/certificates/validation/validation_test.go +++ b/pkg/apis/certificates/validation/validation_test.go @@ -263,12 +263,53 @@ func TestValidateCertificateSigningRequestCreate(t *testing.T) { }, errs: field.ErrorList{}, }, + "missing usages": { + csr: capi.CertificateSigningRequest{ + ObjectMeta: validObjectMeta, + Spec: capi.CertificateSigningRequestSpec{ + Usages: []capi.KeyUsage{}, + Request: newCSRPEM(t), + SignerName: validSignerName, + }, + }, + errs: field.ErrorList{ + field.Required(specPath.Child("usages"), "usages must be provided"), + }, + }, + "unknown and duplicate usages - v1beta1": { + gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1beta1"}, + csr: capi.CertificateSigningRequest{ + ObjectMeta: validObjectMeta, + Spec: capi.CertificateSigningRequestSpec{ + Usages: []capi.KeyUsage{"unknown", "unknown"}, + Request: newCSRPEM(t), + SignerName: validSignerName, + }, + }, + errs: field.ErrorList{}, + }, + "unknown and duplicate usages - v1": { + gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1"}, + csr: capi.CertificateSigningRequest{ + ObjectMeta: validObjectMeta, + Spec: capi.CertificateSigningRequestSpec{ + Usages: []capi.KeyUsage{"unknown", "unknown"}, + Request: newCSRPEM(t), + SignerName: validSignerName, + }, + }, + errs: field.ErrorList{ + field.NotSupported(specPath.Child("usages").Index(0), capi.KeyUsage("unknown"), allValidUsages.List()), + field.NotSupported(specPath.Child("usages").Index(1), capi.KeyUsage("unknown"), allValidUsages.List()), + field.Duplicate(specPath.Child("usages").Index(1), capi.KeyUsage("unknown")), + }, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { el := ValidateCertificateSigningRequestCreate(&test.csr, test.gv) if !reflect.DeepEqual(el, test.errs) { - t.Errorf("returned and expected errors did not match - expected %v but got %v", test.errs.ToAggregate(), el.ToAggregate()) + t.Errorf("returned and expected errors did not match - expected\n%v\nbut got\n%v", test.errs.ToAggregate(), el.ToAggregate()) } }) } @@ -331,6 +372,8 @@ func Test_getValidationOptions(t *testing.T) { allowDuplicateConditionTypes: true, allowEmptyConditionType: true, allowArbitraryCertificate: true, + allowUnknownUsages: true, + allowDuplicateUsages: true, }, }, { @@ -352,6 +395,8 @@ func Test_getValidationOptions(t *testing.T) { allowDuplicateConditionTypes: true, allowEmptyConditionType: true, allowArbitraryCertificate: true, + allowUnknownUsages: true, + allowDuplicateUsages: true, }, }, { @@ -424,6 +469,22 @@ func Test_getValidationOptions(t *testing.T) { allowArbitraryCertificate: true, }, }, + { + name: "v1 compatible update, existing unknown usages", + version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"}, + oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"unknown"}}}, + want: certificateValidationOptions{ + allowUnknownUsages: true, + }, + }, + { + name: "v1 compatible update, existing duplicate usages", + version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"}, + oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"any", "any"}}}, + want: certificateValidationOptions{ + allowDuplicateUsages: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -587,6 +648,19 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) { newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{Certificate: invalidCertificateNoPEM}}, oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{Certificate: invalidCertificateNoPEM}}, }, + { + name: "finalizer change with duplicate and unknown usages", + newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: capi.CertificateSigningRequestSpec{ + Usages: []capi.KeyUsage{"unknown", "unknown"}, + Request: newCSRPEM(t), + SignerName: validSignerName, + }}, + oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: capi.CertificateSigningRequestSpec{ + Usages: []capi.KeyUsage{"unknown", "unknown"}, + Request: newCSRPEM(t), + SignerName: validSignerName, + }}, + }, { name: "add Approved condition", newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{