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

Status: RESOLVED FIXED

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

Comment 1

[Mark Goodwin [:mgoodwin]]

(possibly also rename revocations.txt in the profile to reflect additional uses)

Comment 2

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1429796 Cleanup storage in CertBlocklist to allow easy addition of new types of pair (e.g. whitelist entries) r?keeler

Comment 3

[Mark Goodwin [:mgoodwin]]

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.

Comment 4

[Mark Goodwin [:mgoodwin]]

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.

Comment 5

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1429796 Cleanup storage in CertBlocklist to allow easy addition of new types of pair (e.g. whitelist entries) r?keeler

Comment 8

[Mark Goodwin [:mgoodwin]]

Attachment: Crashy crash

Comment 9

[Mark Goodwin [:mgoodwin]]

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

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: Summary: bug 1429796 - create our cert_storage once to avoid crashes

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

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.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: 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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Andreea Pavel [:apavel]]

Backed out for xperf failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&selectedJob=234865916&searchStr=windows%2C10%2Cx64%2Cpgo%2Ctalos%2Cperformance%2Ctests%2Cwith%2Ce10s%2Ctest-windows10-64-pgo%2Fopt-talos-xperf-e10s%2Ct-e10s%28x%29&revision=b0d08863f7a5fc08a3c0709b5e7151d80ae18261

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=234865916&repo=autoland&lineNumber=1443

Backout: https://hg.mozilla.org/integration/autoland/rev/91403c24fee3d5df8fa978710785807b409d7af8

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/99079ab7e52e
https://hg.mozilla.org/mozilla-central/rev/5514aae0e34e

Comment 18

[Florian Quèze [:florian]]

(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?

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Just code complexity. I filed bug 1538250.

Comment 20

[Victor Porof [:vporof][:vp]]

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