Android certificate crud: validate variable replacement (#36648)

<!-- Add the related story/sub-task/bug number, like Resolves #123, or
remove if NA -->
**Related issue:** Resolves #36533 

If variables can't be interpolated return 400.

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

- [X] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files)
for more information.

## Testing

- [X] Added/updated automated tests

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Corrected HTTP status code returned when certificate template variable
interpolation fails
* Certificate delivery status now properly reflects failed
interpolation, improving visibility into deployment issues

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com>

changes/36533-use-proper-status-code-on-failure-to-interpolate

@@ -0,0 +1 @@
+* Fixed incorrect status code on failure to interpolate certificate template variables.

server/datastore/mysql/host_certificate_templates.go

@@ -2,6 +2,8 @@ package mysql
 
 import (
 	"context"
+	"database/sql"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -163,24 +165,29 @@ func (ds *Datastore) DeleteHostCertificateTemplates(ctx context.Context, hostCer
 	return nil
 }
 
-func (ds *Datastore) UpdateCertificateStatus(
+func (ds *Datastore) UpsertCertificateStatus(
 	ctx context.Context,
 	hostUUID string,
 	certificateTemplateID uint,
 	status fleet.MDMDeliveryStatus,
 	detail *string,
 ) error {
+	updateStmt := `
+    UPDATE host_certificate_templates
+    SET status = ?, detail = ?
+    WHERE host_uuid = ? AND certificate_template_id = ?`
+
+	insertStmt := `
+		INSERT INTO host_certificate_templates (host_uuid, certificate_template_id, status, detail, fleet_challenge)
+		VALUES (?, ?, ?, ?, ?)`
+
 	// Validate the status.
 	if !status.IsValid() {
 		return ctxerr.Wrap(ctx, fmt.Errorf("Invalid status '%s'", string(status)))
 	}
 
 	// Attempt to update the certificate status for the given host and template.
-	result, err := ds.writer(ctx).ExecContext(ctx, `
-    UPDATE host_certificate_templates
-    SET status = ?, detail = ?
-    WHERE host_uuid = ? AND certificate_template_id = ?
-`, status, detail, hostUUID, certificateTemplateID)
+	result, err := ds.writer(ctx).ExecContext(ctx, updateStmt, status, detail, hostUUID, certificateTemplateID)
 	if err != nil {
 		return err
 	}
@@ -190,8 +197,24 @@ func (ds *Datastore) UpdateCertificateStatus(
 		return err
 	}
 
+	// If no records were updated, then insert a new status.
 	if rowsAffected == 0 {
-		return ctxerr.Wrap(ctx, notFound("Label").WithMessage(fmt.Sprintf("No certificate found for host UUID '%s' and template ID '%d'", hostUUID, certificateTemplateID)))
+		// We need to check whether the certificate template exists ... we do this way because
+		// there are no FK constraints between host_certificate_templates and certificate_templates.
+		var result uint
+		err := ds.writer(ctx).GetContext(ctx, &result, `SELECT id FROM certificate_templates WHERE id = ?`, certificateTemplateID)
+		if err != nil {
+			if errors.Is(err, sql.ErrNoRows) {
+				return ctxerr.Wrap(ctx, notFound("CertificateTemplate").WithMessage(fmt.Sprintf("No certificate template found for template ID '%d'",
+					certificateTemplateID)))
+			}
+			return ctxerr.Wrap(ctx, err, "could not read certificate template for inserting new record")
+		}
+
+		params := []any{hostUUID, certificateTemplateID, status, detail, ""}
+		if _, err := ds.writer(ctx).ExecContext(ctx, insertStmt, params...); err != nil {
+			return ctxerr.Wrap(ctx, err, "could not insert new host certificate template")
+		}
 	}
 
 	return nil

server/datastore/mysql/host_certificate_templates_test.go

@@ -22,7 +22,7 @@ func TestHostCertificateTemplates(t *testing.T) {
 		{"ListAndroidHostUUIDsWithDeliverableCertificateTemplates", testListAndroidHostUUIDsWithDeliverableCertificateTemplates},
 		{"ListCertificateTemplatesForHosts", testListCertificateTemplatesForHosts},
 		{"BulkInsertAndDeleteHostCertificateTemplates", testBulkInsertAndDeleteHostCertificateTemplates},
-		{"UpdateHostCertificateTemplateStatus", testUpdateHostCertificateTemplateStatus},
+		{"UpsertHostCertificateTemplateStatus", testUpsertHostCertificateTemplateStatus},
 	}
 
 	for _, c := range cases {
@@ -612,7 +612,7 @@ func testBulkInsertAndDeleteHostCertificateTemplates(t *testing.T, ds *Datastore
 	}
 }
 
-func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) {
+func testUpsertHostCertificateTemplateStatus(t *testing.T, ds *Datastore) {
 	nodeKey := uuid.New().String()
 	uuid := uuid.New().String()
 	hostName := "test-update-host-certificate-template"
@@ -634,15 +634,23 @@ func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) {
 	require.NoError(t, err)
 	caID := ca.ID
 
-	certTemplate := &fleet.CertificateTemplate{
+	ct1, err := ds.CreateCertificateTemplate(ctx, &fleet.CertificateTemplate{
 		Name:                   "Cert1",
 		TeamID:                 teamID,
 		CertificateAuthorityID: caID,
 		SubjectName:            "CN=Test Subject 1",
-	}
-	savedTemplate, err := ds.CreateCertificateTemplate(ctx, certTemplate)
+	})
 	require.NoError(t, err)
-	require.NotNil(t, savedTemplate)
+	require.NotNil(t, ct1)
+
+	ct2, err := ds.CreateCertificateTemplate(ctx, &fleet.CertificateTemplate{
+		Name:                   "Cert2",
+		TeamID:                 teamID,
+		CertificateAuthorityID: caID,
+		SubjectName:            "CN=Test Subject 1",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, ct2)
 
 	// Create a host
 	host, err := ds.NewHost(context.Background(), &fleet.Host{
@@ -654,9 +662,6 @@ func testUpdateHostCertificateTemplateStatus(t *testing.T, ds *Datastore) {
 	})
 	require.NoError(t, err)
 
-	// TODO -- add a host certificate template when we have a foreign key set up.
-	certificateTemplateID := savedTemplate.ID
-
 	// Create a record in host_certificate_templates using ad hoc SQL
 	sql := `
 INSERT INTO host_certificate_templates (
@@ -667,7 +672,7 @@ INSERT INTO host_certificate_templates (
 ) VALUES (?, ?, ?, ?);
 	`
 	ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error {
-		_, err = q.ExecContext(context.Background(), sql, host.UUID, certificateTemplateID, "pending", "some_challenge_value")
+		_, err = q.ExecContext(context.Background(), sql, host.UUID, ct1.ID, "pending", "some_challenge_value")
 		require.NoError(t, err)
 		return nil
 	})
@@ -681,35 +686,33 @@ INSERT INTO host_certificate_templates (
 		detail           *string
 	}{
 		{
-			name:             "Valid Update",
-			templateID:       certificateTemplateID,
-			newStatus:        "verified",
-			expectedErrorMsg: "",
+			name:       "Valid Update",
+			templateID: ct1.ID,
+			newStatus:  "verified",
 		},
 		{
-			name:             "Valid Update with some details",
-			templateID:       certificateTemplateID,
-			newStatus:        "failed",
-			detail:           ptr.String("some details"),
-			expectedErrorMsg: "",
+			name:       "Valid Update with some details",
+			templateID: ct1.ID,
+			newStatus:  "failed",
+			detail:     ptr.String("some details"),
 		},
 		{
 			name:             "Invalid Status",
-			templateID:       certificateTemplateID,
+			templateID:       ct1.ID,
 			newStatus:        "invalid_status",
 			expectedErrorMsg: fmt.Sprintf("Invalid status '%s'", "invalid_status"),
 		},
 		{
-			name:             "Wrong Template ID",
-			templateID:       9999,
-			newStatus:        "verified",
-			expectedErrorMsg: fmt.Sprintf("No certificate found for host UUID '%s' and template ID '%d'", host.UUID, 9999),
+			name:       "Creates a new status if record does not exist",
+			templateID: ct2.ID,
+			newStatus:  "verified",
+			detail:     ptr.String("some details"),
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(fmt.Sprintf("TestUpdateHostCertificateTemplate:%s", tc.name), func(t *testing.T) {
-			err := ds.UpdateCertificateStatus(context.Background(), host.UUID, tc.templateID, fleet.MDMDeliveryStatus(tc.newStatus), tc.detail)
+			err := ds.UpsertCertificateStatus(context.Background(), host.UUID, tc.templateID, fleet.MDMDeliveryStatus(tc.newStatus), tc.detail)
 			if tc.expectedErrorMsg == "" {
 				require.NoError(t, err)
 				// Verify the update

server/fleet/datastore.go

@@ -2513,7 +2513,7 @@ type Datastore interface {
 	// processed together as upserts using INSERT...ON DUPLICATE KEY UPDATE.
 	BatchApplyCertificateAuthorities(ctx context.Context, ops CertificateAuthoritiesBatchOperations) error
 	// UpdateCertificateStatus allows a host to update the installation status of a certificate given its template.
-	UpdateCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status MDMDeliveryStatus, detail *string) error
+	UpsertCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status MDMDeliveryStatus, detail *string) error
 
 	// BatchUpsertCertificateTemplates upserts a batch of certificates.
 	BatchUpsertCertificateTemplates(ctx context.Context, certificates []*CertificateTemplate) error

server/mock/datastore_mock.go

@@ -1637,7 +1637,7 @@ type UpdateCertificateAuthorityByIDFunc func(ctx context.Context, id uint, certi
 
 type BatchApplyCertificateAuthoritiesFunc func(ctx context.Context, ops fleet.CertificateAuthoritiesBatchOperations) error
 
-type UpdateCertificateStatusFunc func(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus, detail *string) error
+type UpsertCertificateStatusFunc func(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus, detail *string) error
 
 type BatchUpsertCertificateTemplatesFunc func(ctx context.Context, certificates []*fleet.CertificateTemplate) error
 
@@ -4087,8 +4087,8 @@ type DataStore struct {
 	BatchApplyCertificateAuthoritiesFunc        BatchApplyCertificateAuthoritiesFunc
 	BatchApplyCertificateAuthoritiesFuncInvoked bool
 
-	UpdateCertificateStatusFunc        UpdateCertificateStatusFunc
-	UpdateCertificateStatusFuncInvoked bool
+	UpsertCertificateStatusFunc        UpsertCertificateStatusFunc
+	UpsertCertificateStatusFuncInvoked bool
 
 	BatchUpsertCertificateTemplatesFunc        BatchUpsertCertificateTemplatesFunc
 	BatchUpsertCertificateTemplatesFuncInvoked bool
@@ -9781,11 +9781,11 @@ func (s *DataStore) BatchApplyCertificateAuthorities(ctx context.Context, ops fl
 	return s.BatchApplyCertificateAuthoritiesFunc(ctx, ops)
 }
 
-func (s *DataStore) UpdateCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus, detail *string) error {
+func (s *DataStore) UpsertCertificateStatus(ctx context.Context, hostUUID string, certificateTemplateID uint, status fleet.MDMDeliveryStatus, detail *string) error {
 	s.mu.Lock()
-	s.UpdateCertificateStatusFuncInvoked = true
+	s.UpsertCertificateStatusFuncInvoked = true
 	s.mu.Unlock()
-	return s.UpdateCertificateStatusFunc(ctx, hostUUID, certificateTemplateID, status, detail)
+	return s.UpsertCertificateStatusFunc(ctx, hostUUID, certificateTemplateID, status, detail)
 }
 
 func (s *DataStore) BatchUpsertCertificateTemplates(ctx context.Context, certificates []*fleet.CertificateTemplate) error {

server/service/certificate_templates.go

@@ -43,8 +43,14 @@ func (svc *Service) replaceCertificateVariables(ctx context.Context, subjectName
 	for _, fleetVar := range fleetVars {
 		switch fleetVar {
 		case string(fleet.FleetVarHostUUID):
+			if host.UUID == "" {
+				return "", ctxerr.Errorf(ctx, "host does not have a UUID for variable %s", fleetVar)
+			}
 			result = fleet.FleetVarHostUUIDRegexp.ReplaceAllString(result, host.UUID)
 		case string(fleet.FleetVarHostHardwareSerial):
+			if host.HardwareSerial == "" {
+				return "", ctxerr.Errorf(ctx, "host %s does not have a hardware serial for variable %s", host.UUID, fleetVar)
+			}
 			result = fleet.FleetVarHostHardwareSerialRegexp.ReplaceAllString(result, host.HardwareSerial)
 		case string(fleet.FleetVarHostEndUserIDPUsername):
 			users, err := fleet.GetEndUsers(ctx, svc.ds, host.ID)
@@ -55,6 +61,8 @@ func (svc *Service) replaceCertificateVariables(ctx context.Context, subjectName
 				return "", ctxerr.Errorf(ctx, "host %s does not have an IDP username for variable %s", host.UUID, fleetVar)
 			}
 			result = fleet.FleetVarHostEndUserIDPUsernameRegexp.ReplaceAllString(result, users[0].IdpUserName)
+		default:
+			return "", ctxerr.Errorf(ctx, "unsupported Fleet variable %s in certificate template", fleetVar)
 		}
 	}
 

server/service/certificates.go

@@ -170,7 +170,19 @@ func (svc *Service) GetDeviceCertificateTemplate(ctx context.Context, id uint) (
 
 	subjectName, err := svc.replaceCertificateVariables(ctx, certificate.SubjectName, host)
 	if err != nil {
-		return nil, ctxerr.Wrap(ctx, err, "replacing certificate variables")
+		// If the certificate variables cannot be replaced, mark the certificate as failed.
+		errorMsg := fmt.Sprintf("Could not replace certificate variables: %s", err.Error())
+		if err := svc.ds.UpsertCertificateStatus(
+			ctx,
+			host.UUID,
+			certificate.ID,
+			fleet.MDMDeliveryFailed,
+			&errorMsg,
+		); err != nil {
+			return nil, err
+		}
+		certificate.Status = &fleet.MDMDeliveryFailed
+		return certificate, nil
 	}
 	certificate.SubjectName = subjectName
 
@@ -425,5 +437,5 @@ func (svc *Service) UpdateCertificateStatus(
 		return fleet.NewInvalidArgumentError("status", string(status))
 	}
 
-	return svc.ds.UpdateCertificateStatus(ctx, host.UUID, certificateTemplateID, status, detail)
+	return svc.ds.UpsertCertificateStatus(ctx, host.UUID, certificateTemplateID, status, detail)
 }

server/service/integration_core_test.go

@@ -8031,6 +8031,19 @@ func (s *integrationTestSuite) TestCertificatesSpecs() {
 	host.OrbitNodeKey = &orbitNodeKey
 	require.NoError(t, s.ds.UpdateHost(ctx, host))
 
+	savedCertificateTemplates, _, err := s.ds.GetCertificateTemplatesByTeamID(ctx, team.ID, fleet.ListOptions{Page: 0, PerPage: 10})
+	require.NoError(t, err)
+	certID := savedCertificateTemplates[0].ID
+
+	var getCertResp getDeviceCertificateTemplateResponse
+
+	resp := s.DoRawWithHeaders("GET", fmt.Sprintf("/api/fleetd/certificates/%d", certID), nil, http.StatusOK, map[string]string{
+		"Authorization": fmt.Sprintf("Node key %s", orbitNodeKey),
+	})
+	require.NoError(t, json.NewDecoder(resp.Body).Decode(&getCertResp))
+	require.NoError(t, resp.Body.Close())
+	require.Equal(t, *getCertResp.Certificate.Status, fleet.MDMDeliveryFailed)
+
 	// Add an IDP user for the host
 	err = s.ds.ReplaceHostDeviceMapping(ctx, host.ID, []*fleet.HostDeviceMapping{
 		{
@@ -8041,14 +8054,8 @@ func (s *integrationTestSuite) TestCertificatesSpecs() {
 	}, fleet.DeviceMappingMDMIdpAccounts)
 	require.NoError(t, err)
 
-	savedCertificateTemplates, _, err := s.ds.GetCertificateTemplatesByTeamID(ctx, team.ID, fleet.ListOptions{Page: 0, PerPage: 10})
-	require.NoError(t, err)
-	certID := savedCertificateTemplates[0].ID
-
 	// Get certificate without node_key
-	var getCertResp getDeviceCertificateTemplateResponse
-
-	resp := s.DoRawWithHeaders("GET", fmt.Sprintf("/api/fleetd/certificates/%d", certID), nil, http.StatusUnauthorized, nil)
+	resp = s.DoRawWithHeaders("GET", fmt.Sprintf("/api/fleetd/certificates/%d", certID), nil, http.StatusUnauthorized, nil)
 	require.NoError(t, resp.Body.Close())
 
 	// Get certificate with node_key (should return replaced variables)