From 44c0fe8b393808899001198fa9a3d17d7ef89c9c Mon Sep 17 00:00:00 2001 From: Jordan Montgomery Date: Tue, 23 Sep 2025 13:29:36 -0400 Subject: [PATCH] Update duplicate CA names logic (#33349) **Related issue:** Resolves #33351 Resolves an unreleased bug with Gitops validation of CA names. Previously only gitops path was validating that a CA didn't have the same name as another CA(I.e. Hydrant didn't have same name as digicert) whereas correct validation per PM is only within a given type of CA, i.e. can't have 2 hydrant with same name. Will also need cherrypick to 4.74. No changes file sinec this is an unreleased bug in the overall Hydrant CA story. # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [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 - [x] Alerted the release DRI if additional load testing is needed --- ee/server/service/certificate_authorities.go | 55 +++++-------------- ...ntegration_certificate_authorities_test.go | 40 ++++---------- 2 files changed, 24 insertions(+), 71 deletions(-) diff --git a/ee/server/service/certificate_authorities.go b/ee/server/service/certificate_authorities.go index 53080d0e54..317e4e860d 100644 --- a/ee/server/service/certificate_authorities.go +++ b/ee/server/service/certificate_authorities.go @@ -439,26 +439,15 @@ func (svc *Service) getCertificateAuthoritiesBatchOperations(ctx context.Context return nil, err } - // collect existing CA names for duplicate name checking - existingNames := make(map[string]string) - for _, ca := range existing.DigiCert { - existingNames[ca.Name] = "digicert" - } - for _, ca := range existing.CustomScepProxy { - existingNames[ca.Name] = "custom_scep_proxy" - } - for _, ca := range existing.Hydrant { - existingNames[ca.Name] = "hydrant" - } - - // collect all CA names for duplicate name checking across both incoming and existing - allNames := make(map[string]struct{}) - // check for duplicate names across all CA types - checkAllNames := func(name, caType string) error { - if _, ok := allNames[name]; ok { - return fmtDuplicateCANameError(name, caType) + // track processed CA names for duplicate name checking + allNames := make(map[string][]string) + checkAllNames := func(name, caType, displayCAType string) error { + for i := 0; i < len(allNames[name]); i++ { + if allNames[name][i] == caType { + return fmtDuplicateCANameError(name, caType, displayCAType) + } } - allNames[name] = struct{}{} + allNames[name] = append(allNames[name], caType) return nil } @@ -470,13 +459,7 @@ func (svc *Service) getCertificateAuthoritiesBatchOperations(ctx context.Context ca.Name = fleet.Preprocess(ca.Name) ca.URL = fleet.Preprocess(ca.URL) ca.ProfileID = fleet.Preprocess(ca.ProfileID) - // check against existing names, excluding self if updating - if existing, ok := existingNames[ca.Name]; ok { - if existing != "digicert" { - return nil, fmtDuplicateCANameError(ca.Name, "digicert") - } - } - if err := checkAllNames(ca.Name, "digicert"); err != nil { + if err := checkAllNames(ca.Name, "digicert", "DigiCert"); err != nil { return nil, err } } @@ -486,13 +469,7 @@ func (svc *Service) getCertificateAuthoritiesBatchOperations(ctx context.Context return nil, fleet.NewInvalidArgumentError("name", "certificate_authorities.custom_scep_proxy: CA name cannot be empty.") } ca.Name = fleet.Preprocess(ca.Name) - // check against existing names, excluding self if updating - if existing, ok := existingNames[ca.Name]; ok { - if existing != "custom_scep_proxy" { - return nil, fmtDuplicateCANameError(ca.Name, "custom_scep_proxy") - } - } - if err := checkAllNames(ca.Name, "custom_scep_proxy"); err != nil { + if err := checkAllNames(ca.Name, "custom_scep_proxy", "Custom SCEP Proxy"); err != nil { return nil, err } } @@ -503,13 +480,7 @@ func (svc *Service) getCertificateAuthoritiesBatchOperations(ctx context.Context } ca.Name = fleet.Preprocess(ca.Name) ca.URL = fleet.Preprocess(ca.URL) - // check against existing names, excluding self if updating - if existing, ok := existingNames[ca.Name]; ok { - if existing != "hydrant" { - return nil, fmtDuplicateCANameError(ca.Name, "hydrant") - } - } - if err := checkAllNames(ca.Name, "hydrant"); err != nil { + if err := checkAllNames(ca.Name, "hydrant", "Hydrant"); err != nil { return nil, err } } @@ -1175,7 +1146,7 @@ func (svc *Service) validateCustomSCEPProxyUpdate(ctx context.Context, customSCE return nil } -func fmtDuplicateCANameError(name, caType string) error { +func fmtDuplicateCANameError(name, caType, displayCAType string) error { return fleet.NewInvalidArgumentError("name", fmt.Sprintf("certificate_authorities.%s.name: Couldn’t edit certificate authority. "+ - "\"%s\" name is already used by another certificate authority. Please choose a different name and try again.", caType, name)) + "\"%s\" name is already used by another %s certificate authority. Please choose a different name and try again.", caType, name, displayCAType)) } diff --git a/server/service/integration_certificate_authorities_test.go b/server/service/integration_certificate_authorities_test.go index ec80c738b6..a423d220ab 100644 --- a/server/service/integration_certificate_authorities_test.go +++ b/server/service/integration_certificate_authorities_test.go @@ -433,7 +433,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { res := s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) require.Contains(t, errMsg, "certificate_authorities.digicert") - require.Contains(t, errMsg, "name is already used by another certificate authority") + require.Contains(t, errMsg, "name is already used by another DigiCert certificate authority") // try to create digicert with same name as another custom scep testCopy = goodDigiCertCA @@ -446,10 +446,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.digicert") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) // try to create digicert with same name as another hydrant testCopy = goodDigiCertCA @@ -462,10 +459,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.digicert") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) }) t.Run("digicert more than 1 user principal name", func(t *testing.T) { @@ -765,9 +759,9 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { res := s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) require.Contains(t, errMsg, "certificate_authorities.custom_scep_proxy") - require.Contains(t, errMsg, "name is already used by another certificate authority") + require.Contains(t, errMsg, "name is already used by another Custom SCEP Proxy certificate authority") - // try to create custom scep with same name as another digicert + // try to create custom scep with same name as the digicert. Should not error testCopy = goodCustomSCEPCA testCopy.Name = goodDigiCertCA.Name duplicateReq = batchApplyCertificateAuthoritiesRequest{ @@ -778,12 +772,9 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.custom_scep_proxy") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) - // try to create custom scep with same name as another hydrant + // try to create custom scep with same name as the Hydrant CA. Should not error testCopy = goodCustomSCEPCA testCopy.Name = goodHydrantCA.Name duplicateReq = batchApplyCertificateAuthoritiesRequest{ @@ -794,10 +785,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.custom_scep_proxy") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) }) t.Run("custom_scep challenge not set", func(t *testing.T) { @@ -974,7 +962,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { res := s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) errMsg := extractServerErrorText(res.Body) require.Contains(t, errMsg, "certificate_authorities.hydrant") - require.Contains(t, errMsg, "name is already used by another certificate authority") + require.Contains(t, errMsg, "name is already used by another Hydrant certificate authority") // try to create hydrant with same name as another digicert testCopy = goodHydrantCA @@ -987,10 +975,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.hydrant") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) // try to create hydrant with same name as another custom scep testCopy = goodHydrantCA @@ -1003,10 +988,7 @@ func (s *integrationMDMTestSuite) TestBatchApplyCertificateAuthorities() { }, DryRun: false, } - res = s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusUnprocessableEntity) - errMsg = extractServerErrorText(res.Body) - require.Contains(t, errMsg, "certificate_authorities.hydrant") - require.Contains(t, errMsg, "name is already used by another certificate authority") + s.Do("POST", "/api/v1/fleet/spec/certificate_authorities", duplicateReq, http.StatusOK) }) // TODO(hca): hydrant happy path and other specific tests