From 034691966f7b645d63802cf363499a6b8e8ee32a Mon Sep 17 00:00:00 2001 From: Magnus Jensen Date: Fri, 8 May 2026 11:02:22 +0200 Subject: [PATCH] check push cert staleness after 5 minutes of in-memory cache time (#44919) **Related issue:** Resolves #44376 I opted for an in-memory cache here, as it's not a critical cache piece, we are fine with the cache being different times on different containers (just means some might rotate to the correct cert faster than 5 minutes). It's also a small piece of work, rather than pulling in redis etc. Verified that it now logs, if the cert is stale after a 5 minute in-memory cache. ``` ts=2026-05-07T11:30:08Z level=info msg="push certificate is stale after re-checking" topic=com.apple.mgmt.External.34c4a9b0-6501-4ce6-afc6-32eac6420ee7 staleToken="\x90C\xe4K\xc6a\x97\xb5?\x1b\x9a\x04'\xe7b\x8d" newHash=".fP\xc7O7\xab\xab\x9d\x92\xd5#\xe4u\xe0\xf6" ts=2026-05-07T11:30:08Z level=info component=apple-mdm-push msg="retrieved push cert" topic=com.apple.mgmt.External.34c4a9b0-6501-4ce6-afc6-32eac6420ee7 ``` # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * APNs push certificates now refresh in-memory when rotated; staleness is detected using certificate checksums with a short grace window. * **Tests** * Added tests for certificate retrieval, staleness detection/refresh behavior, and push-cert storage error handling. * **Documentation** * Updated docs to describe the APNs push-certificate refresh and staleness behavior. --- .../44376-stale-apns-push-cert-not-updated | 1 + server/datastore/mysql/apple_mdm.go | 4 +- server/datastore/mysql/apple_mdm_test.go | 8 ++ server/datastore/mysql/nanomdm_storage.go | 64 ++++++++- .../datastore/mysql/nanomdm_storage_test.go | 131 ++++++++++++++++++ server/mdm/assets/assets.go | 27 +++- server/mdm/assets/assets_test.go | 17 ++- 7 files changed, 238 insertions(+), 14 deletions(-) create mode 100644 changes/44376-stale-apns-push-cert-not-updated diff --git a/changes/44376-stale-apns-push-cert-not-updated b/changes/44376-stale-apns-push-cert-not-updated new file mode 100644 index 0000000000..31937fd755 --- /dev/null +++ b/changes/44376-stale-apns-push-cert-not-updated @@ -0,0 +1 @@ +- Fixed an issue where an old APNs cert would stay in memory until a restart, instead of correctly updating in place. \ No newline at end of file diff --git a/server/datastore/mysql/apple_mdm.go b/server/datastore/mysql/apple_mdm.go index ed90ab4035..438f2d528a 100644 --- a/server/datastore/mysql/apple_mdm.go +++ b/server/datastore/mysql/apple_mdm.go @@ -6427,7 +6427,7 @@ func (ds *Datastore) GetAllMDMConfigAssetsByName(ctx context.Context, assetNames stmt := ` SELECT - name, value + name, value, HEX(md5_checksum) as md5_checksum FROM mdm_config_assets WHERE @@ -6459,7 +6459,7 @@ WHERE return nil, ctxerr.Wrapf(ctx, err, "decrypting mdm config asset %s", asset.Name) } - assetMap[asset.Name] = fleet.MDMConfigAsset{Name: asset.Name, Value: decryptedVal} + assetMap[asset.Name] = fleet.MDMConfigAsset{Name: asset.Name, Value: decryptedVal, MD5Checksum: asset.MD5Checksum} } if len(res) < len(assetNames) { diff --git a/server/datastore/mysql/apple_mdm_test.go b/server/datastore/mysql/apple_mdm_test.go index e43927463c..2cb81be409 100644 --- a/server/datastore/mysql/apple_mdm_test.go +++ b/server/datastore/mysql/apple_mdm_test.go @@ -7159,6 +7159,10 @@ func testMDMConfigAsset(t *testing.T, ds *Datastore) { a, err := ds.GetAllMDMConfigAssetsByName(ctx, []fleet.MDMAssetName{fleet.MDMAssetCACert, fleet.MDMAssetCAKey}, nil) require.NoError(t, err) + for key, asset := range a { + asset.MD5Checksum = "" + a[key] = asset + } require.Equal(t, wantAssets, a) h, err := ds.GetAllMDMConfigAssetsHashes(ctx, []fleet.MDMAssetName{fleet.MDMAssetCACert, fleet.MDMAssetCAKey}) @@ -7210,6 +7214,10 @@ func testMDMConfigAsset(t *testing.T, ds *Datastore) { a, err = ds.GetAllMDMConfigAssetsByName(ctx, []fleet.MDMAssetName{fleet.MDMAssetCACert, fleet.MDMAssetCAKey}, ds.reader(ctx)) require.NoError(t, err) + for key, asset := range a { + asset.MD5Checksum = "" + a[key] = asset + } require.Equal(t, wantNewAssets, a) h, err = ds.GetAllMDMConfigAssetsHashes(ctx, []fleet.MDMAssetName{fleet.MDMAssetCACert, fleet.MDMAssetCAKey}) diff --git a/server/datastore/mysql/nanomdm_storage.go b/server/datastore/mysql/nanomdm_storage.go index 6e8fb96c04..c2e0ead15a 100644 --- a/server/datastore/mysql/nanomdm_storage.go +++ b/server/datastore/mysql/nanomdm_storage.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "strings" + "sync" "time" abmctx "github.com/fleetdm/fleet/v4/server/contexts/apple_bm" @@ -99,23 +100,78 @@ func (ds *Datastore) NewTestMDMAppleMDMStorage(asyncCap int, asyncInterval time. }, nil } +type pushCertStalenessCheck struct { + hash string + updatedAt time.Time +} + +// We store staleness check in-memory since it's a short-lived 5 minute time window. +// And it also means some containers might rotate it faster than 5 minutes depending on the time. +var ( + pushCertStaleness *pushCertStalenessCheck + pushCertStalenessMu sync.RWMutex +) + // RetrievePushCert partially implements nanomdm_storage.PushCertStore. // -// Always returns "0" as stale token because fleet.Datastore always returns a valid push certificate. +// Returns the push certificate and its MD5 checksum as the stale token. func (s *NanoMDMStorage) RetrievePushCert( ctx context.Context, topic string, ) (*tls.Certificate, string, error) { - cert, err := assets.APNSKeyPair(ctx, s.ds) + cert, checksum, err := assets.APNSKeyPair(ctx, s.ds) if err != nil { return nil, "", ctxerr.Wrap(ctx, err, "loading push certificate") } - return cert, "0", nil + pushCertStalenessMu.Lock() + defer pushCertStalenessMu.Unlock() + checkInMemoryHash(checksum) + return cert, checksum, nil +} + +// checkInMemoryHash checks the incoming hash agains the in-memory hash. +// if criteria is met, it updates the in-memory hash with the new hash and updatedAt = now. +func checkInMemoryHash(hash string) { + if pushCertStaleness == nil || pushCertStaleness.hash != hash || time.Since(pushCertStaleness.updatedAt) > 5*time.Minute { + // We will not call this unless we are stale, OR on new topic getting a provider, which means we should be fine to update here. + // Update on new hash, or if it's been more than 5 minutes since last update, to avoid fetching the cert on each stale check. + pushCertStaleness = &pushCertStalenessCheck{ + hash: hash, + updatedAt: time.Now(), + } + } } // IsPushCertStale partially implements nanomdm_storage.PushCertStore. // -// Always returns `false` because the underlying datastore implementation makes sure that the token is always fresh. +// Checks the provided stale token against the in-memory hash of the current push certificate. If they differ, the cert is stale. +// If the token is the same, it checks if the certificate was last updated more than 5 minutes ago. If so, it re-fetches the certificate and updates the hash for future checks. func (s *NanoMDMStorage) IsPushCertStale(ctx context.Context, topic, staleToken string) (bool, error) { + pushCertStalenessMu.RLock() + staleness := pushCertStaleness + pushCertStalenessMu.RUnlock() + if staleness == nil { + return true, nil + } + if staleness.hash != staleToken { + s.logger.InfoContext(ctx, "push certificate is stale", "topic", topic, "staleToken", staleToken, "currentHash", staleness.hash, "updatedAt", staleness.updatedAt) + return true, nil + } + + // If updated at is more than 5 minutes ago, re-fetch and re-calculate the has for staleness + if time.Since(staleness.updatedAt) > 5*time.Minute { + _, checksum, err := assets.APNSKeyPair(ctx, s.ds) + if err != nil { + return false, fmt.Errorf("loading push certificate for staleness check: %w", err) + } + pushCertStalenessMu.Lock() + defer pushCertStalenessMu.Unlock() + checkInMemoryHash(checksum) + if checksum != staleToken { + s.logger.InfoContext(ctx, "push certificate is stale after re-checking", "topic", topic, "staleToken", staleToken, "newHash", checksum) + return true, nil + } + } + return false, nil } diff --git a/server/datastore/mysql/nanomdm_storage_test.go b/server/datastore/mysql/nanomdm_storage_test.go index eac453303b..d3bf9c7acc 100644 --- a/server/datastore/mysql/nanomdm_storage_test.go +++ b/server/datastore/mysql/nanomdm_storage_test.go @@ -12,9 +12,11 @@ import ( "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm" + mdmtesting "github.com/fleetdm/fleet/v4/server/mdm/testing_utils" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/test" "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -29,6 +31,9 @@ func TestNanoMDMStorage(t *testing.T) { {"TestEnqueueDeviceLockCommandRaceCondition", testEnqueueDeviceLockCommandRaceCondition}, {"TestEnqueueDeviceUnlockCommand", testEnqueueDeviceUnlockCommand}, {"TestStoreAuthenticatePreservesBootstrapTokenDuringSCEPRenewal", testStoreAuthenticatePreservesBootstrapTokenDuringSCEPRenewal}, + {"TestRetrievePushCert", testRetrievePushCert}, + {"TestIsPushCertStale", testIsPushCertStale}, + {"TestStorePushCert", testStorePushCert}, } for _, c := range cases { @@ -493,3 +498,129 @@ func testEnqueueDeviceLockCommandRaceCondition(t *testing.T, ds *Datastore) { require.Len(t, pins, 1, "Only one PIN should be generated") require.Equal(t, pins[0], storedPIN, "Stored PIN should match the successful request") } + +func testRetrievePushCert(t *testing.T, ds *Datastore) { + ctx := t.Context() + ns, err := ds.NewMDMAppleMDMStorage() + require.NoError(t, err) + + t.Cleanup(func() { + _ = ds.HardDeleteMDMConfigAsset(ctx, fleet.MDMAssetAPNSCert) + _ = ds.HardDeleteMDMConfigAsset(ctx, fleet.MDMAssetAPNSKey) + pushCertStaleness = nil + }) + + apnsCert, apnsKey, err := GenerateTestCertBytes(mdmtesting.NewTestMDMAppleCertTemplate()) + require.NoError(t, err) + + err = ds.InsertMDMConfigAssets(ctx, []fleet.MDMConfigAsset{ + {Name: fleet.MDMAssetAPNSCert, Value: apnsCert}, + {Name: fleet.MDMAssetAPNSKey, Value: apnsKey}, + }, nil) + require.NoError(t, err) + + cert, hash, err := ns.RetrievePushCert(ctx, "com.apple.mgmt.test") + require.NoError(t, err) + require.NotNil(t, cert) + require.NotEmpty(t, hash) + require.NotNil(t, pushCertStaleness) + require.Equal(t, hash, pushCertStaleness.hash) + assert.WithinDuration(t, time.Now(), pushCertStaleness.updatedAt, 500*time.Millisecond) + oldUpdatedAt := pushCertStaleness.updatedAt + + // Retrieve again with same cert - should not update staleness + cert2, hash2, err := ns.RetrievePushCert(ctx, "com.apple.mgmt.test") + require.NoError(t, err) + require.NotNil(t, cert2) + require.Equal(t, hash, hash2) + require.Equal(t, oldUpdatedAt, pushCertStaleness.updatedAt) + stalenessHash := pushCertStaleness.hash + + // Insert a new cert with different content to simulate cert rotation + newApnsCert, newApnsKey, err := GenerateTestCertBytes(mdmtesting.NewTestMDMAppleCertTemplate()) + require.NoError(t, err) + require.NoError(t, ds.InsertOrReplaceMDMConfigAsset(ctx, fleet.MDMConfigAsset{Name: fleet.MDMAssetAPNSCert, Value: newApnsCert})) + require.NoError(t, ds.InsertOrReplaceMDMConfigAsset(ctx, fleet.MDMConfigAsset{Name: fleet.MDMAssetAPNSKey, Value: newApnsKey})) + + cert3, hash3, err := ns.RetrievePushCert(ctx, "com.apple.mgmt.test") + require.NoError(t, err) + require.NotNil(t, cert3) + require.NotEqual(t, hash, hash3) + require.Equal(t, hash3, pushCertStaleness.hash) + assert.WithinDuration(t, time.Now(), pushCertStaleness.updatedAt, 500*time.Millisecond) + require.NotEqual(t, oldUpdatedAt, pushCertStaleness.updatedAt) + require.NotEqual(t, stalenessHash, pushCertStaleness.hash) +} + +func testIsPushCertStale(t *testing.T, ds *Datastore) { + ctx := t.Context() + ns, err := ds.NewMDMAppleMDMStorage() + require.NoError(t, err) + t.Cleanup(func() { + _ = ds.HardDeleteMDMConfigAsset(ctx, fleet.MDMAssetAPNSCert) + _ = ds.HardDeleteMDMConfigAsset(ctx, fleet.MDMAssetAPNSKey) + pushCertStaleness = nil + }) + + // Initially there is no cert, so it should be considered stale + stale, err := ns.IsPushCertStale(ctx, "com.apple.mgmt.test", "nonexistent-token") + require.NoError(t, err) + require.True(t, stale) + + // Insert a cert + apnsCert, apnsKey, err := GenerateTestCertBytes(mdmtesting.NewTestMDMAppleCertTemplate()) + require.NoError(t, err) + + err = ds.InsertMDMConfigAssets(ctx, []fleet.MDMConfigAsset{ + {Name: fleet.MDMAssetAPNSCert, Value: apnsCert}, + {Name: fleet.MDMAssetAPNSKey, Value: apnsKey}, + }, nil) + require.NoError(t, err) + + // Retrieve the cert to get the current hash + cert, hash, err := ns.RetrievePushCert(ctx, "com.apple.mgmt.test") + require.NoError(t, err) + require.NotNil(t, cert) + require.NotEmpty(t, hash) + + // Check staleness with correct token - should not be stale + stale, err = ns.IsPushCertStale(ctx, "com.apple.mgmt.test", hash) + require.NoError(t, err) + require.False(t, stale) + + // Check staleness with incorrect token - should be stale + stale, err = ns.IsPushCertStale(ctx, "com.apple.mgmt.test", "invalid-token") + require.NoError(t, err) + require.True(t, stale) + + // Insert a new cert to simulate rotation + newApnsCert, newApnsKey, err := GenerateTestCertBytes(mdmtesting.NewTestMDMAppleCertTemplate()) + require.NoError(t, err) + require.NoError(t, ds.InsertOrReplaceMDMConfigAsset(ctx, fleet.MDMConfigAsset{Name: fleet.MDMAssetAPNSCert, Value: newApnsCert})) + require.NoError(t, ds.InsertOrReplaceMDMConfigAsset(ctx, fleet.MDMConfigAsset{Name: fleet.MDMAssetAPNSKey, Value: newApnsKey})) + + // Check staleness with old token - should not be stale since under 5 minutes + stale, err = ns.IsPushCertStale(ctx, "com.apple.mgmt.test", hash) + require.NoError(t, err) + require.False(t, stale, "We allow the wrong cert for up to 5 minutes after rotation") + require.WithinDuration(t, time.Now(), pushCertStaleness.updatedAt, 5*time.Minute) + + // Fake 5 minutes passing + pushCertStaleness.updatedAt = time.Now().Add(-6 * time.Minute) + + // Check staleness with old token - should be stale since cert is now old + stale, err = ns.IsPushCertStale(ctx, "com.apple.mgmt.test", hash) + require.NoError(t, err) + require.True(t, stale) +} + +// ensure we always use our custom MDM config assets impl. +func testStorePushCert(t *testing.T, ds *Datastore) { + ctx := t.Context() + ns, err := ds.NewMDMAppleMDMStorage() + require.NoError(t, err) + + err = ns.StorePushCert(ctx, nil, nil) + require.Error(t, err) + require.Equal(t, "please use fleet.Datastore to manage MDM assets", err.Error()) +} diff --git a/server/mdm/assets/assets.go b/server/mdm/assets/assets.go index 218b3132fc..32d7fb01f2 100644 --- a/server/mdm/assets/assets.go +++ b/server/mdm/assets/assets.go @@ -18,8 +18,8 @@ func CAKeyPair(ctx context.Context, ds fleet.MDMAssetRetriever) (*tls.Certificat return KeyPair(ctx, ds, fleet.MDMAssetCACert, fleet.MDMAssetCAKey) } -func APNSKeyPair(ctx context.Context, ds fleet.MDMAssetRetriever) (*tls.Certificate, error) { - return KeyPair(ctx, ds, fleet.MDMAssetAPNSCert, fleet.MDMAssetAPNSKey) +func APNSKeyPair(ctx context.Context, ds fleet.MDMAssetRetriever) (*tls.Certificate, string, error) { + return KeyPairWithMD5(ctx, ds, fleet.MDMAssetAPNSCert, fleet.MDMAssetAPNSKey) } func KeyPair(ctx context.Context, ds fleet.MDMAssetRetriever, certName, keyName fleet.MDMAssetName) (*tls.Certificate, error) { @@ -44,6 +44,29 @@ func KeyPair(ctx context.Context, ds fleet.MDMAssetRetriever, certName, keyName return &cert, nil } +// KeyPairWithMD5 returns the certificate from the keypair, along with the MD5 checksum of the certificate. +func KeyPairWithMD5(ctx context.Context, ds fleet.MDMAssetRetriever, certName, keyName fleet.MDMAssetName) (*tls.Certificate, string, error) { + assets, err := ds.GetAllMDMConfigAssetsByName(ctx, []fleet.MDMAssetName{ + certName, + keyName, + }, nil) + if err != nil { + return nil, "", fmt.Errorf("loading %s, %s keypair from the database: %w", certName, keyName, err) + } + + cert, err := tls.X509KeyPair(assets[certName].Value, assets[keyName].Value) + if err != nil { + return nil, "", fmt.Errorf("parsing %s, %s keypair: %w", certName, keyName, err) + } + + cert.Leaf, err = x509.ParseCertificate(cert.Certificate[0]) + if err != nil { + return nil, "", fmt.Errorf("parsing %s certificate leaf: %w", certName, err) + } + + return &cert, assets[certName].MD5Checksum, nil +} + func X509Cert(ctx context.Context, ds fleet.MDMAssetRetriever, certName fleet.MDMAssetName) (*x509.Certificate, error) { assets, err := ds.GetAllMDMConfigAssetsByName(ctx, []fleet.MDMAssetName{certName}, nil) if err != nil { diff --git a/server/mdm/assets/assets_test.go b/server/mdm/assets/assets_test.go index 00151ab23e..dec18ea179 100644 --- a/server/mdm/assets/assets_test.go +++ b/server/mdm/assets/assets_test.go @@ -72,7 +72,8 @@ func TestCAKeyPair(t *testing.T) { } ds.GetAllMDMConfigAssetsByNameFunc = func(ctx context.Context, assetNames []fleet.MDMAssetName, - _ sqlx.QueryerContext) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { + _ sqlx.QueryerContext, + ) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { require.ElementsMatch(t, []fleet.MDMAssetName{fleet.MDMAssetCACert, fleet.MDMAssetCAKey}, assetNames) return assets, nil } @@ -95,11 +96,12 @@ func TestAPNSKeyPair(t *testing.T) { fleet.MDMAssetAPNSKey: {Value: keyPEM}, } ds.GetAllMDMConfigAssetsByNameFunc = func(ctx context.Context, assetNames []fleet.MDMAssetName, - _ sqlx.QueryerContext) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { + _ sqlx.QueryerContext, + ) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { require.ElementsMatch(t, []fleet.MDMAssetName{fleet.MDMAssetAPNSCert, fleet.MDMAssetAPNSKey}, assetNames) return assets, nil } - cert, err := APNSKeyPair(ctx, ds) + cert, _, err := APNSKeyPair(ctx, ds) require.NoError(t, err) require.NotNil(t, cert) require.True(t, ds.GetAllMDMConfigAssetsByNameFuncInvoked) @@ -116,7 +118,8 @@ func TestX509Cert(t *testing.T) { fleet.MDMAssetAPNSCert: {Value: certPEM}, } ds.GetAllMDMConfigAssetsByNameFunc = func(ctx context.Context, assetNames []fleet.MDMAssetName, - _ sqlx.QueryerContext) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { + _ sqlx.QueryerContext, + ) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { require.ElementsMatch(t, []fleet.MDMAssetName{fleet.MDMAssetAPNSCert}, assetNames) return assets, nil } @@ -138,7 +141,8 @@ func TestAPNSTopic(t *testing.T) { fleet.MDMAssetAPNSCert: {Value: certPEM}, } ds.GetAllMDMConfigAssetsByNameFunc = func(ctx context.Context, assetNames []fleet.MDMAssetName, - _ sqlx.QueryerContext) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { + _ sqlx.QueryerContext, + ) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { require.ElementsMatch(t, []fleet.MDMAssetName{fleet.MDMAssetAPNSCert}, assetNames) return assets, nil } @@ -195,7 +199,8 @@ func TestABMToken(t *testing.T) { fleet.MDMAssetABMKey: {Value: keyPEM}, } ds.GetAllMDMConfigAssetsByNameFunc = func(ctx context.Context, assetNames []fleet.MDMAssetName, - _ sqlx.QueryerContext) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { + _ sqlx.QueryerContext, + ) (map[fleet.MDMAssetName]fleet.MDMConfigAsset, error) { require.ElementsMatch(t, []fleet.MDMAssetName{ fleet.MDMAssetABMCert, fleet.MDMAssetABMKey,