Closed Bug 1429796 Opened 3 years ago Closed 1 year ago

Cleanup storage in CertBlocklist, add new types of pair (e.g. whitelist entries, crlite status)

Categories

(Core :: Security: PSM, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

(Depends on 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(3 files, 3 obsolete files)

No description provided.
(possibly also rename revocations.txt in the profile to reflect additional uses)
Depends on: 1429797
Priority: -- → P1
Whiteboard: [psm-assigned]
Depends on: 1492414
Test coverage: We're missing test coverage for removal of entries (this isn't covered for the existing implementation). We could do with a test or two for setting and reading whitelist / enrolment state (though it's largely hitting the same code as the OneCRL bits).

Tweaks: I need to make a minor change to blocklist-clients to read additions / removals rather than enumerating the whole of the collection data.
The "migrate from revocations.txt" stuff needs a) moving the a different method, b) the file removal bit and c) to not have the unwrap() calls, since we don't want panics.

Rationale: I've largely mirrored the XPCom interface in the SecurityState struct to allow us the use of ? to keep the actual implementation code clean, while still handling all error cases.

I spoke with Nika about this - she's happy with the approach (XPCom in Rust is fine, provided there's no non-scriptable stuff). In future, it might be nice to use a different ffi approach for the non-JS callers but that doesn't feel important right now. Nika suggested we might want to get froydnj's eyes on this.
Ah, also, I've just spotted I actually need to implement IsBlocklistFresh - pref branch reading works fine but it's in a WIP patch. I'll move it over.
Duplicate of this bug: 1429797
Duplicate of this bug: 1429799
Summary: Cleanup storage in CertBlocklist to allow easy addition of new types of pair (e.g. whitelist entries) → Cleanup storage in CertBlocklist, add new types of pair (e.g. whitelist entries, crlite status)
Attachment #9027711 - Attachment is obsolete: true
Attached file Crashy crash

We have a crash in rkv that could be a jemalloc bug. Myk and I are currently investigating.

Flags: needinfo?(dkeeler)
Depends on: 1531887
Attachment #9049693 - Attachment is obsolete: true

bug 1429796 - cert_storage: create rkv environment and store only once to avoid races r?mgoodwin,jcj

This patch also base64-decodes the API inputs before storing in the DB in
anticipation of being able to pass binary data directly.

Attachment #9050842 - Attachment is obsolete: true

This patch also base64-decodes the API inputs before storing in the DB in
anticipation of being able to pass binary data directly.

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bd54f8dfd9e
Cleanup storage in CertBlocklist to allow easy addition of new types of pair (e.g. whitelist entries) r=keeler
https://hg.mozilla.org/integration/autoland/rev/b0d08863f7a5
cert_storage: create rkv environment and store only once to avoid races r=mgoodwin,jcj

Looks like we need to whitelist the DB file backing cert_storage.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de76cc9178bd2dd9140e81ea53aa3e5a1f349947

Flags: needinfo?(mgoodwin)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99079ab7e52e
Cleanup storage in CertBlocklist to allow easy addition of new types of pair (e.g. whitelist entries) r=keeler
https://hg.mozilla.org/integration/autoland/rev/5514aae0e34e
cert_storage: create rkv environment and store only once to avoid races r=mgoodwin,jcj
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1538093

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #15)

Looks like we need to whitelist the DB file backing cert_storage.

It looks like the <profile>/security_state/data.mdb file is being accessed from the main thread. Is this intentional? If so, what makes the I/O to this file impossible to do off main thread?

Flags: needinfo?(dkeeler)

Just code complexity. I filed bug 1538250.

Flags: needinfo?(dkeeler)
Depends on: 1538539
Depends on: 1538541
No longer depends on: 1538093
Regressions: 1538093
Depends on: 1553256

Making this depend on ship-rkv meta instead of multiple individual bugs.

No longer depends on: 1538539, 1538541
You need to log in before you can comment on or make changes to this bug.