ACME: migration to add hardware_serial to DEP assignments by mna · Pull Request #42833 · fleetdm/fleet

mna · 2026-04-01T18:20:41Z

Related issue: Resolves #41491

Checklist for submitter

Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.

Testing

Added/updated automated tests

Database migrations

Checked schema for all modified table for columns that will auto-update timestamps during migration.

This should not update any existing timestamp in that table, per their definition:

  `added_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, <- only set at creation time if null
  `deleted_at` timestamp NULL DEFAULT NULL, <- not set automatically
  `response_updated_at` timestamp NULL DEFAULT NULL, <- not set automatically
  `mdm_migration_deadline` timestamp(6) NULL DEFAULT NULL, <- not set automatically
  `mdm_migration_completed` timestamp(6) NULL DEFAULT NULL, <- not set automatically

Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

mna · 2026-04-01T18:41:01Z

server/mdm/acme/internal/service/account_order.go

 	}
+	if !strings.HasSuffix(certPEM, "\n") {
+		certPEM += "\n"
+	}


Fix for a Claude feedback on my latest get certificate PR.

mna · 2026-04-01T18:55:18Z

server/datastore/mysql/mdm.go

 func (ds *Datastore) GetDeviceInfoForACMERenewal(ctx context.Context, hostUUIDs []string) ([]fleet.DeviceInfoForACMERenewal, error) {
+	// TODO(mna): anyone knows what those TODOs (from Sarah's PRs) were for?


It's about host_dep_assignments, but not about the new hardware_serial field, so I 'm not sure if there's anything to change here.

I think the suggestion was we c ould put the model information here as well? I think it is OK not to for now

mna · 2026-04-01T19:16:12Z

server/datastore/mysql/apple_mdm.go


 	stmt := `
-		INSERT INTO host_dep_assignments (host_id, abm_token_id, mdm_migration_deadline)
+		INSERT INTO host_dep_assignments (host_id, abm_token_id, mdm_migration_deadline, hardware_serial)


Note that while I've added it to the insertion here, I haven't modified the fleet.HostDEPAssignment struct to add that field (nor to load it when it is read). Wasn't clear to me if that was needed and I wanted to change as little as necessary to avoid impacting more than ACME.

It doesn't seem needed at the moment so I think that is OK

codecov · 2026-04-01T19:32:54Z

Codecov Report

❌ Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (feature-ACME@07e788b). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...401153503_AddHardwareSerialToHostDEPAssignments.go	71.42%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##             feature-ACME   #42833   +/-   ##
===============================================
  Coverage                ?   66.50%           
===============================================
  Files                   ?     2536           
  Lines                   ?   202687           
  Branches                ?     9047           
===============================================
  Hits                    ?   134797           
  Misses                  ?    55654           
  Partials                ?    12236

Flag	Coverage Δ
backend	`68.28% <86.04%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

mna · 2026-04-01T21:10:41Z

@claude review once

claude · 2026-04-01T21:20:47Z

...er/datastore/mysql/migrations/tables/20260401153503_AddHardwareSerialToHostDEPAssignments.go

+func Up_20260401153503(tx *sql.Tx) error {
+	_, err := tx.Exec(`
+		ALTER TABLE host_dep_assignments
+			ADD COLUMN hardware_serial varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT ''
+	`)
+	if err != nil {
+		return errors.Wrap(err, "add host_dep_assignments.hardware_serial column")
+	}
+
+	_, err = tx.Exec(`
+		UPDATE host_dep_assignments hda
+		JOIN hosts h ON h.id = hda.host_id
+		SET hda.hardware_serial = h.hardware_serial
+		WHERE hda.deleted_at IS NULL
+	`)
+	if err != nil {
+		return errors.Wrap(err, "backfill host_dep_assignments.hardware_serial column")
+	}


🔴 The migration adds hardware_serial to host_dep_assignments but omits an index on the column, causing GetHostDEPAssignmentsBySerial to perform a full table scan on every ACME certificate lookup. Add ALTER TABLE host_dep_assignments ADD INDEX idx_hdep_hardware_serial (hardware_serial) to the migration to restore query performance.

Extended reasoning...

What the bug is and how it manifests

The migration Up_20260401153503 adds a hardware_serial VARCHAR(255) column to host_dep_assignments and backfills it, but never creates an index on the new column. The PR simultaneously changes GetHostDEPAssignmentsBySerial to query directly on hdep.hardware_serial = ? instead of using the previous subselect approach.

The specific code path that triggers it

In apple_mdm.go, GetHostDEPAssignmentsBySerial now executes:

SELECT host_id, added_at, deleted_at, abm_token_id, mdm_migration_deadline, mdm_migration_completed FROM host_dep_assignments hdep WHERE hdep.hardware_serial = ? AND hdep.deleted_at IS NULL

This function is called in hot paths: ACME device attestation challenges and enrollment profile generation. Both are invoked for every ACME certificate issuance and renewal request.

Why existing code does not prevent it

The schema.sql diff confirms the table indexes after this migration are:

PRIMARY KEY (host_id)

KEY idx_hdep_response (assign_profile_response, response_updated_at)

KEY fk_host_dep_assignments_abm_token_id (abm_token_id)

There is no index on hardware_serial. The previous query used WHERE hdep.host_id IN (SELECT id FROM hosts WHERE hardware_serial = ?), which leveraged the idx_hosts_hardware_serial index on the hosts table plus a primary-key lookup on host_dep_assignments. The new approach eliminates both of those optimizations.

What the impact would be

For large enterprise deployments with tens or hundreds of thousands of DEP-enrolled devices, every ACME certificate lookup/renewal performs a full table scan of host_dep_assignments. This is a direct performance regression on a critical, frequently-invoked code path. The established pattern in this codebase is to index hardware_serial wherever it is queried (the hosts table has idx_hosts_hardware_serial for this reason).

How to fix it

Add a third ALTER TABLE statement to Up_20260401153503:

_, err = tx.Exec("ALTER TABLE host_dep_assignments ADD INDEX idx_hdep_hardware_serial (hardware_serial)") if err != nil { return errors.Wrap(err, "add index on host_dep_assignments.hardware_serial") }

Also update schema.sql to include the new key definition.

Step-by-step proof

Before this PR: GetHostDEPAssignmentsBySerial("ABC123") runs WHERE hdep.host_id IN (SELECT id FROM hosts WHERE hardware_serial = ?). MySQL uses idx_hosts_hardware_serial to find the host id (index seek, ~1 row), then does a primary-key lookup on host_dep_assignments -- effectively O(1).

This PR: the migration adds hardware_serial to host_dep_assignments with no index. The query becomes WHERE hdep.hardware_serial = ?. With no index on this column, MySQL must scan every row in host_dep_assignments to find matches.

On a deployment with 100,000 DEP-enrolled devices, each ACME certificate lookup now reads ~100,000 rows instead of ~1 row. With frequent certificate renewals, this quickly becomes a bottleneck.

EXPLAIN for the new query would show type: ALL (full table scan) on host_dep_assignments; with the index added it would show type: ref using idx_hdep_hardware_serial.

Added an index

**Related issue:** Resolves #41491 - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Added/updated automated tests - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. This should not update any existing timestamp in that table, per their definition: ``` `added_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP, <- only set at creation time if null `deleted_at` timestamp NULL DEFAULT NULL, <- not set automatically `response_updated_at` timestamp NULL DEFAULT NULL, <- not set automatically `mdm_migration_deadline` timestamp(6) NULL DEFAULT NULL, <- not set automatically `mdm_migration_completed` timestamp(6) NULL DEFAULT NULL, <- not set automatically ``` - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). --------- Co-authored-by: Jordan Montgomery <elijah.jordan.montgomery@gmail.com>

mna added 3 commits April 1, 2026 11:48

Create migration file

6198f52

Ensure no duplicate newlines in get certificate response

984c213

FInish up the migration

28dce0e

mna temporarily deployed to Docker Hub April 1, 2026 18:20 — with GitHub Actions Inactive

Add migration tests

3a15c07

mna had a problem deploying to Docker Hub April 1, 2026 18:35 — with GitHub Actions Error

Regen schema

e3845bc

mna temporarily deployed to Docker Hub April 1, 2026 18:40 — with GitHub Actions Inactive

mna commented Apr 1, 2026

View reviewed changes

Update other hdep code with serial

5eff308

mna temporarily deployed to Docker Hub April 1, 2026 18:53 — with GitHub Actions Inactive

mna commented Apr 1, 2026

View reviewed changes

mna added 2 commits April 1, 2026 15:02

Try to silence the nilaway false positive

ac72491

Fix other inserts in tests and some issues

ad8e9dd

mna temporarily deployed to Docker Hub April 1, 2026 19:11 — with GitHub Actions Inactive

mna commented Apr 1, 2026

View reviewed changes

Possibly fix the ambiguous hardware serial

77038b0

mna temporarily deployed to Docker Hub April 1, 2026 20:00 — with GitHub Actions Inactive

Fix bad prefix

328569a

mna temporarily deployed to Docker Hub April 1, 2026 20:21 — with GitHub Actions Inactive

Fix invalid alias

261eaf8

mna temporarily deployed to Docker Hub April 1, 2026 20:53 — with GitHub Actions Inactive

mna marked this pull request as ready for review April 1, 2026 21:09

mna requested a review from a team as a code owner April 1, 2026 21:09

claude bot reviewed Apr 1, 2026

View reviewed changes

Add serial index to hdep

01e250f

JordanMontgomery temporarily deployed to Docker Hub April 2, 2026 12:42 — with GitHub Actions Inactive

Fix nilaway complaint(I think?)

cde86ef

JordanMontgomery temporarily deployed to Docker Hub April 2, 2026 12:56 — with GitHub Actions Inactive

MagnusHJensen approved these changes Apr 2, 2026

View reviewed changes

JordanMontgomery merged commit 873d82a into feature-ACME Apr 2, 2026
42 of 45 checks passed

JordanMontgomery deleted the mna-41491-migrate-serials-dep-for-acme branch April 2, 2026 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ACME: migration to add hardware_serial to DEP assignments#42833

ACME: migration to add hardware_serial to DEP assignments#42833
JordanMontgomery merged 13 commits intofeature-ACMEfrom
mna-41491-migrate-serials-dep-for-acme

mna commented Apr 1, 2026 •

edited

Loading

Uh oh!

mna Apr 1, 2026

Uh oh!

mna Apr 1, 2026

Uh oh!

JordanMontgomery Apr 2, 2026

Uh oh!

mna Apr 1, 2026

Uh oh!

JordanMontgomery Apr 2, 2026

Uh oh!

codecov bot commented Apr 1, 2026 •

edited

Loading

Uh oh!

claude bot left a comment

Uh oh!

mna commented Apr 1, 2026

Uh oh!

claude bot Apr 1, 2026

Uh oh!

JordanMontgomery Apr 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		func (ds *Datastore) GetDeviceInfoForACMERenewal(ctx context.Context, hostUUIDs []string) ([]fleet.DeviceInfoForACMERenewal, error) {
		// TODO(mna): anyone knows what those TODOs (from Sarah's PRs) were for?

Conversation

mna commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Testing

Database migrations

Uh oh!

mna Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

mna Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

JordanMontgomery Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

mna Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

JordanMontgomery Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

mna commented Apr 1, 2026

Uh oh!

claude bot Apr 1, 2026

Choose a reason for hiding this comment

What the bug is and how it manifests

The specific code path that triggers it

Why existing code does not prevent it

What the impact would be

How to fix it

Step-by-step proof

Uh oh!

JordanMontgomery Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mna commented Apr 1, 2026 •

edited

Loading

codecov bot commented Apr 1, 2026 •

edited

Loading