From a3cc31dac8b1698a2d3b4a7351e2a267888c4e69 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Wed, 31 Dec 2025 09:18:31 -0600 Subject: [PATCH] Validate certificate template names (#37765) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix for unreleased bug: - Enforce validation rules for certificate template names (e.g., length, allowed characters). - Implement duplicate name error handling in MySQL datastore. **Related issue:** Resolves #37761 # Checklist for submitter ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results ## Summary by CodeRabbit ## Release Notes * **New Features** * Certificate template naming now enforced with validation rules: no empty or whitespace-only names, character restrictions, and maximum length limits. * Validation occurs on both client and server sides. * **Bug Fixes** * Improved error handling and messaging for duplicate certificate templates. * Corrected certificate validation error message text. ✏️ Tip: You can customize this high-level summary in your review settings. --- .../components/AddCertificateModal/helpers.ts | 2 +- .../datastore/mysql/certificate_templates.go | 3 + server/service/certificate_templates_test.go | 134 ++++++++++++++++++ server/service/certificates.go | 40 +++++- ...tion_android_certificate_templates_test.go | 10 +- 5 files changed, 182 insertions(+), 7 deletions(-) diff --git a/frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts b/frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts index 111270dc96..3d19a2a000 100644 --- a/frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts +++ b/frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts @@ -37,7 +37,7 @@ export const generateFormValidations = ( { name: "required", isValid: (formData: IAddCertFormData) => { - return formData.name.length > 0; + return formData.name.trim().length > 0; }, }, { diff --git a/server/datastore/mysql/certificate_templates.go b/server/datastore/mysql/certificate_templates.go index 8735689010..91e53251b9 100644 --- a/server/datastore/mysql/certificate_templates.go +++ b/server/datastore/mysql/certificate_templates.go @@ -162,6 +162,9 @@ func (ds *Datastore) CreateCertificateTemplate(ctx context.Context, certificateT ) VALUES (?, ?, ?, ?) `, certificateTemplate.Name, certificateTemplate.TeamID, certificateTemplate.CertificateAuthorityID, certificateTemplate.SubjectName) if err != nil { + if IsDuplicate(err) { + return nil, ctxerr.Wrap(ctx, alreadyExists("CertificateTemplate", certificateTemplate.Name), "inserting certificate_template") + } return nil, ctxerr.Wrap(ctx, err, "inserting certificate_template") } diff --git a/server/service/certificate_templates_test.go b/server/service/certificate_templates_test.go index e9447ecbdc..2c1513b51e 100644 --- a/server/service/certificate_templates_test.go +++ b/server/service/certificate_templates_test.go @@ -85,6 +85,88 @@ func TestCreateCertificateTemplate(t *testing.T) { // Check that the error is about invalid CA type require.Contains(t, err.Error(), "not found") }) + + t.Run("Empty or whitespace-only name", func(t *testing.T) { + whitespaceNames := []string{"", " ", " ", "\t", "\n", " \t\n "} + for _, name := range whitespaceNames { + _, err := svc.CreateCertificateTemplate(ctx, name, TeamID, uint(ValidCATypeID), "CN=$FLEET_VAR_HOST_UUID") + require.Error(t, err) + require.Contains(t, err.Error(), "Certificate template name is required") + } + }) + + t.Run("Name too long", func(t *testing.T) { + longName := string(make([]byte, 256)) + for i := range longName { + longName = longName[:i] + "a" + longName[i+1:] + } + _, err := svc.CreateCertificateTemplate(ctx, longName, TeamID, uint(ValidCATypeID), "CN=$FLEET_VAR_HOST_UUID") + require.Error(t, err) + require.Contains(t, err.Error(), "Certificate template name is too long") + }) + + t.Run("Name with invalid characters", func(t *testing.T) { + testCases := []struct { + name string + }{ + {name: "template@name"}, + {name: "template#name"}, + {name: "template$name"}, + {name: "template%name"}, + {name: "template.name"}, + {name: "template/name"}, + {name: "template\\name"}, + {name: "template!name"}, + {name: "template?name"}, + {name: "template*name"}, + {name: "template+name"}, + {name: "template=name"}, + {name: "template"}, + {name: "template(name)"}, + {name: "template[name]"}, + {name: "template{name}"}, + {name: "template|name"}, + {name: "template;name"}, + {name: "template:name"}, + {name: "template'name"}, + {name: "template\"name"}, + {name: "template`name"}, + {name: "template~name"}, + {name: "template^name"}, + {name: "template name"}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, err := svc.CreateCertificateTemplate(ctx, tc.name, TeamID, uint(ValidCATypeID), "CN=$FLEET_VAR_HOST_UUID") + require.Error(t, err) + require.Contains(t, err.Error(), "Invalid certificate template name") + }) + } + }) + + t.Run("Name with valid characters", func(t *testing.T) { + validNames := []string{ + "my template", + " my template ", + "my-template", + "my_template", + "MyTemplate123", + "Template 1", + "UPPERCASE", + "lowercase", + "Mix-Ed_Case 123", + "a", + "1", + "a1", + "1a", + } + for _, name := range validNames { + t.Run(name, func(t *testing.T) { + _, err := svc.CreateCertificateTemplate(ctx, name, TeamID, uint(ValidCATypeID), "CN=$FLEET_VAR_HOST_UUID") + require.NoError(t, err) + }) + } + }) } func TestApplyCertificateTemplateSpecs(t *testing.T) { @@ -236,4 +318,56 @@ func TestApplyCertificateTemplateSpecs(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "not found") }) + + t.Run("Empty name", func(t *testing.T) { + err := svc.ApplyCertificateTemplateSpecs(ctx, []*fleet.CertificateRequestSpec{ + { + Name: "", + CertificateAuthorityId: 1, + SubjectName: "foo", + }, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "Certificate template name is required") + }) + + t.Run("Whitespace-only name", func(t *testing.T) { + err := svc.ApplyCertificateTemplateSpecs(ctx, []*fleet.CertificateRequestSpec{ + { + Name: " ", + CertificateAuthorityId: 1, + SubjectName: "foo", + }, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "Certificate template name is required") + }) + + t.Run("Name with invalid characters", func(t *testing.T) { + err := svc.ApplyCertificateTemplateSpecs(ctx, []*fleet.CertificateRequestSpec{ + { + Name: "template@name", + CertificateAuthorityId: 1, + SubjectName: "foo", + }, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "Invalid certificate template name") + }) + + t.Run("Name too long", func(t *testing.T) { + longName := string(make([]byte, 256)) + for i := range longName { + longName = longName[:i] + "a" + longName[i+1:] + } + err := svc.ApplyCertificateTemplateSpecs(ctx, []*fleet.CertificateRequestSpec{ + { + Name: longName, + CertificateAuthorityId: 1, + SubjectName: "foo", + }, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "Certificate template name is too long") + }) } diff --git a/server/service/certificates.go b/server/service/certificates.go index c500da3247..3ca91998b9 100644 --- a/server/service/certificates.go +++ b/server/service/certificates.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "regexp" + "strings" "github.com/fleetdm/fleet/v4/server/authz" "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" @@ -12,6 +14,32 @@ import ( "github.com/go-kit/kit/log/level" ) +// Certificate template name validation constants +const ( + maxCertificateTemplateNameLength = 255 +) + +// certificateTemplateNameRegex allows only letters, numbers, spaces, dashes, and underscores +var certificateTemplateNameRegex = regexp.MustCompile(`^[a-zA-Z0-9 \-_]+$`) + +// validateCertificateTemplateName validates the certificate template name. +// Returns a BadRequestError if validation fails. +func validateCertificateTemplateName(name string) error { + if strings.TrimSpace(name) == "" { + return &fleet.BadRequestError{Message: "Certificate template name is required."} + } + + if len(name) > maxCertificateTemplateNameLength { + return &fleet.BadRequestError{Message: fmt.Sprintf("Certificate template name is too long. Maximum is %d characters.", maxCertificateTemplateNameLength)} + } + + if !certificateTemplateNameRegex.MatchString(name) { + return &fleet.BadRequestError{Message: "Invalid certificate template name. Only letters, numbers, spaces, dashes, and underscores are allowed."} + } + + return nil +} + type createCertificateTemplateRequest struct { Name string `json:"name"` TeamID uint `json:"team_id"` // If not provided, intentionally defaults to 0 aka "No team" @@ -47,6 +75,11 @@ func (svc *Service) CreateCertificateTemplate(ctx context.Context, name string, return nil, err } + // Validate certificate template name + if err := validateCertificateTemplateName(name); err != nil { + return nil, err + } + if err := validateCertificateTemplateFleetVariables(subjectName); err != nil { return nil, &fleet.BadRequestError{Message: err.Error()} } @@ -387,6 +420,11 @@ func (svc *Service) ApplyCertificateTemplateSpecs(ctx context.Context, specs []* var certificates []*fleet.CertificateTemplate for _, spec := range specs { + // Validate certificate template name + if err := validateCertificateTemplateName(spec.Name); err != nil { + return err + } + // Get the CA to validate its existence and type. ca, ok := casByID[spec.CertificateAuthorityId] if !ok { @@ -394,7 +432,7 @@ func (svc *Service) ApplyCertificateTemplateSpecs(ctx context.Context, specs []* } if ca.Type != string(fleet.CATypeCustomSCEPProxy) { - return &fleet.BadRequestError{Message: fmt.Sprintf("Ccertificate `%s`: Currently, only the custom_scep_proxy certificate authority is supported.", spec.Name)} + return &fleet.BadRequestError{Message: fmt.Sprintf("Certificate `%s`: Currently, only the custom_scep_proxy certificate authority is supported.", spec.Name)} } // Validate Fleet variables in subject name diff --git a/server/service/integration_android_certificate_templates_test.go b/server/service/integration_android_certificate_templates_test.go index 626f1df690..5f668de650 100644 --- a/server/service/integration_android_certificate_templates_test.go +++ b/server/service/integration_android_certificate_templates_test.go @@ -186,7 +186,7 @@ func (s *integrationMDMTestSuite) TestCertificateTemplateLifecycle() { }) // Step: Create a certificate template - certTemplateName := t.Name() + "-CertTemplate" + certTemplateName := strings.ReplaceAll(t.Name(), "/", "-") + "-CertTemplate" var createResp createCertificateTemplateResponse s.DoJSON("POST", "/api/latest/fleet/certificates", createCertificateTemplateRequest{ Name: certTemplateName, @@ -307,7 +307,7 @@ func (s *integrationMDMTestSuite) TestCertificateTemplateSpecEndpointAndAMAPIFai caID := ca.ID // Step: Create certificate template via spec/certificates endpoint with $FLEET_VAR_HOST_UUID - certTemplateName := t.Name() + "-CertTemplate" + certTemplateName := strings.ReplaceAll(t.Name(), "/", "-") + "-CertTemplate" var applyResp applyCertificateTemplateSpecsResponse s.DoJSON("POST", "/api/latest/fleet/spec/certificates", applyCertificateTemplateSpecsRequest{ Specs: []*fleet.CertificateRequestSpec{ @@ -429,7 +429,7 @@ func (s *integrationMDMTestSuite) TestCertificateTemplateNoTeamWithIDPVariable() caID := ca.ID // Step: Create certificate template for "no team" (team_id = 0) with IDP_USERNAME variable - certTemplateName := t.Name() + "-CertTemplate" + certTemplateName := strings.ReplaceAll(t.Name(), "/", "-") + "-CertTemplate" var createResp createCertificateTemplateResponse subjectName := "CN=$FLEET_VAR_HOST_END_USER_IDP_USERNAME" s.DoJSON("POST", "/api/latest/fleet/certificates", createCertificateTemplateRequest{ @@ -586,7 +586,7 @@ func (s *integrationMDMTestSuite) TestCertificateTemplateUnenrollReenroll() { require.NoError(t, s.ds.UpdateHost(ctx, host)) // Step: Create the first certificate template (while host is enrolled) - certTemplateName := t.Name() + "-CertTemplate1" + certTemplateName := strings.ReplaceAll(t.Name(), "/", "-") + "-CertTemplate1" var createResp createCertificateTemplateResponse s.DoJSON("POST", "/api/latest/fleet/certificates", createCertificateTemplateRequest{ Name: certTemplateName, @@ -614,7 +614,7 @@ func (s *integrationMDMTestSuite) TestCertificateTemplateUnenrollReenroll() { require.Equal(t, 0, enrolledStatus, "Host should be marked as unenrolled in host_mdm") // Step: Create a second certificate template while host is unenrolled - certTemplateName2 := t.Name() + "-CertTemplate2" + certTemplateName2 := strings.ReplaceAll(t.Name(), "/", "-") + "-CertTemplate2" s.DoJSON("POST", "/api/latest/fleet/certificates", createCertificateTemplateRequest{ Name: certTemplateName2, TeamID: teamID,