Android certificate crud: validate variable replacement - #36648
Conversation
If variables can't be interpolated return 400.
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #36648 +/- ##
============================================
+ Coverage 65.95% 65.99% +0.03%
Complexity 2 2
============================================
Files 2247 2248 +1
Lines 183631 183768 +137
Branches 7591 7591
============================================
+ Hits 121121 121269 +148
+ Misses 51436 51415 -21
- Partials 11074 11084 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
@juan-fdz-hawa Thank you for taking this task.
We need a bit of a customized flow here. This endpoint will only be hit by a specific Android agent before fetching a certificate.
Thus, if any fleet vars specified in the SubjectName are empty/missing, then we need to mark the HostCertificateTemplate with status fleet.MDMDeliveryFailed for the host and certificate template.
This way the certificate/profile will immediately show up as failed on the Host Details OS settings modal. And the Android agent will not try to use SCEP to get the cert.
After updating the DB, this endpoint should return OK with status=fleet.MDMDeliveryFailed in its regular payload (instead of Pending). We do not need to decrypt/return SCEPchallenge/fleetChallenge if status is not Pending.
This is a similar flow we do with MDM profiles when we can't replace variables:
fleet/server/service/apple_mdm.go
Line 5706 in 915f529
For reference, this is the updated flowchart for Android certs:
https://github.com/fleetdm/fleet/pull/36538/files?short_path=3ef7de3#diff-3ef7de32380db57632181ccc1ebdaf92304c00a42d6a9439350d34992d772479
Thank you for the context! I have a couple of follow up questions:
|
|
The implementation we currently have in place for
Ok, no problem. I'll update the
|
GitHub Actions
Inactive
I don't think we need the |
GitHub Actions
Inactive
@getvictor I think you're right. When I specified that one, I thought the agent would use it. I didn't add it to the API PR for #30876 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis PR changes certificate template variable interpolation failure handling from returning errors to upserting failed delivery status records. It refactors the status update method from Update to Upsert pattern, supporting record insertion when needed, and updates integration tests to verify the new behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Android Device/Client
participant Service as Fleet Service
participant Datastore as MySQL Datastore
Client->>Service: Request device certificate template
activate Service
Service->>Service: Attempt to interpolate variables<br/>(replace placeholders)
alt Variable Interpolation Success
Service->>Datastore: UpsertCertificateStatus<br/>(MDMDeliverySuccess)
activate Datastore
Datastore->>Datastore: UPDATE existing record
Datastore-->>Service: Success
deactivate Datastore
Service-->>Client: Return certificate (200)
else Variable Interpolation Fails
rect rgb(255, 200, 200)
Note over Service: New Behavior:<br/>Record failure instead of error
Service->>Datastore: UpsertCertificateStatus<br/>(MDMDeliveryFailed, error detail)
activate Datastore
Datastore->>Datastore: UPDATE existing record
alt No rows affected
Datastore->>Datastore: SELECT template exists?
alt Template exists
Datastore->>Datastore: INSERT new record
else Template not found
Datastore-->>Service: Error (template not found)
end
end
Datastore-->>Service: Status recorded
deactivate Datastore
end
Service-->>Client: Return certificate with<br/>MDMDeliveryFailed status (200)
end
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/integration_core_test.go (1)
8034-8046: Make certificate template selection deterministic and add nil‑safety in assertionsRight now the test assumes
savedCertificateTemplates[0]is the correct template and thatCertificate/Statusare always non‑nil:
savedCertificateTemplates[0]relies on datastore ordering and non‑empty results; later checks (including subject interpolation against hardware serial) implicitly expect this to be “Template 1”.*getCertResp.Certificate.Statuswill panic if eitherCertificateorStatusis unexpectedly nil; arequire.NotNilwould yield a clearer test failure.Consider tightening this to assert size and pick the intended template by name, and to guard the dereference:
- savedCertificateTemplates, _, err := s.ds.GetCertificateTemplatesByTeamID(ctx, team.ID, fleet.ListOptions{Page: 0, PerPage: 10}) - require.NoError(t, err) - certID := savedCertificateTemplates[0].ID + savedCertificateTemplates, _, err := s.ds.GetCertificateTemplatesByTeamID(ctx, team.ID, fleet.ListOptions{Page: 0, PerPage: 10}) + require.NoError(t, err) + require.NotEmpty(t, savedCertificateTemplates) + + var certID uint + for _, tmpl := range savedCertificateTemplates { + if tmpl.Name == "Template 1" { + certID = tmpl.ID + break + } + } + require.NotZero(t, certID, "expected to find certificate template %q for this test", "Template 1") @@ - require.NoError(t, json.NewDecoder(resp.Body).Decode(&getCertResp)) - require.NoError(t, resp.Body.Close()) - require.Equal(t, *getCertResp.Certificate.Status, fleet.MDMDeliveryFailed) + require.NoError(t, json.NewDecoder(resp.Body).Decode(&getCertResp)) + require.NoError(t, resp.Body.Close()) + require.NotNil(t, getCertResp.Certificate) + require.NotNil(t, getCertResp.Certificate.Status) + require.Equal(t, fleet.MDMDeliveryFailed, *getCertResp.Certificate.Status)This removes any dependency on implicit ordering and makes failures around the returned payload easier to diagnose.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
changes/36533-use-proper-status-code-on-failure-to-interpolate(1 hunks)server/datastore/mysql/host_certificate_templates.go(3 hunks)server/datastore/mysql/host_certificate_templates_test.go(5 hunks)server/fleet/datastore.go(1 hunks)server/mock/datastore_mock.go(3 hunks)server/service/certificates.go(2 hunks)server/service/integration_core_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/host_certificate_templates.goserver/service/certificates.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/integration_core_test.goserver/datastore/mysql/host_certificate_templates_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: getvictor
Repo: fleetdm/fleet PR: 36139
File: android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt:75-76
Timestamp: 2025-11-26T18:58:18.865Z
Learning: In Fleet's Android MDM agent SCEP implementation (android/app/src/main/java/com/fleetdm/agent/scep/ScepClientImpl.kt), OptimisticCertificateVerifier is intentionally used because: (1) SCEP URL is provided by authenticated MDM server, (2) challenge password authenticates enrollment, (3) enterprise SCEP servers use internal CAs not in system trust stores, (4) enrolled certificate is validated when used.
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/datastore/mysql/host_certificate_templates.go
📚 Learning: 2025-07-08T16:12:48.797Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/datastore/mysql/host_certificate_templates.goserver/datastore/mysql/host_certificate_templates_test.go
🧬 Code graph analysis (2)
server/fleet/datastore.go (1)
server/fleet/mdm.go (1)
MDMDeliveryStatus(462-462)
server/service/integration_core_test.go (2)
server/fleet/app.go (1)
ListOptions(1251-1275)server/fleet/mdm.go (1)
MDMDeliveryFailed(500-500)
🔇 Additional comments (9)
changes/36533-use-proper-status-code-on-failure-to-interpolate (1)
1-1: LGTM! Clear and concise changelog entry.The changelog entry accurately describes the fix for handling certificate template variable interpolation failures.
server/service/certificates.go (2)
405-405: LGTM! Method rename reflects upsert semantics.The change from
UpdateCertificateStatustoUpsertCertificateStatuscorrectly reflects the new semantics where the status record is inserted if it doesn't exist. This aligns with the PR objectives to handle cases where thehost_certificate_templatesrow may not yet exist.
157-170: Good implementation of failed status handling with proper SQL filtering.The implementation correctly aligns with PR objectives by upserting failed status and returning the certificate instead of an error. The underlying SQL query properly filters by both
host_uuidANDcertificate_template_idin the UPDATE clause (line 178 of host_certificate_templates.go), ensuring updates target the correct record.Note that if
UpsertCertificateStatusitself fails, an error is still returned—this breaks the "always return OK" pattern but is necessary since we couldn't record the failure.server/service/integration_core_test.go (1)
8058-8058: Good negative‑path coverage for missing node key on fleetd certificate fetchThe unauthorized
GET /api/fleetd/certificates/{id}case correctly verifies that a missingAuthorization/node key yields401and ensures the response body is closed; no changes needed here.server/fleet/datastore.go (1)
2516-2516: LGTM!The rename from
UpdateCertificateStatustoUpsertCertificateStatusaccurately reflects the new behavior where the method inserts a record when none exists for the host/template pair.server/datastore/mysql/host_certificate_templates.go (1)
168-219: Upsert implementation looks correct.The update-first, then check-and-insert pattern correctly handles both scenarios:
- Updating existing
host_certificate_templatesrecords- Creating new records when variable interpolation fails (per PR objectives)
The empty
fleet_challengeon insert aligns with the requirement to not return/decrypt SCEP challenge when marking status as failed.server/datastore/mysql/host_certificate_templates_test.go (1)
615-735: Test coverage for upsert behavior is comprehensive.The test correctly covers:
- Updating an existing record (
ct1with pre-insertedhost_certificate_templatesrow)- Creating a new record when none exists (
ct2without pre-existing row)- Invalid status validation
- Detail field propagation
The use of
CreateCertificateTemplateAPI instead of ad-hoc SQL is a good improvement for test maintainability.server/mock/datastore_mock.go (2)
9784-9788: UpsertCertificateStatus mock method correctly wires to function fieldThe new
UpsertCertificateStatusmethod uses the standard pattern (lock, set...Invoked, unlock, delegate to func) and passes all arguments through unchanged. This cleanly maintains behavior with the updated name.
1640-1640: UpsertCertificateStatus mock type and fields are correctly implementedThe
UpsertCertificateStatusFunctype at line 1640 and correspondingDataStorefields (lines 4090-4091) properly align with the method implementation (lines 9784-9788), following the standard mock pattern. Parameter signatures match and the invocation flag convention is consistent with the rest of the mock.Note: Searches reveal
UpdateCertificateStatusreferences remain in the service layer (./server/mock/service/service_mock.go,./server/service/certificates.go,./server/fleet/service.go). This is expected—the datastore layer usesUpsertCertificateStatuswhile the service layer retainsUpdateCertificateStatus. No action needed.
4 participants
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.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.