From 49db931ffbe67f4f428457f46dddf5753fa12a97 Mon Sep 17 00:00:00 2001 From: Dante Catalfamo <43040593+dantecatalfamo@users.noreply.github.com> Date: Fri, 5 Jun 2026 16:20:32 -0400 Subject: [PATCH] Auto-clean duplicate Okta CA SCEP cert after profile install (#46172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Related issue:** Resolves #42757 ## Summary Resending or renewing the Okta conditional access profile leaves an orphaned SCEP certificate in the per-user macOS keychain, accumulating duplicates with every renewal. This PR auto-runs an existing keychain-cleanup script after a successful `InstallProfile` ack for the Okta CA profile, so admins no longer have to find and run the script manually. ## Root cause Investigation in the issue thread isolated the trigger: - The Okta CA `.mobileconfig` bundles `com.apple.security.scep` with `com.apple.security.identitypreference` in a single profile (macOS rejects the alternative — `Identity payload not found in same profile as identity preference payload`). - The Identity Preference payload creates a keychain-resident preference item that keeps the *old* cert pinned across profile replacement, even though the rewritten Identity Preference now points to the fresh SCEP enrollment. - EAP-TLS Wi-Fi profiles renew cleanly because they reference the cert via SystemConfiguration (`PayloadCertificateUUID`), not the keychain — so this isn't a generic SCEP-bundling issue. The team decision in the issue (`@sharon-fdm`) was to delete the duplicate certificate rather than restructure the profile. A standalone cleanup script already shipped at `docs/solutions/macos/scripts/delete-duplicate-scep-certificates.sh` and was linked from the Okta CA guide; admins had to find and run it. ## Approach Hook the existing Apple MDM `InstallProfile` ack path in `MDMAppleCheckinAndCommandService.CommandAndReportResults`, parallel to the existing ACME `CertificateList` follow-up. When the ack is for the Okta CA profile and status is `verifying`, enqueue an internal host script run that executes the cleanup script targeting the host's per-user MDM enrollment short name. Key properties: - **Single hook, three paths covered.** Admin "Resend" nulls the profile status and the reconciliation cron re-enqueues an `InstallProfile`; the SCEP renewal cron also re-issues `InstallProfile`. Both flow through the same ack handler this hook attaches to. - **Idempotent.** The cleanup script no-ops when only one matching cert is present, so triggering on initial installs (not just renewals) is safe and removes the need to distinguish "is this a renewal". - **Tightly gated.** Single indexed lookup keyed on `(host_uuid, command_uuid, profile_identifier, platform='darwin')`. Other SCEP-bearing profiles do not trigger the script. No work happens for hosts with no per-user enrollment. - **Internal-script semantics** (matches lock/unlock/wipe prior art). Runs even when scripts are globally disabled. Does not appear in the user-facing host activity feed. - **Failure-isolated.** Enqueue errors are logged but do not break the ack path; the renewal itself is what matters. - **Defense in depth on the shell call.** The macOS short name is validated against a strict regex (`^[A-Za-z0-9_][A-Za-z0-9_.-]*$`, ≤31 chars) before being interpolated, and POSIX single-quote-escaped on the way through. ## Files **New** - `server/service/conditional_access_cleanup.go` — `//go:embed` of the cleanup script, the hook helper `maybeRunOktaCACleanupScript`, the validated shell-wrapper builder, and the POSIX single-quote escape helper. - `server/service/conditional_access_cleanup_test.go` — unit coverage for username validation, shell escaping, the routing decisions of the hook helper (mock-based), and an embed-sync assertion against the docs copy. - `server/service/embedded_scripts/delete-duplicate-scep-certificates.sh` — embed source-of-truth copy, byte-for-byte equal to the public `docs/solutions/macos/scripts/` script. - `changes/42757-okta-conditional-access-duplicate-scep-cert-cleanup` — user-visible changes note. **Datastore** - `server/datastore/mysql/mdm.go` — `OktaCACleanupTargetForInstallCommand`: single SQL lookup that returns `(host_id, user_short_name, ok)` for the new hook. Returns `ok=false` for non-Okta profiles, non-darwin hosts, or hosts without a user-channel enrollment. - `server/datastore/mysql/scripts.go` — `NewInternalHostScriptExecutionRequest`: thin wrapper that routes through the existing internal-script codepath (`isInternal=true`) used by lock/unlock/wipe. Refactored the existing public method to share an internal helper. **Interface / mocks** - `server/fleet/conditional_access_idp.go` — exported `ConditionalAccessOktaProfileIdentifier`, `ConditionalAccessOktaCertificateCN`, and the new `OktaCACleanupTarget` struct, so both the template-render path and the SQL lookup can reference the same source of truth. - `server/fleet/datastore.go` — `OktaCACleanupTargetForInstallCommand` and `NewInternalHostScriptExecutionRequest` added to the `Datastore` interface. - `server/mock/datastore_mock.go` — regenerated (additions only). **Wiring** - `server/service/apple_mdm.go` — call into `maybeRunOktaCACleanupScript` from the InstallProfile `MDMDeliveryVerifying` branch, alongside the existing ACME `maybeQueueCertificateListForACMEProfile` follow-up. Warns on error rather than failing the ack. - `server/service/conditional_access_idp.go` — use the new `fleet.ConditionalAccessOktaCertificateCN` constant when rendering the profile template, eliminating the magic string duplication. **Tests touched** - `server/datastore/mysql/mdm_test.go` — integration test `testOktaCACleanupTargetForInstallCommand` covering the happy path, non-Okta profile, device-only enrollment, and unknown command. - `server/datastore/mysql/scripts_test.go` — `testNewInternalHostScriptExecutionRequest` confirming the internal flag is set correctly and the new entry only appears under the internal-only listing filter. - `server/service/apple_mdm_test.go` — added the new mock stub for `OktaCACleanupTargetForInstallCommandFunc` to `TestMDMCommandAndReportResultsProfileHandling` so the existing test continues to pass with the new hook in the codepath. - `server/service/conditional_access_idp_test.go` — the rendered-profile assertion now also pins on the shared `ConditionalAccessOktaProfileIdentifier` and `ConditionalAccessOktaCertificateCN` constants so the template can't drift from the SQL lookup. --- ...itional-access-duplicate-scep-cert-cleanup | 1 + server/datastore/mysql/mdm.go | 39 +++++ server/datastore/mysql/mdm_test.go | 83 ++++++++++ server/datastore/mysql/scripts.go | 15 +- server/datastore/mysql/scripts_test.go | 39 +++++ server/fleet/conditional_access_idp.go | 20 +++ server/fleet/datastore.go | 14 ++ server/mock/datastore_mock.go | 24 +++ server/service/apple_mdm.go | 9 ++ server/service/apple_mdm_test.go | 3 + server/service/conditional_access_cleanup.go | 83 ++++++++++ .../conditional_access_cleanup_test.go | 149 ++++++++++++++++++ server/service/conditional_access_idp.go | 2 +- server/service/conditional_access_idp_test.go | 7 +- .../delete-duplicate-scep-certificates.sh | 147 +++++++++++++++++ 15 files changed, 631 insertions(+), 4 deletions(-) create mode 100644 changes/42757-okta-conditional-access-duplicate-scep-cert-cleanup create mode 100644 server/service/conditional_access_cleanup.go create mode 100644 server/service/conditional_access_cleanup_test.go create mode 100755 server/service/embedded_scripts/delete-duplicate-scep-certificates.sh diff --git a/changes/42757-okta-conditional-access-duplicate-scep-cert-cleanup b/changes/42757-okta-conditional-access-duplicate-scep-cert-cleanup new file mode 100644 index 0000000000..1154f8a43c --- /dev/null +++ b/changes/42757-okta-conditional-access-duplicate-scep-cert-cleanup @@ -0,0 +1 @@ +- Removed orphaned duplicate SCEP certificates from the per-user keychain automatically after an Okta conditional access profile is reinstalled or renewed on macOS hosts. diff --git a/server/datastore/mysql/mdm.go b/server/datastore/mysql/mdm.go index ab07e43618..5cd7716293 100644 --- a/server/datastore/mysql/mdm.go +++ b/server/datastore/mysql/mdm.go @@ -1657,6 +1657,45 @@ WHERE return dest, nil } +// OktaCACleanupTargetForInstallCommand returns the host ID and target +// macOS short name needed to schedule the Okta conditional access +// keychain-cleanup script after a successful InstallProfile ack. The ok +// return is false when the command's profile is not the Okta CA profile, +// the host's platform is not darwin, or the host has no per-user MDM +// enrollment short name on record. All gating is done in a single indexed +// lookup keyed on (host_uuid, command_uuid). +func (ds *Datastore) OktaCACleanupTargetForInstallCommand(ctx context.Context, hostUUID, commandUUID string) (fleet.OktaCACleanupTarget, bool, error) { + const stmt = ` +SELECT + h.id AS host_id, + COALESCE(( + SELECT nu.user_short_name + FROM nano_enrollments ne + INNER JOIN nano_users nu ON ne.user_id = nu.id + WHERE ne.type = 'User' AND ne.enabled = 1 AND ne.device_id = h.uuid + ORDER BY ne.created_at ASC, ne.id ASC LIMIT 1 + ), '') AS user_short_name +FROM host_mdm_apple_profiles hmap +JOIN hosts h ON h.uuid = hmap.host_uuid +WHERE hmap.command_uuid = ? + AND hmap.host_uuid = ? + AND hmap.profile_identifier = ? + AND h.platform = 'darwin'` + + var dest fleet.OktaCACleanupTarget + err := sqlx.GetContext(ctx, ds.reader(ctx), &dest, stmt, commandUUID, hostUUID, fleet.ConditionalAccessOktaProfileIdentifier) + if err != nil { + if errors.Is(err, sql.ErrNoRows) { + return dest, false, nil + } + return dest, false, ctxerr.Wrap(ctx, err, "look up Okta CA cleanup target") + } + if dest.UserShortName == "" { + return dest, false, nil + } + return dest, true, nil +} + func (ds *Datastore) ProfileHasACMEPayloadForCommand(ctx context.Context, hostUUID, commandUUID string) (fleet.ProfileACMECommandResult, error) { const stmt = ` SELECT diff --git a/server/datastore/mysql/mdm_test.go b/server/datastore/mysql/mdm_test.go index bf28e58aa0..9602af8319 100644 --- a/server/datastore/mysql/mdm_test.go +++ b/server/datastore/mysql/mdm_test.go @@ -65,6 +65,7 @@ func TestMDMShared(t *testing.T) { {"TestCleanUpMDMManagedCertificates", testCleanUpMDMManagedCertificates}, {"TestEnqueueCommandWithName", testEnqueueCommandWithName}, {"TestProfileHasACMEPayloadForCommand", testProfileHasACMEPayloadForCommand}, + {"TestOktaCACleanupTargetForInstallCommand", testOktaCACleanupTargetForInstallCommand}, {"TestRenewMDMManagedCertificatesNullType", testRenewMDMManagedCertificatesNullType}, } @@ -6206,6 +6207,88 @@ func testProfileHasACMEPayloadForCommand(t *testing.T, ds *Datastore) { }) } +func testOktaCACleanupTargetForInstallCommand(t *testing.T, ds *Datastore) { + ctx := t.Context() + + newHost := func(t *testing.T, suffix, platform string) *fleet.Host { + t.Helper() + h, err := ds.NewHost(ctx, &fleet.Host{ + DetailUpdatedAt: time.Now(), + LabelUpdatedAt: time.Now(), + PolicyUpdatedAt: time.Now(), + SeenTime: time.Now(), + OsqueryHostID: ptr.String("okta-cleanup-osq-" + suffix), + NodeKey: ptr.String("okta-cleanup-nk-" + suffix), + UUID: "okta-cleanup-host-" + suffix, + Hostname: "okta-cleanup-" + suffix, + Platform: platform, + }) + require.NoError(t, err) + return h + } + + insertHostProfile := func(t *testing.T, hostUUID, identifier, commandUUID string) { + t.Helper() + // host_mdm_apple_profiles has no FK to mdm_apple_configuration_profiles, + // so we can insert the host-profile row directly with a fresh + // profile_uuid per sub-test and avoid the (team_id, identifier) + // unique-key conflict that comes from reusing the Okta CA identifier. + require.NoError(t, ds.BulkUpsertMDMAppleHostProfiles(ctx, []*fleet.MDMAppleBulkUpsertHostProfilePayload{{ + ProfileUUID: uuid.NewString(), + ProfileIdentifier: identifier, + HostUUID: hostUUID, + Checksum: []byte("0123456789abcdef"), + Scope: fleet.PayloadScopeUser, + OperationType: fleet.MDMOperationTypeInstall, + CommandUUID: commandUUID, + }})) + } + + t.Run("okta CA profile + per-user enrollment present: returns target", func(t *testing.T) { + host := newHost(t, "happy", "darwin") + nanoEnroll(t, ds, host, true) // creates Device + User enrollment with user_short_name = "alice" + cmdUUID := uuid.NewString() + insertHostProfile(t, host.UUID, fleet.ConditionalAccessOktaProfileIdentifier, cmdUUID) + + got, ok, err := ds.OktaCACleanupTargetForInstallCommand(ctx, host.UUID, cmdUUID) + require.NoError(t, err) + require.True(t, ok) + require.Equal(t, host.ID, got.HostID) + require.Equal(t, "alice", got.UserShortName) + }) + + t.Run("non-Okta profile identifier: ok=false", func(t *testing.T) { + host := newHost(t, "wrong-id", "darwin") + nanoEnroll(t, ds, host, true) + cmdUUID := uuid.NewString() + insertHostProfile(t, host.UUID, "com.example.unrelated", cmdUUID) + + _, ok, err := ds.OktaCACleanupTargetForInstallCommand(ctx, host.UUID, cmdUUID) + require.NoError(t, err) + require.False(t, ok) + }) + + t.Run("okta profile + only device-channel enrollment: ok=false", func(t *testing.T) { + host := newHost(t, "no-user-chan", "darwin") + nanoEnroll(t, ds, host, false) // Device-only enrollment, no nano_users row + cmdUUID := uuid.NewString() + insertHostProfile(t, host.UUID, fleet.ConditionalAccessOktaProfileIdentifier, cmdUUID) + + _, ok, err := ds.OktaCACleanupTargetForInstallCommand(ctx, host.UUID, cmdUUID) + require.NoError(t, err) + require.False(t, ok) + }) + + t.Run("unknown command: ok=false, no error", func(t *testing.T) { + host := newHost(t, "no-cmd", "darwin") + nanoEnroll(t, ds, host, true) + + _, ok, err := ds.OktaCACleanupTargetForInstallCommand(ctx, host.UUID, "no-such-cmd") + require.NoError(t, err) + require.False(t, ok) + }) +} + func testRenewMDMManagedCertificatesNullType(t *testing.T, ds *Datastore) { ctx := t.Context() diff --git a/server/datastore/mysql/scripts.go b/server/datastore/mysql/scripts.go index 417d3c2afd..9aad11d19d 100644 --- a/server/datastore/mysql/scripts.go +++ b/server/datastore/mysql/scripts.go @@ -36,6 +36,19 @@ var hostScriptDetailsAllowedOrderKeys = common_mysql.OrderKeyAllowlist{ } func (ds *Datastore) NewHostScriptExecutionRequest(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) { + return ds.newHostScriptExecutionRequestPublic(ctx, request, false) +} + +// NewInternalHostScriptExecutionRequest enqueues a host script run flagged +// as internal (fleet-initiated). Internal scripts run even when scripts +// are globally disabled and do not appear in the user-facing host activity +// feed. Use for server-driven follow-up actions (e.g. cleanup scripts +// after MDM events) rather than user-requested runs. +func (ds *Datastore) NewInternalHostScriptExecutionRequest(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) { + return ds.newHostScriptExecutionRequestPublic(ctx, request, true) +} + +func (ds *Datastore) newHostScriptExecutionRequestPublic(ctx context.Context, request *fleet.HostScriptRequestPayload, isInternal bool) (*fleet.HostScriptResult, error) { var res *fleet.HostScriptResult return res, ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { var err error @@ -49,7 +62,7 @@ func (ds *Datastore) NewHostScriptExecutionRequest(ctx context.Context, request id, _ := scRes.LastInsertId() request.ScriptContentID = uint(id) //nolint:gosec // dismiss G115 } - res, err = ds.newHostScriptExecutionRequest(ctx, tx, request, false) + res, err = ds.newHostScriptExecutionRequest(ctx, tx, request, isInternal) return err }) } diff --git a/server/datastore/mysql/scripts_test.go b/server/datastore/mysql/scripts_test.go index 05c10a52e5..af543775ed 100644 --- a/server/datastore/mysql/scripts_test.go +++ b/server/datastore/mysql/scripts_test.go @@ -56,6 +56,7 @@ func TestScripts(t *testing.T) { {"BatchSetScriptActivatesNextActivity", testBatchSetScriptActivatesNextActivity}, {"CountHostScriptAttempts", testCountHostScriptAttempts}, {"ScriptModificationResetsAttemptNumber", testScriptModificationResetsAttemptNumber}, + {"NewInternalHostScriptExecutionRequest", testNewInternalHostScriptExecutionRequest}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -3252,3 +3253,41 @@ func testScriptModificationResetsAttemptNumber(t *testing.T, ds *Datastore) { require.Equal(t, int64(0), *results[1].AttemptNumber) require.True(t, results[1].Canceled) } + +func testNewInternalHostScriptExecutionRequest(t *testing.T, ds *Datastore) { + ctx := context.Background() + + res, err := ds.NewInternalHostScriptExecutionRequest(ctx, &fleet.HostScriptRequestPayload{ + HostID: 1, + ScriptContents: "echo internal", + }) + require.NoError(t, err) + require.NotZero(t, res.ID) + require.Nil(t, res.UserID) + + // The internal-only filter on ListPendingHostScriptExecutions surfaces + // internal scripts; the default (all-pending) listing includes them + // alongside user-initiated ones. + pendingAll, err := ds.ListPendingHostScriptExecutions(ctx, 1, false) + require.NoError(t, err) + require.Len(t, pendingAll, 1) + require.Equal(t, res.ID, pendingAll[0].ID) + + pendingInternal, err := ds.ListPendingHostScriptExecutions(ctx, 1, true) + require.NoError(t, err) + require.Len(t, pendingInternal, 1) + require.Equal(t, res.ID, pendingInternal[0].ID) + + // A non-internal request should NOT appear under the internal-only filter, + // confirming the new entry routed through the internal codepath. + resUser, err := ds.NewHostScriptExecutionRequest(ctx, &fleet.HostScriptRequestPayload{ + HostID: 2, + ScriptContents: "echo user", + }) + require.NoError(t, err) + require.NotZero(t, resUser.ID) + + pendingUserViaInternal, err := ds.ListPendingHostScriptExecutions(ctx, 2, true) + require.NoError(t, err) + require.Empty(t, pendingUserViaInternal) +} diff --git a/server/fleet/conditional_access_idp.go b/server/fleet/conditional_access_idp.go index 5cddda8422..538da8f290 100644 --- a/server/fleet/conditional_access_idp.go +++ b/server/fleet/conditional_access_idp.go @@ -9,3 +9,23 @@ type ConditionalAccessIDPAssets struct{} func (c *ConditionalAccessIDPAssets) AuthzType() string { return "conditional_access_idp_assets" } + +// ConditionalAccessOktaProfileIdentifier is the top-level PayloadIdentifier +// of the .mobileconfig profile delivered for Okta conditional access. It is +// also the value stored in host_mdm_apple_profiles.profile_identifier for +// hosts that have received the profile, and is used to detect when an +// InstallProfile ack applies to the Okta CA profile. +const ConditionalAccessOktaProfileIdentifier = "com.fleetdm.conditional-access-okta" + +// ConditionalAccessOktaCertificateCN is the Subject CN of the SCEP +// certificate issued for Okta conditional access. The duplicate-cert +// cleanup script matches certificates in the keychain by this CN. +const ConditionalAccessOktaCertificateCN = "Fleet conditional access for Okta" + +// OktaCACleanupTarget identifies where to run the Okta conditional access +// keychain-cleanup script after a successful InstallProfile ack for the +// Okta CA profile. +type OktaCACleanupTarget struct { + HostID uint `db:"host_id"` + UserShortName string `db:"user_short_name"` +} diff --git a/server/fleet/datastore.go b/server/fleet/datastore.go index cc23cedeaa..7725f0df1e 100644 --- a/server/fleet/datastore.go +++ b/server/fleet/datastore.go @@ -490,6 +490,14 @@ type Datastore interface { // positive risk (one redundant CertificateList per false match). ProfileHasACMEPayloadForCommand(ctx context.Context, hostUUID, commandUUID string) (ProfileACMECommandResult, error) + // OktaCACleanupTargetForInstallCommand returns the host ID and target + // macOS short name needed to schedule the Okta conditional access + // keychain-cleanup script after a successful InstallProfile ack. The + // ok return is false when the command's profile is not the Okta CA + // profile, the host's platform is not darwin, or the host has no + // per-user MDM enrollment short name on record. + OktaCACleanupTargetForInstallCommand(ctx context.Context, hostUUID, commandUUID string) (OktaCACleanupTarget, bool, error) + // AreHostsConnectedToFleetMDM checks each host MDM enrollment with // this server and returns a map indexed by the host uuid and a boolean // indicating if the enrollment is active. @@ -2426,6 +2434,12 @@ type Datastore interface { // NewHostScriptExecutionRequest creates a new host script result entry with // just the script to run information (result is not yet available). NewHostScriptExecutionRequest(ctx context.Context, request *HostScriptRequestPayload) (*HostScriptResult, error) + // NewInternalHostScriptExecutionRequest is like NewHostScriptExecutionRequest + // but marks the request as internal (fleet-initiated), so it runs even when + // scripts are globally disabled and does not appear in the user-facing host + // activity feed. Use for server-driven follow-up actions (e.g. cleanup + // scripts after MDM events). + NewInternalHostScriptExecutionRequest(ctx context.Context, request *HostScriptRequestPayload) (*HostScriptResult, error) // SetHostScriptExecutionResult stores the result of a host script execution // return nil, "", nil. action is populated if this script was an MDM action (lock/unlock/wipe/uninstall). SetHostScriptExecutionResult(ctx context.Context, result *HostScriptResultPayload, attemptNumber *int) (hsr *HostScriptResult, action string, err error) diff --git a/server/mock/datastore_mock.go b/server/mock/datastore_mock.go index 8978e8a254..8aef32530d 100644 --- a/server/mock/datastore_mock.go +++ b/server/mock/datastore_mock.go @@ -358,6 +358,8 @@ type SoftDeleteMDMHostCertificatesForUnenrolledHostsFunc func(ctx context.Contex type ProfileHasACMEPayloadForCommandFunc func(ctx context.Context, hostUUID string, commandUUID string) (fleet.ProfileACMECommandResult, error) +type OktaCACleanupTargetForInstallCommandFunc func(ctx context.Context, hostUUID string, commandUUID string) (fleet.OktaCACleanupTarget, bool, error) + type AreHostsConnectedToFleetMDMFunc func(ctx context.Context, hosts []*fleet.Host) (map[string]bool, error) type AggregatedMunkiVersionFunc func(ctx context.Context, teamID *uint) ([]fleet.AggregatedMunkiVersion, time.Time, error) @@ -1436,6 +1438,8 @@ type SetOrUpdateMDMAppleDeclarationFunc func(ctx context.Context, declaration *f type NewHostScriptExecutionRequestFunc func(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) +type NewInternalHostScriptExecutionRequestFunc func(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) + type SetHostScriptExecutionResultFunc func(ctx context.Context, result *fleet.HostScriptResultPayload, attemptNumber *int) (hsr *fleet.HostScriptResult, action string, err error) type GetHostScriptExecutionResultFunc func(ctx context.Context, execID string) (*fleet.HostScriptResult, error) @@ -2560,6 +2564,9 @@ type DataStore struct { ProfileHasACMEPayloadForCommandFunc ProfileHasACMEPayloadForCommandFunc ProfileHasACMEPayloadForCommandFuncInvoked bool + OktaCACleanupTargetForInstallCommandFunc OktaCACleanupTargetForInstallCommandFunc + OktaCACleanupTargetForInstallCommandFuncInvoked bool + AreHostsConnectedToFleetMDMFunc AreHostsConnectedToFleetMDMFunc AreHostsConnectedToFleetMDMFuncInvoked bool @@ -4177,6 +4184,9 @@ type DataStore struct { NewHostScriptExecutionRequestFunc NewHostScriptExecutionRequestFunc NewHostScriptExecutionRequestFuncInvoked bool + NewInternalHostScriptExecutionRequestFunc NewInternalHostScriptExecutionRequestFunc + NewInternalHostScriptExecutionRequestFuncInvoked bool + SetHostScriptExecutionResultFunc SetHostScriptExecutionResultFunc SetHostScriptExecutionResultFuncInvoked bool @@ -6282,6 +6292,13 @@ func (s *DataStore) ProfileHasACMEPayloadForCommand(ctx context.Context, hostUUI return s.ProfileHasACMEPayloadForCommandFunc(ctx, hostUUID, commandUUID) } +func (s *DataStore) OktaCACleanupTargetForInstallCommand(ctx context.Context, hostUUID string, commandUUID string) (fleet.OktaCACleanupTarget, bool, error) { + s.mu.Lock() + s.OktaCACleanupTargetForInstallCommandFuncInvoked = true + s.mu.Unlock() + return s.OktaCACleanupTargetForInstallCommandFunc(ctx, hostUUID, commandUUID) +} + func (s *DataStore) AreHostsConnectedToFleetMDM(ctx context.Context, hosts []*fleet.Host) (map[string]bool, error) { s.mu.Lock() s.AreHostsConnectedToFleetMDMFuncInvoked = true @@ -10055,6 +10072,13 @@ func (s *DataStore) NewHostScriptExecutionRequest(ctx context.Context, request * return s.NewHostScriptExecutionRequestFunc(ctx, request) } +func (s *DataStore) NewInternalHostScriptExecutionRequest(ctx context.Context, request *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) { + s.mu.Lock() + s.NewInternalHostScriptExecutionRequestFuncInvoked = true + s.mu.Unlock() + return s.NewInternalHostScriptExecutionRequestFunc(ctx, request) +} + func (s *DataStore) SetHostScriptExecutionResult(ctx context.Context, result *fleet.HostScriptResultPayload, attemptNumber *int) (hsr *fleet.HostScriptResult, action string, err error) { s.mu.Lock() s.SetHostScriptExecutionResultFuncInvoked = true diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index c976098bbf..23514a0864 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -4180,6 +4180,15 @@ func (svc *MDMAppleCheckinAndCommandService) CommandAndReportResults(r *mdm.Requ svc.logger.WarnContext(r.Context, "queue CertificateList after ACME profile install", "err", err, "host_uuid", cmdResult.Identifier(), "command_uuid", cmdResult.CommandUUID) } + // Okta conditional access bundles a SCEP payload with an Identity + // Preference payload, which leaves the previous cert pinned in + // the per-user keychain across renewals. After a successful ack, + // queue an internal cleanup script that removes the orphaned + // duplicate. No-op when the profile isn't the Okta CA one. + if err := svc.maybeRunOktaCACleanupScript(r.Context, cmdResult.Identifier(), cmdResult.CommandUUID); err != nil { + svc.logger.WarnContext(r.Context, "run Okta CA keychain cleanup after profile install", + "err", err, "host_uuid", cmdResult.Identifier(), "command_uuid", cmdResult.CommandUUID) + } } return nil, nil case "RemoveProfile": diff --git a/server/service/apple_mdm_test.go b/server/service/apple_mdm_test.go index 1141b5ae3e..7eebff749e 100644 --- a/server/service/apple_mdm_test.go +++ b/server/service/apple_mdm_test.go @@ -2766,6 +2766,9 @@ func TestMDMCommandAndReportResultsProfileHandling(t *testing.T) { ds.ProfileHasACMEPayloadForCommandFunc = func(ctx context.Context, hUUID, cmdUUID string) (fleet.ProfileACMECommandResult, error) { return fleet.ProfileACMECommandResult{Platform: "ios"}, nil } + ds.OktaCACleanupTargetForInstallCommandFunc = func(ctx context.Context, hUUID, cmdUUID string) (fleet.OktaCACleanupTarget, bool, error) { + return fleet.OktaCACleanupTarget{}, false, nil + } _, err := svc.CommandAndReportResults( &mdm.Request{Context: ctx}, diff --git a/server/service/conditional_access_cleanup.go b/server/service/conditional_access_cleanup.go new file mode 100644 index 0000000000..efa9cf7c16 --- /dev/null +++ b/server/service/conditional_access_cleanup.go @@ -0,0 +1,83 @@ +package service + +import ( + "context" + _ "embed" + "fmt" + "regexp" + "strings" + + "github.com/fleetdm/fleet/v4/server/contexts/ctxerr" + "github.com/fleetdm/fleet/v4/server/fleet" +) + +// deleteDuplicateOktaSCEPScript is the macOS shell script that removes +// orphaned duplicate SCEP certificates left in the per-user keychain after +// the Okta conditional access profile is reinstalled or renewed. The +// canonical, customer-facing copy lives at +// docs/solutions/macos/scripts/delete-duplicate-scep-certificates.sh; the +// embed_sync test asserts the two stay byte-identical. +// +//go:embed embedded_scripts/delete-duplicate-scep-certificates.sh +var deleteDuplicateOktaSCEPScript string + +// macOS short names are 1-31 chars of ASCII letters, digits, and a small +// set of punctuation. We're conservative here because the value is +// interpolated into a shell command line; single-quote escaping covers the +// rest. Reject anything weird so a malformed nano_users row cannot reach +// the shell. +var validMacOSShortNameRE = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9_.-]*$`) + +// buildOktaCACleanupScript wraps the embedded cleanup script with the +// positional arguments it expects (auto-confirm, target user, certificate +// CN). Returns ok=false when the username fails validation, in which case +// the caller should log and skip rather than dispatch a malformed script. +func buildOktaCACleanupScript(username string) (string, bool) { + if len(username) == 0 || len(username) > 31 || !validMacOSShortNameRE.MatchString(username) { + return "", false + } + return fmt.Sprintf("#!/bin/bash\nset -- -y -u %s %s\n%s", + shellSingleQuote(username), + shellSingleQuote(fleet.ConditionalAccessOktaCertificateCN), + deleteDuplicateOktaSCEPScript, + ), true +} + +// shellSingleQuote returns s wrapped in single quotes, escaping any +// embedded single quotes via the POSIX 'foo'"'"'bar' idiom. +func shellSingleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'"'"'`) + "'" +} + +// maybeRunOktaCACleanupScript schedules the Okta conditional access +// keychain-cleanup script on the host after a successful InstallProfile +// ack for the Okta CA profile. It silently no-ops when the command does +// not apply to the Okta CA profile, when no per-user MDM enrollment short +// name is on record, or when the short name fails validation. Errors are +// returned to the caller for logging; the caller must not fail the ack +// path on them. +func (svc *MDMAppleCheckinAndCommandService) maybeRunOktaCACleanupScript(ctx context.Context, hostUUID, commandUUID string) error { + target, ok, err := svc.ds.OktaCACleanupTargetForInstallCommand(ctx, hostUUID, commandUUID) + if err != nil { + return ctxerr.Wrap(ctx, err, "look up Okta CA cleanup target") + } + if !ok { + return nil + } + + script, ok := buildOktaCACleanupScript(target.UserShortName) + if !ok { + svc.logger.DebugContext(ctx, "skip Okta CA keychain cleanup: invalid macOS username", + "host_uuid", hostUUID, "command_uuid", commandUUID) + return nil + } + + if _, err := svc.ds.NewInternalHostScriptExecutionRequest(ctx, &fleet.HostScriptRequestPayload{ + HostID: target.HostID, + ScriptContents: script, + SyncRequest: false, + }); err != nil { + return ctxerr.Wrap(ctx, err, "enqueue Okta CA keychain cleanup script") + } + return nil +} diff --git a/server/service/conditional_access_cleanup_test.go b/server/service/conditional_access_cleanup_test.go new file mode 100644 index 0000000000..c98e8162be --- /dev/null +++ b/server/service/conditional_access_cleanup_test.go @@ -0,0 +1,149 @@ +package service + +import ( + "context" + "errors" + "log/slog" + "os" + "strings" + "testing" + + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/mock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEmbeddedCleanupScriptMatchesDocs(t *testing.T) { + docs, err := os.ReadFile("../../docs/solutions/macos/scripts/delete-duplicate-scep-certificates.sh") + require.NoError(t, err, "the public Okta CA cleanup script must exist; if you moved it, update the test path") + require.Equal(t, string(docs), deleteDuplicateOktaSCEPScript, + "embedded_scripts/delete-duplicate-scep-certificates.sh has drifted from the docs copy; keep them in sync") +} + +func TestBuildOktaCACleanupScript(t *testing.T) { + cases := []struct { + name string + username string + wantOK bool + }{ + {"valid simple", "alice", true}, + {"valid with dot", "alice.smith", true}, + {"valid with underscore prefix", "_servicedesk", true}, + {"valid with dash", "alice-smith", true}, + {"valid mixed", "Alice.Smith-1", true}, + {"empty", "", false}, + {"starts with dash", "-alice", false}, + {"starts with dot", ".alice", false}, + {"contains space", "alice smith", false}, + {"contains slash", "alice/smith", false}, + {"contains single quote", "ali'ce", false}, + {"contains backtick", "ali`ce", false}, + {"contains semicolon", "alice;rm -rf /", false}, + {"contains shell metachar dollar", "ali$ce", false}, + {"too long", strings.Repeat("a", 32), false}, + {"exactly 31 chars", strings.Repeat("a", 31), true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + got, ok := buildOktaCACleanupScript(c.username) + require.Equal(t, c.wantOK, ok) + if !c.wantOK { + assert.Empty(t, got) + return + } + assert.Contains(t, got, "set -- -y -u '"+c.username+"' "+shellSingleQuote(fleet.ConditionalAccessOktaCertificateCN)) + assert.Contains(t, got, deleteDuplicateOktaSCEPScript, + "wrapped script must contain the embedded cleanup script verbatim") + }) + } +} + +func TestShellSingleQuote(t *testing.T) { + cases := map[string]string{ + "": "''", + "foo": "'foo'", + "foo bar": "'foo bar'", + "al'ce": `'al'"'"'ce'`, + "$danger`": "'$danger`'", + } + for in, want := range cases { + t.Run(in, func(t *testing.T) { + require.Equal(t, want, shellSingleQuote(in)) + }) + } +} + +func TestMaybeRunOktaCACleanupScript(t *testing.T) { + ctx := context.Background() + const ( + hostUUID = "host-uuid" + commandUUID = "cmd-uuid" + hostID = uint(99) + shortName = "alice" + ) + probeErrBoom := errors.New("boom") + + cases := []struct { + name string + target fleet.OktaCACleanupTarget + targetOK bool + targetErr error + wantErr bool + wantEnqueue bool + wantInvalidLog bool + }{ + { + name: "okta CA profile + valid user: enqueues", + target: fleet.OktaCACleanupTarget{HostID: hostID, UserShortName: shortName}, + targetOK: true, + wantEnqueue: true, + }, + { + name: "not okta CA profile: no enqueue", + targetOK: false, + }, + { + name: "lookup error: propagates", + targetErr: probeErrBoom, + wantErr: true, + }, + { + name: "invalid username: skipped without error", + target: fleet.OktaCACleanupTarget{HostID: hostID, UserShortName: "ali ce"}, + targetOK: true, + wantInvalidLog: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ds := new(mock.Store) + ds.OktaCACleanupTargetForInstallCommandFunc = func(_ context.Context, hUUID, cmdUUID string) (fleet.OktaCACleanupTarget, bool, error) { + require.Equal(t, hostUUID, hUUID) + require.Equal(t, commandUUID, cmdUUID) + return c.target, c.targetOK, c.targetErr + } + ds.NewInternalHostScriptExecutionRequestFunc = func(_ context.Context, req *fleet.HostScriptRequestPayload) (*fleet.HostScriptResult, error) { + require.Equal(t, hostID, req.HostID) + require.Contains(t, req.ScriptContents, "set -- -y -u '"+shortName+"' '"+fleet.ConditionalAccessOktaCertificateCN+"'") + require.Contains(t, req.ScriptContents, deleteDuplicateOktaSCEPScript) + require.False(t, req.SyncRequest) + require.Nil(t, req.UserID) + return &fleet.HostScriptResult{}, nil + } + + svc := &MDMAppleCheckinAndCommandService{ + ds: ds, + logger: slog.New(slog.DiscardHandler), + } + err := svc.maybeRunOktaCACleanupScript(ctx, hostUUID, commandUUID) + if c.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, c.wantEnqueue, ds.NewInternalHostScriptExecutionRequestFuncInvoked) + }) + } +} diff --git a/server/service/conditional_access_idp.go b/server/service/conditional_access_idp.go index a6ca3e9a58..acaefdfe29 100644 --- a/server/service/conditional_access_idp.go +++ b/server/service/conditional_access_idp.go @@ -392,7 +392,7 @@ func (svc *Service) ConditionalAccessGetIdPAppleProfile(ctx context.Context) (pr CACertBase64: caCertBase64, SCEPURL: scepURL, Challenge: challenge, - CertificateCN: "Fleet conditional access for Okta", + CertificateCN: fleet.ConditionalAccessOktaCertificateCN, MTLSURL: mtlsURL, CACertUUID: caCertUUID, SCEPPayloadUUID: scepPayloadUUID, diff --git a/server/service/conditional_access_idp_test.go b/server/service/conditional_access_idp_test.go index 720d1b7e87..a2384cded5 100644 --- a/server/service/conditional_access_idp_test.go +++ b/server/service/conditional_access_idp_test.go @@ -258,8 +258,11 @@ func TestConditionalAccessGetIdPAppleProfile(t *testing.T) { require.Contains(t, profileStr, "com.fleetdm.conditional-access-preference") require.Contains(t, profileStr, "com.fleetdm.chrome.certs") - // Verify certificate CN is present in the profile - require.Contains(t, profileStr, "Fleet conditional access for Okta") + // Top-level PayloadIdentifier and certificate CN must match the + // shared constants so the InstallProfile-ack cleanup hook can + // reliably gate on them. + require.Contains(t, profileStr, fleet.ConditionalAccessOktaProfileIdentifier) + require.Contains(t, profileStr, fleet.ConditionalAccessOktaCertificateCN) // Verify the renewal-ID marker is in the SCEP payload's Subject OU // so auto-renewal activates by default. Substituted to diff --git a/server/service/embedded_scripts/delete-duplicate-scep-certificates.sh b/server/service/embedded_scripts/delete-duplicate-scep-certificates.sh new file mode 100755 index 0000000000..5df7ceeca8 --- /dev/null +++ b/server/service/embedded_scripts/delete-duplicate-scep-certificates.sh @@ -0,0 +1,147 @@ +#!/bin/bash +# Deletes orphaned duplicate certificates matching a given CN from the login +# keychain, keeping the most recently issued one (the one tied to the current +# profile). +# +# Usage: ./delete-scep-certs.sh [-y] [-a] [-u username] +# -y Skip confirmation prompt +# -a Remove all matching certificates (including the newest) +# -u Target a specific user's login keychain (required when running as root) +# +# Example: ./delete-scep-certs.sh "Fleet conditional access for Okta" + +set -e + +auto_confirm=false +remove_all=false +target_user="" +while getopts "yau:" opt; do + case "$opt" in + y) auto_confirm=true ;; + a) remove_all=true ;; + u) target_user="$OPTARG" ;; + *) + echo "Usage: $0 [-y] [-a] [-u username] " >&2 + exit 1 + ;; + esac +done +shift $((OPTIND - 1)) + +if [ $# -eq 0 ]; then + echo "Usage: $0 [-y] [-a] [-u username] " >&2 + echo " -y Skip confirmation prompt" >&2 + echo " -a Remove all matching certificates (including the newest)" >&2 + echo " -u Target a specific user's login keychain (required when running as root)" >&2 + exit 1 +fi + +CN="$1" + +# Resolve the keychain path. +if [ -n "$target_user" ]; then + KEYCHAIN="/Users/$target_user/Library/Keychains/login.keychain-db" + if [ ! -f "$KEYCHAIN" ]; then + echo "Error: keychain not found at $KEYCHAIN" >&2 + exit 1 + fi +elif [ "$(id -u)" -eq 0 ]; then + echo "Error: running as root without -u flag. Specify the target user with -u ." >&2 + exit 1 +else + KEYCHAIN="login.keychain-db" +fi + +# Collect SHA-1 hash and Not Before date for every matching certificate. +# Output is written to a temp file as: +tmpfile=$(mktemp) +trap 'rm -f "$tmpfile" "$tmpfile.raw" "$tmpfile.err"' EXIT + +security find-certificate -a -c "$CN" -Z -p "$KEYCHAIN" >"$tmpfile.raw" 2>"$tmpfile.err" || true + +# Split the raw output into individual cert blocks and extract hash + date. +current_hash="" +current_pem="" +while IFS= read -r line; do + case "$line" in + "SHA-1 hash:"*) + current_hash=$(echo "$line" | awk '{print $NF}') + ;; + "-----BEGIN CERTIFICATE-----") + current_pem="$line"$'\n' + ;; + "-----END CERTIFICATE-----") + current_pem+="$line"$'\n' + not_before=$(echo "$current_pem" | openssl x509 -noout -startdate 2>/dev/null | cut -d= -f2) + epoch=$(date -j -f "%b %e %T %Y %Z" "$not_before" "+%s" 2>/dev/null || echo "0") + echo "$epoch $current_hash" >> "$tmpfile" + current_pem="" + ;; + *) + if [ -n "$current_pem" ]; then + current_pem+="$line"$'\n' + fi + ;; + esac +done < "$tmpfile.raw" + +total=$(wc -l < "$tmpfile" | tr -d ' ') + +if [ "$total" -eq 0 ]; then + echo "No certificates found matching \"$CN\"" + exit 0 +fi + +if [ "$total" -eq 1 ] && [ "$remove_all" = false ]; then + echo "Only one certificate found matching \"$CN\", nothing to delete." + exit 0 +fi + +# Sort by epoch descending; the first line is the newest. +newest_hash=$(sort -rn "$tmpfile" | head -1 | awk '{print $2}') + +if [ "$remove_all" = true ]; then + echo "Found $total certificate(s) matching \"$CN\"" + echo " Will delete: ALL $total certificate(s):" + while read -r epoch hash; do + issued=$(date -r "$epoch" "+%Y-%m-%d %H:%M:%S" 2>/dev/null || echo "unknown") + echo " $hash (issued $issued)" + done < "$tmpfile" +else + to_delete=$((total - 1)) + echo "Found $total certificate(s) matching \"$CN\"" + echo " Keeping newest: $newest_hash" + echo " Will delete: $to_delete orphaned certificate(s):" + while read -r epoch hash; do + if [ "$hash" = "$newest_hash" ]; then + continue + fi + issued=$(date -r "$epoch" "+%Y-%m-%d %H:%M:%S" 2>/dev/null || echo "unknown") + echo " $hash (issued $issued)" + done < "$tmpfile" +fi + +if [ "$auto_confirm" = false ]; then + printf "\nProceed? [y/N] " + read -r answer + if [ "$answer" != "y" ] && [ "$answer" != "Y" ]; then + echo "Aborted." + exit 0 + fi +fi + +deleted=0 +while read -r epoch hash; do + if [ "$remove_all" = false ] && [ "$hash" = "$newest_hash" ]; then + continue + fi + echo "Deleting $hash" + security delete-identity -Z "$hash" "$KEYCHAIN" + deleted=$((deleted + 1)) +done < "$tmpfile" + +if [ "$remove_all" = true ]; then + echo "Done. Deleted all $deleted certificate(s)." +else + echo "Done. Deleted $deleted orphaned certificate(s), kept 1." +fi