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

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
Last month

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

(Depends on 4 bugs)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Comment 1

2 years ago
(possibly also rename revocations.txt in the profile to reflect additional uses)
Assignee

Updated

2 years ago
Depends on: 1429797
Priority: -- → P1
Whiteboard: [psm-assigned]
Assignee

Updated

9 months ago
Depends on: 1492414
Assignee

Comment 3

7 months ago
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.
Assignee

Comment 4

7 months ago
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.
Assignee

Updated

5 months ago
Duplicate of this bug: 1429797
Assignee

Updated

5 months ago
Duplicate of this bug: 1429799
Assignee

Updated

5 months ago
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)
Assignee

Updated

5 months ago
Attachment #9027711 - Attachment is obsolete: true
No longer blocks: 657228
Depends on: 657228
Assignee

Comment 8

4 months ago
Posted file Crashy crash
Assignee

Comment 9

4 months ago

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

Assignee

Updated

4 months ago
Flags: needinfo?(dkeeler)
Flags: needinfo?(dkeeler)
Assignee

Updated

4 months ago
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.

Comment 13

3 months ago
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)

Comment 16

3 months ago
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

Comment 17

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(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
You need to log in before you can comment on or make changes to this bug.