Add support for SAN in Android certificate templates.

[getvictor]

2/3rds of this PR is OpenSpec and tests. Use OpenSpec files as a reference (if needed). They're there to help the review, and not to be a review surface themselves.

• Backend implementation for subject_alternative_name in certificate templates.
• Includes schema migration, variable expansion, GitOps support.
• Limits SAN types to DNS, EMAIL, UPN, IP, and URI.

Related issue: Resolves #41472

Checklist for submitter

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

• Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  See Changes files for more information.

• 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.

• Timeouts are implemented and retries are limited to avoid infinite loops

• If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes

Testing

• Added/updated automated tests

• Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)

• QA'd all new/changed functionality manually

For unreleased bug fixes in a release candidate, one of:

• Confirmed that the fix is not expected to adversely impact load test results
• Alerted the release DRI if additional load testing is needed

Database migrations

• Checked schema for all modified table for columns that will auto-update timestamps during migration.
• Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
• Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

New Fleet configuration settings

• Setting(s) is/are explicitly excluded from GitOps

If you didn't check the box above, follow this checklist for GitOps-enabled settings:

• Verified that the setting is exported via fleetctl generate-gitops
• Verified the setting is documented in a separate PR to the GitOps documentation
• Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional)
• Verified that any relevant UI is disabled when GitOps mode is enabled

fleetd/orbit/Fleet Desktop

• Verified compatibility with the latest released version of Fleet (see Must rule)
• If the change applies to only one platform, confirmed that runtime.GOOS is used as needed to isolate changes
• Verified that fleetd runs on macOS, Linux and Windows
• Verified auto-update works from the released version of component to the new version (see tools/tuf/test)

Summary by CodeRabbit

• New Features
  • Certificate templates can include an optional Subject Alternative Name (SAN) for Android certificates; SANs are expanded at delivery and preserved in generated certificates.
• Validation
  • SAN format and allowed keys are validated; SANs require a Premium license when provided.
• Chores
  • Database schema and spec metadata updated to store SANs.
• Tests
  • New unit and end-to-end tests cover SAN behavior and variable expansion.

[getvictor]

@CodeRabbit full review

[getvictor]

/agentic_review

[coderabbitai]

✅ Actions performed

Full review triggered.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (7) 📘 Rule violations (0)

1. Premium gate contradicts spec 🐞 Bug ≡ Correctness ⭐ New

Description

The service only enforces Premium licensing when subject_alternative_name is non-empty, but the
spec added in this PR states non-Premium deployments must reject any certificate template
create/update. As written, code+tests allow non-Premium tenants to create templates as long as SAN
is omitted, which conflicts with the PR’s own requirements and needs a decision/fix before merge.

Code

server/service/certificates.go[R102-118]

+	// Validate the optional SAN: format (token shape, KEY allow-list, length cap) and any
+	// $FLEET_VAR_* references against the same allow-list as subject_name. SAN is Premium-only.
+	if strings.TrimSpace(subjectAlternativeName) != "" {
+		lic, err := svc.License(ctx)
+		if err != nil {
+			return nil, ctxerr.Wrap(ctx, err, "getting license for SAN check")
+		}
+		if !lic.IsPremium() {
+			return nil, fleet.ErrMissingLicense
+		}
+		if err := validateCertificateTemplateSubjectAlternativeName(subjectAlternativeName); err != nil {
+			return nil, &fleet.BadRequestError{Message: err.Error()}
+		}
+		if err := validateCertificateTemplateFleetVariables(subjectAlternativeName); err != nil {
+			return nil, &fleet.BadRequestError{Message: err.Error()}
+		}
+	}

Evidence

CreateCertificateTemplate gates on Premium only inside the subjectAlternativeName non-empty
branch, while the openspec requirement says non-Premium must reject any create/update of a
certificate template. The unit test added in this PR explicitly asserts Free-tier creation succeeds
when SAN is empty, demonstrating the mismatch is intentional in code but inconsistent with the spec
text.

server/service/certificates.go[102-118]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[347-352]
server/service/certificate_templates_test.go[221-253]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The PR’s spec requires non-Premium deployments to reject *any* certificate template create/update, but the implementation (and tests) only enforce Premium when `subject_alternative_name` is set.

### Issue Context
This is a contract mismatch: either the spec is too strict (and must be updated), or the service must enforce Premium for all certificate template writes (and likely reads/UI exposure as well).

### Fix Focus Areas
- server/service/certificates.go[84-119]
- server/service/certificate_templates_test.go[221-263]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[347-352]

### Expected fix
Choose one:
1) **Enforce Premium for all certificate template creates/updates/spec apply**: move the `License().IsPremium()` check outside the SAN branch (and adjust tests accordingly), OR
2) **Make SAN-only Premium**: update the openspec requirement text to match the implemented behavior and ensure docs/design reflect that only SAN is gated.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. SAN allows empty values 🐞 Bug ≡ Correctness ⭐ New

Description

validateCertificateTemplateSubjectAlternativeName permits tokens like DNS= because it checks
only for presence of = and an allowed KEY, not a non-empty value. This lets admins create
templates that are guaranteed to fail later during CSR generation/delivery, despite the design
calling empty values malformed.

Code

server/service/certificate_templates.go[R57-80]

+func validateCertificateTemplateSubjectAlternativeName(san string) error {
+	if strings.TrimSpace(san) == "" {
+		return nil
+	}
+	if len(san) > maxCertificateTemplateSubjectAlternativeNameLength {
+		return fmt.Errorf("subject_alternative_name is too long. Maximum is %d bytes.",
+			maxCertificateTemplateSubjectAlternativeNameLength)
+	}
+	for raw := range strings.SplitSeq(san, ",") {
+		token := strings.TrimSpace(raw)
+		if token == "" {
+			continue
+		}
+		eqIdx := strings.Index(token, "=")
+		if eqIdx <= 0 {
+			return fmt.Errorf("subject_alternative_name token %q is missing '='", token)
+		}
+		key := strings.ToUpper(strings.TrimSpace(token[:eqIdx]))
+		if _, ok := subjectAlternativeNameAllowedKeys[key]; !ok {
+			return fmt.Errorf(
+				"subject_alternative_name has unsupported key %q. Allowed keys are DNS, EMAIL, UPN, IP, URI.",
+				key)
+		}
+	}

Evidence

The validator extracts KEY from the substring before = and checks it against the allow-list, but
never rejects an empty right-hand side. The design doc explicitly lists "DNS=" as malformed and
expected to throw with a clear message.

server/service/certificate_templates.go[51-82]
openspec/changes/android-cert-san-attributes/design.md[195-200]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
SAN validation accepts tokens with empty values (e.g. `DNS=`), which are documented as malformed and will fail later at CSR generation.

### Issue Context
This is still “format-only” validation (not value-content parsing) and provides fast feedback to admins.

### Fix Focus Areas
- server/service/certificate_templates.go[57-80]
- server/service/certificate_templates_test.go[298-333]

### Expected fix
In `validateCertificateTemplateSubjectAlternativeName`, after computing `eqIdx`, add a check like:
- `strings.TrimSpace(token[eqIdx+1:]) != ""` (otherwise return a 422/BadRequestError message identifying the token).
Also add a unit test covering `DNS=` (and similar) to ensure it is rejected early.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. SAN column type mismatch 🐞 Bug ≡ Correctness

Description

The design doc recommends subject_alternative_name VARCHAR(1024) NULL, but the existing
certificate_templates.subject_name column is TEXT, and the spec simultaneously calls for
allowing up to 4096 bytes. If implemented as written, SAN values can be truncated/rejected and the
schema won’t actually “mirror subject_name’s type.”

Code

openspec/changes/android-cert-san-attributes/design.md[R77-82]

+### Storage shape: nullable column, mirror `subject_name`'s type
+
+- **Decision:** New migration adds `subject_alternative_name VARCHAR(1024) NULL` (or whatever type `subject_name` already uses;
+  match it). NULL means "no SAN", which is the existing default behavior.
+- The Go struct field is a plain `string` with `json:"subject_alternative_name,omitempty"`. Empty string and NULL are treated
+  equivalently on read; on write the datastore translates `""` -> NULL.

Evidence

The proposed SAN column type in the new spec contradicts the existing table definition and the
length expectations described elsewhere in the same change set, creating a high risk of implementing
an undersized column or inconsistent schema.

openspec/changes/android-cert-san-attributes/design.md[77-82]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-33]
server/datastore/mysql/migrations/tables/20251124140138_CreateTableCertifcatesTemplates.go[13-20]
server/datastore/mysql/schema.sql[360-366]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The openspec design currently suggests `subject_alternative_name VARCHAR(1024) NULL`, but the existing `certificate_templates.subject_name` is `TEXT`, and the spec also defines a 4096-byte cap. This mismatch can lead to implementing an undersized or inconsistent DB column.
### Issue Context
- Existing DB schema uses `TEXT` for `subject_name`.
- The spec’s create-time validation mentions a 4096-byte max SAN string.
### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/design.md[77-82]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-33]
- server/datastore/mysql/migrations/tables/20251124140138_CreateTableCertifcatesTemplates.go[13-20]
- server/datastore/mysql/schema.sql[360-366]
### Suggested change
Update the design/spec text to explicitly state the SAN column should match `subject_name` (`TEXT`, same collation) and ensure the documented max length (4096 bytes) is compatible with that choice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. SAN validation contradictions 🐞 Bug ⚙ Maintainability

Description

The proposal declares server-side SAN format validation a non-goal (“lets the SAN string through
unvalidated”), but the spec and tasks require server-side format validation (token shape/KEY
allow-list/length cap). This contradiction will produce mismatched implementation and tests unless
resolved explicitly.

Code

openspec/changes/android-cert-san-attributes/proposal.md[R43-46]

+- Server-side validation of the SAN string format. Per the Figma dev note, the server lets the SAN string through unvalidated;
+  if parsing or signing fails, the failure surfaces in the host's "OS settings" modal (host details). The server still
+  validates `$FLEET_VAR_*` references against the existing cert-template variable allow-list, since that check is shared with
+  `subject_name`.

Evidence

Within the same openspec change set, one doc says the server must not validate SAN format, while the
spec and task list say the server must validate it (even if conditional). Without an explicit
decision, different parts of the implementation can ship conflicting behavior.

openspec/changes/android-cert-san-attributes/proposal.md[43-46]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-40]
openspec/changes/android-cert-san-attributes/tasks.md[20-25]
openspec/changes/android-cert-san-attributes/design.md[338-341]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The openspec docs disagree on whether the server validates SAN format. The proposal says “no server-side SAN format validation,” while the spec/tasks require format-only validation (token has `=`, KEY allow-list, length cap).
### Issue Context
This is currently described as conditional on designer confirmation, but the contradiction still needs a single source of truth to prevent implementation/test drift.
### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/proposal.md[43-46]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-40]
- openspec/changes/android-cert-san-attributes/tasks.md[20-25]
- openspec/changes/android-cert-san-attributes/design.md[338-341]
### Suggested change
After confirming the intended behavior, update proposal/spec/tasks so they all match:
- If **no validation**: remove the format-validation requirement from spec/tasks.
- If **format-only validation**: remove the non-goal line from proposal and make the spec requirement unconditional.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. No-op down migration 🐞 Bug ☼ Reliability ⭐ New

Description

The migration adds subject_alternative_name but the Down_... function is a no-op, so schema
rollback can’t revert the change. This diverges from the repository’s standard pattern where down
migrations undo the corresponding up migration.

Code

server/datastore/mysql/migrations/tables/20260504193725_AddSubjectAlternativeNameToCertificateTemplates.go[R12-24]

+func Up_20260504193725(tx *sql.Tx) error {
+	_, err := tx.Exec(`
+		ALTER TABLE certificate_templates
+		ADD COLUMN subject_alternative_name TEXT NULL
+	`)
+	if err != nil {
+		return fmt.Errorf("add subject_alternative_name column to certificate_templates: %w", err)
+	}
+	return nil
+}
+
+func Down_20260504193725(tx *sql.Tx) error {
+	return nil

Evidence

The new migration’s Down function returns nil without dropping the column. Existing migrations that
add a column typically drop it in Down (example: seen_time on hosts).

server/datastore/mysql/migrations/tables/20260504193725_AddSubjectAlternativeNameToCertificateTemplates.go[12-25]
server/datastore/mysql/migrations/tables/20170104113816_AddSeenTimeToHosts.go[9-22]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The down migration for adding `subject_alternative_name` is empty, preventing rollback from undoing the schema change.

### Issue Context
Repo migrations commonly implement a reversible Down (especially for additive columns).

### Fix Focus Areas
- server/datastore/mysql/migrations/tables/20260504193725_AddSubjectAlternativeNameToCertificateTemplates.go[12-25]

### Expected fix
Implement `Down_20260504193725` to execute:
```sql
ALTER TABLE certificate_templates DROP COLUMN subject_alternative_name;
```
and return a wrapped error on failure (consistent with the Up function style).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Empty SAN response ambiguity 🐞 Bug ≡ Correctness

Description

The spec allows the create response to “either omit the key or return an empty string” for an absent
SAN, but the design simultaneously specifies a plain string field with json:",omitempty", which
will omit the key (and will not emit an explicit empty-string field). This leaves client behavior
undefined and can cause inconsistent tests/clients.

Code

openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[R18-20]

+- **WHEN** an admin POSTs a certificate template with no `subject_alternative_name` field, or with an empty string
+- **THEN** the template is stored with NULL in `subject_alternative_name`
+- **AND** the response body either omits the key or returns an empty string, with no validation error

Evidence

The spec’s response contract is explicitly non-deterministic (omit vs empty string), while the
design indicates an omitempty string implementation, which deterministically omits the key when
empty. The docs should choose one representation so clients and tests have a stable contract.

openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[18-20]
openspec/changes/android-cert-san-attributes/design.md[81-82]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The spec’s create/get response semantics for an empty/NULL SAN are ambiguous (“either omit or return empty string”), but the design points toward `omitempty` which omits.
### Issue Context
A stable API contract is needed for frontend/GitOps/agent clients and for automated tests.
### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[18-20]
- openspec/changes/android-cert-san-attributes/design.md[81-82]
### Suggested change
Pick one and document it:
- Prefer: **omit `subject_alternative_name` when empty/NULL** (matches `omitempty`).
- If you truly need an explicit empty string in JSON, document that and update the design to use a `*string` (or custom marshaling) so `"subject_alternative_name": ""` can be emitted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Whitespace normalization unclear 🐞 Bug ⚙ Maintainability

Description

The tasks say to “trim whitespace and store empty as NULL,” but the spec also says SAN is stored as
the “exact string,” and the design calls out an exact-string GitOps round-trip expectation. Without
clarifying whether trimming applies only to detect whitespace-only vs trimming all leading/trailing
whitespace, GitOps/UI round-trips can churn or violate the “exact string” scenario.

Code

openspec/changes/android-cert-san-attributes/tasks.md[R10-12]

+- [ ] 1.4 Update the datastore layer (`server/datastore/mysql/certificate_templates.go`) to read and write
+      `subject_alternative_name` in the create / get-by-id / list / get-for-host queries; trim whitespace and store empty as NULL
+

Evidence

There are conflicting statements about preserving the literal SAN string vs trimming whitespace.
This matters directly for GitOps idempotency and for how strict the “stored exact string” scenario
is interpreted.

openspec/changes/android-cert-san-attributes/tasks.md[10-12]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[11-14]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[5-7]
openspec/changes/android-cert-san-attributes/design.md[301-304]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Docs disagree on whether SAN is preserved verbatim vs trimmed. Tasks say to trim before persisting, while the spec says “store exact string,” and the design highlights exact-string GitOps round-trips.
### Issue Context
You likely want to normalize whitespace-only to NULL, but not necessarily mutate non-empty values.
### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/tasks.md[10-12]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[5-7]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[11-14]
- openspec/changes/android-cert-san-attributes/design.md[301-304]
### Suggested change
Clarify and make all docs consistent, e.g.:
- **Only** `TrimSpace` to check if value is empty/whitespace-only (store NULL), but otherwise persist the original string; OR
- Always `TrimSpace` and persist the trimmed value, and update the “exact string” scenario to reflect that normalization.
Also ensure GitOps tests match the chosen normalization.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 3cb17de

Results up to commit 1a071b7

🐞 Bugs (4) 📘 Rule violations (0)

1. SAN column type mismatch 🐞 Bug ≡ Correctness

Description

The design doc recommends subject_alternative_name VARCHAR(1024) NULL, but the existing
certificate_templates.subject_name column is TEXT, and the spec simultaneously calls for
allowing up to 4096 bytes. If implemented as written, SAN values can be truncated/rejected and the
schema won’t actually “mirror subject_name’s type.”

Code

openspec/changes/android-cert-san-attributes/design.md[R77-82]

+### Storage shape: nullable column, mirror `subject_name`'s type
+
+- **Decision:** New migration adds `subject_alternative_name VARCHAR(1024) NULL` (or whatever type `subject_name` already uses;
+  match it). NULL means "no SAN", which is the existing default behavior.
+- The Go struct field is a plain `string` with `json:"subject_alternative_name,omitempty"`. Empty string and NULL are treated
+  equivalently on read; on write the datastore translates `""` -> NULL.

Evidence

The proposed SAN column type in the new spec contradicts the existing table definition and the
length expectations described elsewhere in the same change set, creating a high risk of implementing
an undersized column or inconsistent schema.

openspec/changes/android-cert-san-attributes/design.md[77-82]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-33]
server/datastore/mysql/migrations/tables/20251124140138_CreateTableCertifcatesTemplates.go[13-20]
server/datastore/mysql/schema.sql[360-366]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The openspec design currently suggests `subject_alternative_name VARCHAR(1024) NULL`, but the existing `certificate_templates.subject_name` is `TEXT`, and the spec also defines a 4096-byte cap. This mismatch can lead to implementing an undersized or inconsistent DB column.

### Issue Context
- Existing DB schema uses `TEXT` for `subject_name`.
- The spec’s create-time validation mentions a 4096-byte max SAN string.

### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/design.md[77-82]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-33]
- server/datastore/mysql/migrations/tables/20251124140138_CreateTableCertifcatesTemplates.go[13-20]
- server/datastore/mysql/schema.sql[360-366]

### Suggested change
Update the design/spec text to explicitly state the SAN column should match `subject_name` (`TEXT`, same collation) and ensure the documented max length (4096 bytes) is compatible with that choice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. SAN validation contradictions 🐞 Bug ⚙ Maintainability

Description

The proposal declares server-side SAN format validation a non-goal (“lets the SAN string through
unvalidated”), but the spec and tasks require server-side format validation (token shape/KEY
allow-list/length cap). This contradiction will produce mismatched implementation and tests unless
resolved explicitly.

Code

openspec/changes/android-cert-san-attributes/proposal.md[R43-46]

+- Server-side validation of the SAN string format. Per the Figma dev note, the server lets the SAN string through unvalidated;
+  if parsing or signing fails, the failure surfaces in the host's "OS settings" modal (host details). The server still
+  validates `$FLEET_VAR_*` references against the existing cert-template variable allow-list, since that check is shared with
+  `subject_name`.

Evidence

Within the same openspec change set, one doc says the server must not validate SAN format, while the
spec and task list say the server must validate it (even if conditional). Without an explicit
decision, different parts of the implementation can ship conflicting behavior.

openspec/changes/android-cert-san-attributes/proposal.md[43-46]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-40]
openspec/changes/android-cert-san-attributes/tasks.md[20-25]
openspec/changes/android-cert-san-attributes/design.md[338-341]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The openspec docs disagree on whether the server validates SAN format. The proposal says “no server-side SAN format validation,” while the spec/tasks require format-only validation (token has `=`, KEY allow-list, length cap).

### Issue Context
This is currently described as conditional on designer confirmation, but the contradiction still needs a single source of truth to prevent implementation/test drift.

### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/proposal.md[43-46]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[28-40]
- openspec/changes/android-cert-san-attributes/tasks.md[20-25]
- openspec/changes/android-cert-san-attributes/design.md[338-341]

### Suggested change
After confirming the intended behavior, update proposal/spec/tasks so they all match:
- If **no validation**: remove the format-validation requirement from spec/tasks.
- If **format-only validation**: remove the non-goal line from proposal and make the spec requirement unconditional.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Empty SAN response ambiguity 🐞 Bug ≡ Correctness

Description

The spec allows the create response to “either omit the key or return an empty string” for an absent
SAN, but the design simultaneously specifies a plain string field with json:",omitempty", which
will omit the key (and will not emit an explicit empty-string field). This leaves client behavior
undefined and can cause inconsistent tests/clients.

Code

openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[R18-20]

+- **WHEN** an admin POSTs a certificate template with no `subject_alternative_name` field, or with an empty string
+- **THEN** the template is stored with NULL in `subject_alternative_name`
+- **AND** the response body either omits the key or returns an empty string, with no validation error

Evidence

The spec’s response contract is explicitly non-deterministic (omit vs empty string), while the
design indicates an omitempty string implementation, which deterministically omits the key when
empty. The docs should choose one representation so clients and tests have a stable contract.

openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[18-20]
openspec/changes/android-cert-san-attributes/design.md[81-82]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The spec’s create/get response semantics for an empty/NULL SAN are ambiguous (“either omit or return empty string”), but the design points toward `omitempty` which omits.

### Issue Context
A stable API contract is needed for frontend/GitOps/agent clients and for automated tests.

### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[18-20]
- openspec/changes/android-cert-san-attributes/design.md[81-82]

### Suggested change
Pick one and document it:
- Prefer: **omit `subject_alternative_name` when empty/NULL** (matches `omitempty`).
- If you truly need an explicit empty string in JSON, document that and update the design to use a `*string` (or custom marshaling) so `"subject_alternative_name": ""` can be emitted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Whitespace normalization unclear 🐞 Bug ⚙ Maintainability

Description

The tasks say to “trim whitespace and store empty as NULL,” but the spec also says SAN is stored as
the “exact string,” and the design calls out an exact-string GitOps round-trip expectation. Without
clarifying whether trimming applies only to detect whitespace-only vs trimming all leading/trailing
whitespace, GitOps/UI round-trips can churn or violate the “exact string” scenario.

Code

openspec/changes/android-cert-san-attributes/tasks.md[R10-12]

+- [ ] 1.4 Update the datastore layer (`server/datastore/mysql/certificate_templates.go`) to read and write
+      `subject_alternative_name` in the create / get-by-id / list / get-for-host queries; trim whitespace and store empty as NULL
+

Evidence

There are conflicting statements about preserving the literal SAN string vs trimming whitespace.
This matters directly for GitOps idempotency and for how strict the “stored exact string” scenario
is interpreted.

openspec/changes/android-cert-san-attributes/tasks.md[10-12]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[11-14]
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[5-7]
openspec/changes/android-cert-san-attributes/design.md[301-304]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Docs disagree on whether SAN is preserved verbatim vs trimmed. Tasks say to trim before persisting, while the spec says “store exact string,” and the design highlights exact-string GitOps round-trips.

### Issue Context
You likely want to normalize whitespace-only to NULL, but not necessarily mutate non-empty values.

### Fix Focus Areas
- openspec/changes/android-cert-san-attributes/tasks.md[10-12]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[5-7]
- openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md[11-14]
- openspec/changes/android-cert-san-attributes/design.md[301-304]

### Suggested change
Clarify and make all docs consistent, e.g.:
- **Only** `TrimSpace` to check if value is empty/whitespace-only (store NULL), but otherwise persist the original string; OR
- Always `TrimSpace` and persist the trimmed value, and update the “exact string” scenario to reflect that normalization.
Also ensure GitOps tests match the chosen normalization.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[coderabbitai]

Walkthrough

Adds end-to-end support for an optional certificate Subject Alternative Name (SAN) on Android certificate templates. Changes include a MySQL migration and schema column, datastore read/write handling, API and service-layer structs and method signatures, SAN format and variable validation, GitOps generation and client-side validation, fleetctl test fixtures, and new unit and integration tests exercising SAN behavior and runtime variable expansion.

Possibly related PRs

• fleetdm/fleet#35788: Introduced the certificate templates feature and GitOps flows that this change extends by adding subject_alternative_name handling across the same structs, datastore, and GitOps code.
• fleetdm/fleet#36978: Modifies certificate-template types/serialization and GitOps generation; overlaps with the current PR's changes to template response types and generate_gitops logic.
• fleetdm/fleet#38114: Updates server-side validation and CreateCertificateTemplate logic for subject fields; related at the code level where subject validation and creation are implemented.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description follows the required template structure with most sections present, but the 'Related issue' field is incomplete and all checklist items are unchecked, suggesting incomplete review of requirements.	Complete the 'Related issue' field (currently shows 'Resolves #' with no issue number) and review/check all applicable checklist items to confirm compliance with submission requirements.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: adding Subject Alternative Name (SAN) support to Android certificate templates, which is the primary feature across the entire changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch victor/41472-phase1

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Adds an OpenSpec (proposal/design/spec/tasks) for implementing subject_alternative_name (SAN) support in Android certificate templates, including backend persistence, GitOps, frontend UI, and Android agent CSR behavior.

Changes:

• Added OpenSpec proposal, design, and detailed acceptance spec for SAN support end-to-end.
• Added an implementation task breakdown covering backend/API/datastore, GitOps apply/generate, frontend modal UX, and Android agent parsing/CSR extension wiring.
• Added .openspec.yaml to mark the change as spec-driven.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
openspec/changes/android-cert-san-attributes/tasks.md	Step-by-step implementation plan across backend/frontend/GitOps/Android agent.
openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md	Acceptance requirements and scenarios for SAN persistence/validation/expansion and Android CSR behavior.
openspec/changes/android-cert-san-attributes/proposal.md	Problem statement + intended product/engineering changes and rollout considerations.
openspec/changes/android-cert-san-attributes/design.md	Detailed design decisions, parsing/encoding rules, rollout plan, and risks.
openspec/changes/android-cert-san-attributes/.openspec.yaml	Declares the change as spec-driven with creation date.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[getvictor]

@coderabbitai full review

[getvictor]

/agentic_review

[coderabbitai]

✅ Actions performed

Full review triggered.

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 3cb17de

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/datastore/mysql/certificate_templates.go (1)

261-273: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update all mutable fields in the duplicate-key branch.

Reapplying a spec with the same template name but a new subject_name or certificate_authority_id will keep the old row values, because the ON DUPLICATE KEY UPDATE clause only touches team_id and subject_alternative_name. ApplyCertificateTemplateSpecs goes through this path, so GitOps edits to existing templates won't fully take effect.

Suggested fix

 		ON DUPLICATE KEY UPDATE
-			name = VALUES(name),
-			team_id = VALUES(team_id),
-			subject_alternative_name = VALUES(subject_alternative_name)
+			certificate_authority_id = VALUES(certificate_authority_id),
+			subject_name = VALUES(subject_name),
+			subject_alternative_name = VALUES(subject_alternative_name)
 	`

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/datastore/mysql/certificate_templates.go` around lines 261 - 273, The
INSERT's ON DUPLICATE KEY UPDATE clause in the sqlInsertCertificate constant
only updates team_id and subject_alternative_name, so reapplying a spec via
ApplyCertificateTemplateSpecs won't overwrite changed fields; update the clause
to also set certificate_authority_id and subject_name (using
VALUES(certificate_authority_id) and VALUES(subject_name)) so all mutable fields
from the new spec are applied on duplicate-key upserts.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/datastore/mysql/schema.sql`:
- Line 1973: The CREATE TABLE declaration ending with "ENGINE=InnoDB
AUTO_INCREMENT=522 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci" has the
wrong AUTO_INCREMENT; update the AUTO_INCREMENT value from 522 to 523 to reflect
the new seeded row (id=522) so the table's CREATE statement follows the file's
convention of last_inserted_id + 1.

In `@server/fleet/certificate_templates.go`:
- Line 27: Change the CertificateTemplateResponseSummary.SubjectAlternativeName
from string to *string so sqlx can scan NULL DB values without panicking; update
any JSON handling or marshaling logic that assumes a non-nil string (the struct
tag json:"subject_alternative_name,omitempty" will already omit nils, but ensure
callers and code that dereference SubjectAlternativeName check for nil).
Alternatively, if you prefer DB-side fixes, update the migration to set
subject_alternative_name NOT NULL DEFAULT '' instead of TEXT NULL so the
existing string type remains safe.

In `@server/service/certificate_templates.go`:
- Around line 51-81: The validator currently accepts separator-only SAN strings
like "," because it skips empty tokens; update
validateCertificateTemplateSubjectAlternativeName to detect when no non-empty
key=value tokens were found and reject that case (or normalize to empty before
licensing/storage); specifically, track a count of valid tokens while iterating
the tokens from strings.SplitSeq(san, ",") in
validateCertificateTemplateSubjectAlternativeName and if count==0 after the loop
return an error such as "subject_alternative_name contains no entries" (or
return nil to normalize to empty) so separator-only inputs are not treated as a
valid non-empty SAN; reference the
validateCertificateTemplateSubjectAlternativeName function and the
subjectAlternativeNameAllowedKeys lookup when implementing this change.

In `@server/service/certificates.go`:
- Around line 283-299: When SAN expansion fails in the block handling
certificate.SubjectAlternativeName (and similarly in the earlier subject_name
failure path), clear any active challenges on the returned certificate by
setting certificate.SCEPChallenge = nil and certificate.FleetChallenge = nil
before setting certificate.Status = fleet.CertificateTemplateFailed and
returning; keep the existing UpsertCertificateStatus logic
(svc.ds.UpsertCertificateStatus with fleet.CertificateStatusUpdate) and ensure
you apply the same niling/guard to the subject_name error branch that calls
svc.replaceCertificateVariables so the device cannot receive active challenges
alongside a failed template.

---

Outside diff comments:
In `@server/datastore/mysql/certificate_templates.go`:
- Around line 261-273: The INSERT's ON DUPLICATE KEY UPDATE clause in the
sqlInsertCertificate constant only updates team_id and subject_alternative_name,
so reapplying a spec via ApplyCertificateTemplateSpecs won't overwrite changed
fields; update the clause to also set certificate_authority_id and subject_name
(using VALUES(certificate_authority_id) and VALUES(subject_name)) so all mutable
fields from the new spec are applied on duplicate-key upserts.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 14bc6ab3-da1d-4cb8-8260-36aace2bd42e

📥 Commits

Reviewing files that changed from the base of the PR and between 4be56c1 and 3cb17de.

⛔ Files ignored due to path filters (4)

• openspec/changes/android-cert-san-attributes/design.md is excluded by !**/*.md
• openspec/changes/android-cert-san-attributes/proposal.md is excluded by !**/*.md
• openspec/changes/android-cert-san-attributes/specs/android-cert-san/spec.md is excluded by !**/*.md
• openspec/changes/android-cert-san-attributes/tasks.md is excluded by !**/*.md

📒 Files selected for processing (19)

• cmd/fleetctl/fleetctl/generate_gitops.go
• cmd/fleetctl/fleetctl/generate_gitops_test.go
• cmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamControls.yaml
• cmd/fleetctl/fleetctl/testdata/generateGitops/teamConfig.json
• cmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.yml
• openspec/changes/android-cert-san-attributes/.openspec.yaml
• pkg/spec/gitops.go
• server/datastore/mysql/certificate_templates.go
• server/datastore/mysql/migrations/tables/20260504193725_AddSubjectAlternativeNameToCertificateTemplates.go
• server/datastore/mysql/schema.sql
• server/fleet/app.go
• server/fleet/certificate_templates.go
• server/fleet/service.go
• server/mock/service/service_mock.go
• server/service/certificate_templates.go
• server/service/certificate_templates_test.go
• server/service/certificates.go
• server/service/client.go
• server/service/integration_android_certificate_templates_test.go

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[getvictor]

@claude review once

[codecov]

Codecov Report

❌ Patch coverage is 66.91729% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.68%. Comparing base (5ee53b4) to head (1e94550).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
server/service/certificates.go	40.00%	25 Missing and 5 partials ⚠️
server/service/client.go	41.17%	9 Missing and 1 partial ⚠️
...AddSubjectAlternativeNameToCertificateTemplates.go	69.23%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #44690    +/-   ##
========================================
  Coverage   66.68%   66.68%            
========================================
  Files        2651     2652     +1     
  Lines      213533   213676   +143     
  Branches     9734     9644    -90     
========================================
+ Hits       142398   142494    +96     
- Misses      58171    58209    +38     
- Partials    12964    12973     +9     

Flag	Coverage Δ
backend	68.55% <66.91%> (-0.01%)	⬇️

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.