CRLite remote-settings downloads aren't cleaned up
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
People
(Reporter: dveditz, Assigned: jschanck)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files)
(deleted),
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-phabricator-request
|
dmeehan
:
approval-mozilla-beta+
|
Details |
It appears that the remote-settings CRLite downloads are not cleaned up after they're processed: I've got nearly 1Gb of -filter
and -filter.stash
files in my ~/Library/Caches/Firefox/Profiles/xxxxx/settings/security-state/cert-revocations/
directory going back to September 2020.
I don't see a lot of other remote-settings files left in this "cache" spot, and in particular I don't see addon blocklist files which I believe are now using crlite technology.
This came to light from a fenix issue about app data growing even after you clear all the stored user browsing data (history, cache, cookies, etc). A couple of participants identified the cert-revocations directories as huge (480Mb). On Android these files are in the regular profile, not in the separate caches directory. CRLite is not enabled on Fenix, and about:config
only exists on Nightly, but the participants who identified this were running Mull, a Firefox-derived browser using Arkenfox user.js settings. [FWIW I don't think this is the original Fenix reporter's issue]
Reporter | ||
Comment 1•2 years ago
|
||
On slack, @leplatrem says "
The remote-settings client code seems to delete stuff when records are deleted on the server: https://searchfox.org/mozilla-central/rev/b4150d1c6fae0c51c522df2d2c939cf5ad331d4c/security/manager/ssl/RemoteSecuritySettings.jsm#411
Maybe we need to clean up on the remote-settings server side?
Reporter | ||
Comment 2•2 years ago
|
||
Looks like CRLite is using a now-deprecated remote-settings feature and running into the problems the addons blocklist did (see bug 1634127).
Assignee | ||
Comment 3•2 years ago
|
||
Updated•2 years ago
|
Updated•2 years ago
|
Comment 5•2 years ago
|
||
The RemoteSecuritySettings.jsm file doesn't have any logic to delete cached attachments. Replacing downloadToDisk with download doesn't fix that underlying problem. It will just move the junk from files to IndexedDB.
Use deleteDownloaded at the right time to delete attachments for stale records, i.e. https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/Attachments.jsm#273
(In reply to Daniel Veditz [:dveditz] from comment #2)
Looks like CRLite is using a now-deprecated remote-settings feature and running into the problems the addons blocklist did (see bug 1634127).
In the Blocklist case, a fixed attachmentId is used so there is only one stored attachment. In this crlite case the attachmentId is not explicitly set thus derived from the record id. An arbitrary number of attachments exists, and will continue to pile up without explicit cleanup logic.
Please fix this and add unit tests to verify that there are no lingering attachments.
:leplatrem: Could it make sense to add helpers / options to the RemoteSettingsClient or downloader to support the intended behavior of keeping attachments in sync with the stored records? I.e. delete attachments when no record references it any more. That would support the use case from this bug.
Comment 7•2 years ago
|
||
This is correct, before, with the downloadToDisk()
method, the attachments were stored as files using the filename
field. Now that the code uses the IndexedDB's based implementation, the attachments are stored "per-record".
If you would want updates to overwrite previous attachments, you have to specify the attachmentId
option for download()
like addons blocklist does (default is record.id
).
:leplatrem: Could it make sense to add helpers / options to the RemoteSettingsClient or downloader to support the intended behavior of keeping attachments in sync with the stored records? I.e. delete attachments when no record references it any more. That would support the use case from this bug.
In Bug 1763626, we want to remove downloaded attachments that don't match any record (probably comparing their hash or something similar...). Seeing this bug, I realize its priority has to be raised and its implementation planned.
Comment 8•2 years ago
|
||
(In reply to Mathieu Leplatre [:leplatrem] from comment #7)
This is correct, before, with the
downloadToDisk()
method, the attachments were stored as files using thefilename
field. Now that the code uses the IndexedDB's based implementation, the attachments are stored "per-record".If you would want updates to overwrite previous attachments, you have to specify the
attachmentId
option fordownload()
like addons blocklist does (default isrecord.id
).
This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the attachmentId
. So the minimum that the implementation must do is to call deleteDownloaded
when a removed record is observed during the sync
event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124
Assignee | ||
Comment 9•2 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #8)
This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the
attachmentId
. So the minimum that the implementation must do is to calldeleteDownloaded
when a removed record is observed during thesync
event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124
At any given time CRLite's cert-revocations
collection has about 10 records with attachments, and each record persists for no more than 10 days. My understanding is that the default caching mechanism for the download
function will automatically delete attachments for records that have been deleted. But I'm happy to add some unit tests to confirm this.
Comment 10•2 years ago
|
||
(In reply to John Schanck [:jschanck] from comment #9)
(In reply to Rob Wu [:robwu] from comment #8)
This only works if the implementation has a known limited number of attachments. In the CRLite use case, I think that the number is arbitrary and it's not feasible to choose the
attachmentId
. So the minimum that the implementation must do is to calldeleteDownloaded
when a removed record is observed during thesync
event, e.g. as documented at https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/services/settings/docs/index.rst#116-124At any given time CRLite's
cert-revocations
collection has about 10 records with attachments, and each record persists for no more than 10 days.
I see that the record ids at https://firefox.settings.services.mozilla.com/v1/buckets/security-state/collections/cert-revocations/records are random UUIDs. They are not fixed, so the storage used for the attachments is unbounded.
My understanding is that the default caching mechanism for the
download
function will automatically delete attachments for records that have been deleted.
This understanding is incorrect. The download
method caches the attachment which is used if download is called again for the same record. The downloaded data is currently not actively managed by the RemoteSettings backend, it is the responsibility of the client user to overwrite or delete the data if needed, with the deleteDownloaded
method (as mentioned before). Bug 1763626 is a feature request about offering functionality to delete stale attachments, but that is still open.
Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate cert_storage
component. If the data is unlikely to be used after that, then you could just use downloadAsBytes
instead of download
.
But I'm happy to add some unit tests to confirm this.
I expect these unit tests to fail with the current implementation.
Assignee | ||
Comment 11•2 years ago
|
||
Thanks for the clarification. I've updated the patch to manually delete the downloads.
(In reply to Rob Wu [:robwu] from comment #10)
Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate
cert_storage
component. If the data is unlikely to be used after that, then you could just usedownloadAsBytes
instead ofdownload
.
I was inclined to use downloadAsBytes
, but I think that the original author was concerned about a second round of downloads beginning before the loaded_into_cert_storage
flags were set. This seems plausible to me so I kept it as is.
Comment 12•2 years ago
|
||
(In reply to John Schanck [:jschanck] from comment #11)
Thanks for the clarification. I've updated the patch to manually delete the downloads.
(In reply to Rob Wu [:robwu] from comment #10)
Does CRLite actually need to cache the attachment? It looks like the data is immediately imported in a separate
cert_storage
component. If the data is unlikely to be used after that, then you could just usedownloadAsBytes
instead ofdownload
.I was inclined to use
downloadAsBytes
, but I think that the original author was concerned about a second round of downloads beginning before theloaded_into_cert_storage
flags were set. This seems plausible to me so I kept it as is.
The logic was originally introduced in this patch to bug 1563056 at https://searchfox.org/mozilla-central/diff/ba0c7e0e3aa435144b98f216e9eae20cdc8883ed/security/manager/ssl/RemoteSecuritySettings.jsm#786
In that patch, there was no cert_storage backend, so the reliance on RemoteSettings' cache was very important.
Now there is a storage backend, so the question is whether it's truly necessary to persist the data to disk (whether as a file or in IndexedDB), or whether it suffices to just get the bytes instead. Maybe even with a temporary in-memory data structure if you are concerned about downloading twice in a short time span.
Dana, since you authored the original patch: what do you recommend here?
Bug 1677399 implemented a way to keep track of what we've downloaded and added to cert_storage. Keeping a copy in the cache seems redundant to me (the only way it would be useful is if cert_storage's data got corrupted/deleted while the cache was still ok).
Comment 15•2 years ago
|
||
When there is a signature verification error, which sometimes happens, the local data are deleted. And the markers that are put on records (loaded_into_cert_storage: true
) are gone.
The attachments are thus all re-downloaded when the synchronization is retried.
I suspect that this is one of the reasons why our CDN gets massively hammered by attachments downloads for security-settings. I started to take notes about that https://docs.google.com/document/d/100Yv1kPRcyXlQ29SqAWrTU3c2R_P5q44IR2Mlui9FwE/edit#
I propose that we keep the cache. And more, that we add it to other collections like intermediates
.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 16•2 years ago
|
||
I don't think there is a robust enough caching option for our use case at the moment. I've opened Bug 1796190 to re-investigate caching once there are better options, e.g. the mechanism from Bug 1763626.
Comment 17•2 years ago
|
||
Pushed by jschanck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/747e24d0339e part 1. avoid deprecated downloadToDisk function. r=keeler,leplatrem,robwu https://hg.mozilla.org/integration/autoland/rev/ac705dd27e0a part 2. clean cert-revocations attachment cache. r=keeler,leplatrem,robwu
Assignee | ||
Comment 18•2 years ago
|
||
[Tracking Requested - why for this release]: We're planning a staged rollout of CRLite in the 107 release, which will involve flipping the security.remote_settings.crlite_filters.enabled
pref for some users after 107 goes to release. We need to uplift part 1 of the patch series to 107 beta to ensure we don't later pollute 107 release profiles. Part 2 is optional as release users who have not had the pref set in the past will not have stale filters in their cache.
Updated•2 years ago
|
Comment 19•2 years ago
|
||
Backed out for causing failures on test_crlite_filters.js
- backout: https://hg.mozilla.org/integration/autoland/rev/c537f98572d625fe8ed52a9ed912d06412f0df45
- push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=ac705dd27e0a49bb8d22a1f7d0b76a144926d8d5&group_state=expanded
- failure log: https://treeherder.mozilla.org/logviewer?job_id=393851322&repo=autoland&lineNumber=2429
[task 2022-10-20T17:22:35.899Z] 17:22:35 INFO - TEST-START | security/manager/ssl/tests/unit/test_crlite_filters.js
[task 2022-10-20T17:22:35.940Z] 17:22:35 INFO - adb launch_application: am startservice -W -n 'org.mozilla.geckoview.test_runner/org.mozilla.geckoview.test_runner.XpcshellTestRunnerService$i0' -a android.intent.action.MAIN --es env0 XPCOM_DEBUG_BREAK=stack-and-abort --es env1 MOZ_CRASHREPORTER=1 --es env2 MOZ_CRASHREPORTER_NO_REPORT=1 --es env3 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env4 MOZ_DEVELOPER_REPO_DIR=/builds/worker/checkouts/gecko --es env5 MOZ_DISABLE_CONTENT_SANDBOX=1 --es env6 MOZ_FETCHES_DIR=/builds/worker/fetches --es env7 MOZ_DISABLE_SOCKET_PROCESS=1 --es env8 LD_LIBRARY_PATH=/data/local/tmp/test_root/xpcb --es env9 MOZ_LINKER_CACHE=/data/local/tmp/test_root/xpcb --es env10 GRE_HOME=/data/local/tmp/test_root/xpcb --es env11 XPCSHELL_TEST_PROFILE_DIR=/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es env12 HOME=/data/local/tmp/test_root/xpc/p --es env13 XPCSHELL_TEST_TEMP_DIR=/data/local/tmp/test_root/xpc/tmp/abde7069-81b1-46f5-9edc-c4e9b27d8b3a --es env14 MOZ_ANDROID_DATA_DIR=/data/local/tmp/test_root/xpcb --es env15 MOZ_IN_AUTOMATION=1 --es env16 MOZ_ANDROID_CPU_ABI=x86_64 --es env17 MOZHTTP2_PORT=45707 --es env18 MOZNODE_EXEC_PORT=44879 --es env19 TMPDIR=/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es env20 XPCSHELL_MINIDUMP_DIR=/data/local/tmp/test_root/xpc/minidumps/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38 --es arg0 -g --es arg1 /data/local/tmp/test_root/xpcb --es arg2 --greomni --es arg3 /data/local/tmp/test_root/xpcb/geckoview-test_runner.apk --es arg4 -m --es arg5 -e --es arg6 'const _HEAD_JS_PATH = "/data/local/tmp/test_root/xpc/head.js";' --es arg7 -e --es arg8 'const _MOZINFO_JS_PATH = "/data/local/tmp/test_root/xpc/p/e0a5cbe6-80f0-41d8-9bd1-621c2736bc38/mozinfo.json";' --es arg9 -e --es arg10 'const _PREFS_FILE = "/data/local/tmp/test_root/xpc/user.js";' --es arg11 -e --es arg12 'const _TESTING_MODULES_DIR = "/data/local/tmp/test_root/xpc/m";' --es arg13 -f --es arg14 /data/local/tmp/test_root/xpc/head.js --es arg15 -e --es arg16 'const _HEAD_FILES = ["/data/local/tmp/test_root/xpc/security/manager/ssl/tests/unit/head_psm.js"];' --es arg17 -e --es arg18 'const _JSDEBUGGER_PORT = 0;' --es arg19 -e --es arg20 'const _TEST_CWD = "/data/local/tmp/test_root/xpc/security/manager/ssl/tests/unit";' --es arg21 -e --es arg22 'const _TEST_FILE = ["test_crlite_filters.js"];' --es arg23 -e --es arg24 'const _TEST_NAME = "security/manager/ssl/tests/unit/test_crlite_filters.js";' --es arg25 -e --es arg26 '_execute_test(); quit(0);' --ez use_multiprocess True --es out_file /data/local/tmp/test_root/xpc/logs/xpcshell-cd40dc18-463b-4cd3-b1f9-0bb34a132051.log
[task 2022-10-20T17:22:36.175Z] 17:22:36 INFO - remotexpcshelltests.py | security/manager/ssl/tests/unit/test_crlite_filters.js | 28757 | Launched Test App
[task 2022-10-20T17:22:38.533Z] 17:22:38 INFO - remotexpcshelltests.py | security/manager/ssl/tests/unit/test_crlite_filters.js | 28757 | Application ran for: 0:00:02.634081
[task 2022-10-20T17:22:38.596Z] 17:22:38 WARNING - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_crlite_filters.js | xpcshell return code: 0
[task 2022-10-20T17:22:38.596Z] 17:22:38 INFO - TEST-INFO took 2697ms
[task 2022-10-20T17:22:38.596Z] 17:22:38 INFO - >>>>>>>
Assignee | ||
Comment 20•2 years ago
|
||
Revised patch fixes the bug: https://treeherder.mozilla.org/jobs?repo=try&revision=d76ea78d18fa5b8f11f196efff51cbb7cd5c9ebc
Comment 21•2 years ago
|
||
Pushed by jschanck@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6e302dce9339 part 1. avoid deprecated downloadToDisk function. r=keeler,leplatrem,robwu https://hg.mozilla.org/integration/autoland/rev/1909084518a4 part 2. clean cert-revocations attachment cache. r=keeler,leplatrem,robwu
Comment 22•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6e302dce9339
https://hg.mozilla.org/mozilla-central/rev/1909084518a4
Comment 23•2 years ago
|
||
The patch landed in nightly and beta is affected.
:jschanck, is this bug important enough to require an uplift?
- If yes, please nominate the patch for beta approval.
- If no, please set
status-firefox107
towontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 24•2 years ago
|
||
Comment on attachment 9298906 [details]
Bug 1795710 - part 1. avoid deprecated downloadToDisk function. r=keeler
Beta/Release Uplift Approval Request
- User impact if declined: Attachments from the
cert-revocations
Remote Settings collection will accumulate on disk. - Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The patch changes a
downloadToDisk
call to adownloadAsBytes
call. The former writes a copy of the download to disk. Users were not using the on-disk cache. - String changes made/needed:
- Is Android affected?: No
Comment 25•2 years ago
|
||
Comment on attachment 9298906 [details]
Bug 1795710 - part 1. avoid deprecated downloadToDisk function. r=keeler
Approved for 107.0b5.
Assignee | ||
Comment 27•2 years ago
|
||
Comment on attachment 9298907 [details]
Bug 1795710 - part 2. clean cert-revocations attachment cache. r=keeler
Beta/Release Uplift Approval Request
- User impact if declined: The main purpose of this patch is to free disk space taken up by (unused) cached downloads. If the patch is not taken, the space will be freed when the client updates to 108+. However, I'm requesting uplift because this patch also includes a workaround for bug 1730026. On Oct 25, Remote Settings saw a spike in requests for attachments in the
security_state/intermediates
collection. The number of requests was in excess of the average number of users online at any given time, which suggests that individual clients were making multiple requests for the same file. This is likely due to bug 1730026, which causes multiple copies ofIntermediatePreloadsClient
to be initialized. - Is this code covered by automated tests?: No
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The patch has been tested on Nightly and manually verified by users on several platforms, including Android.
- String changes made/needed:
- Is Android affected?: Yes
Comment 28•2 years ago
|
||
Comment on attachment 9298907 [details]
Bug 1795710 - part 2. clean cert-revocations attachment cache. r=keeler
Approved for 107.0b6.
Comment 29•2 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 30•2 years ago
|
||
Forgot to update the bug, but I let John know: new files aren't being saved in my nightly profile, and all the old files were successfully cleaned up.
Updated•1 year ago
|
Description
•