783994 - switch Mozilla to use the cert9+key4 (aka sql) database file format

Status: RESOLVED FIXED

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

Description

[Kai Engert (:KaiE:)]

The NSS team would like to stop supporting an older database (cert8+key3) file format, currently being used by default by Mozilla client applications.

We would like to change PSM to init NSS to use the newer cert9+key4 file format.

Technical background, the old cert8+key3 is a nosql key+data format, called "dbm", only allowing exclusive access. The new file format cert9+key4 uses an sqlite database that supports concurrency.

Note, this bug doesn't yet propose to introduce shared databases with Mozilla. This will require more work, that shall be done in separate follow-up work. (The tricky part of it will be migration of data from several applications into a consolidated location, which can require multiple password prompts and UI guidance.)

The efforts required for simply switching to the new file format at the Mozilla application level will be very small, only the parameters used for the NSS initialization call will change.

Note that NSS will perform an automatic conversion from the old format to the new format, if necessary.

Because newer software will write new certificates to the newer files, only, support teams should be aware of the following edge scenario: If a user has added certificates using a newer software (e.g. newer Firefox), but then the user switches back to an older software version (e.g. older Firefox), then the older software will continue to use the old files, which will not contain the information added with the newer software.

This bug is currently being blocked by two bugs, affecting users who use a home directory on a network file system.

Comment 1

[Kai Engert (:KaiE:)]

In my opinion we must have fixes for the blocking bugs (see dependency list) prior to proceeding with this task.

Comment 2

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #1)
> In my opinion we must have fixes for the blocking bugs (see dependency list)
> prior to proceeding with this task.

One of them has been fixed.

For the other one (bug 508081) the the original bug reporters haven't shown sufficient interest to follow up, despite my efforts to get that bug resolved. Therefore it shouldn't block the bug any longer. I'm removing the dependency.

Comment 3

[Kai Engert (:KaiE:)]

This is now blocked by bug 730495. Mozilla needs to ensure that sqlite is properly initialized, and not shut down the session that NSS initializes for its storage.

Comment 4

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

It would be worthwhile to evaluate making this change (as I recall, the performance of the sql database is much worse than dbm, but maybe this wouldn't be noticeable during normal browsing).

Comment 5

[Kai Engert (:KaiE:)]

See also bug 1377940.

If Firefox prefers to remain on the classic DBM file format, then it should change NSS init to prefix the profile directory with the string "dbm:", to explicitly request the DBM file format.

Comment 6

[Franziskus Kiefer [:franziskus]]

David, where are we on this? Maybe we can take this occasion and finally move. Otherwise we'll have to land a patch in Firefox together with the next NSS version that holds the patch from bug 1377940.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #6)
> David, where are we on this? Maybe we can take this occasion and finally
> move. Otherwise we'll have to land a patch in Firefox together with the next
> NSS version that holds the patch from bug 1377940.

Just for completeness, changing NSS wouldn't be necessary.

PSM makes a call to NSS_Init* with a path. You could prefix that path with "dbm:" to continue to get the old behavior. You could make that change at any time, even now, without coordinating with NSS.

Comment 8

[Kai Engert (:KaiE:)]

I realize, if we changed Firefox to explicitly request the "dbm" file format, then it would break those users, who have already decided that they want to use the new SQL format. This is possible, by defining an environment variable (which overrides the NSS default). I have personally running this configuration for years already.

Maybe it's time to just try it out, and see if we get any complaints.

It's 4 more weeks until the development cycle is over, so maybe now is still a good time to land this change?

I will start a full firefox try build, which picks up the NSS default change.

Comment 9

[Kai Engert (:KaiE:)]

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

Comment 10

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

If I recall correctly, there are some behavioral differences between the dbm and sql databases that cause test failures (the try run in comment 9 would seem to confirm my recollection). Benton will probably be working on addressing these issues. We're meeting tomorrow to plan that out.

Comment 11

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

Er, to clarify - it seems some of the tests are run without a preexisting dbm database, which causes a sql db to be created, which causes failures.

Comment 12

[Kai Engert (:KaiE:)]

I just noticed there are some places in firefox code that specifically reference the cert8.db/key3.db/secmod.db filenames. I'm trying to change those and will start another try build.

Comment 13

[Mike Hommey [:glandium]]

For one, there's:
https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_isBuiltInRoot_reload.js#81-82

Comment 14

[Kai Engert (:KaiE:)]

(In reply to Mike Hommey [:glandium] from comment #13)
> For one, there's:
> https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/
> unit/test_cert_isBuiltInRoot_reload.js#81-82

Right. See the new try build
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8b4278ea1b5adfd3f296c25a12bfb4318c78557
which uses this patch
  https://hg.mozilla.org/try/rev/b8b4278ea1b5adfd3f296c25a12bfb4318c78557
which adjusts it.

There are more places that references the filenames that I had assumed. Let's see how far this try build gets, and how much further adjustments we'll need.

Comment 15

[Kai Engert (:KaiE:)]

One major difference is that when using the SQL file format, modifications to the DB apparently require one to login to the database first. This causes many PSM tests to fail. I'm currently running xpshell tests locally and adding calls to loginToDBWithDefaultPassword();

I also have many local conversion from key3/cert4 files alredy. But some tests still fail, after I made both modifications (convert and login).

Comment 16

[Kai Engert (:KaiE:)]

Bob, how can Firefox ensure that no runtime operations will fail, that depend on the SQL token database being logged in (which previously didn't require to be logged in with DBM) ?

Is it necessary that Firefox attempts to login prior to executing NSS operations?

It's probably difficult to identify all operations that might depend on NSS being logged in, so a workaround would be to login to NSS unconditionally.

If I understand correctly, PSM could try to login with the empty default password, and if that fails, prompt the user to enter the password.

This would change the behavior of Firefox for users who have a master password set, to enter their password earlier.

Comment 17

[Kai Engert (:KaiE:)]

I rediscovered that this bug still depends on bug 730495.

Some of the PSM tests trigger the following assertion:
  mozilla/db/sqlite3/src/sqlite3.c:22844: sqlite3MutexAlloc: Assertion `GLOBAL(int, mutexIsInit)' failed.

Comment 18

[Kai Engert (:KaiE:)]

Removing bug 730495 from the dependency list, because we have the workaround checked in.

IMHO this depends on bug 1377940 to get implemented first. Only after NSS is able to switch its default format we should consider to switch Firefox.

Nevertheless, we should continue to work on making all of this happen.

Comment 19

[Kai Engert (:KaiE:)]

When running the Firefox test suite with NSS using the SQL database, we run into many failures.

I found that many (not all) of the failures can be fixed, by explicitly adding calls to PK11_InitPin(""), like some of the already existing tests do.

I haven't yet been able to find out if the reason is bug 1379154 (potentially related to the firefox tests using checked in databases in the old dbm file format, which are uninitialized), or because NSS-with-SQL is more strict in requiring the database to initialized in general.

I have started to work on a patch that, amongst other things, added explicit calls to loginToDBWithDefaultPassword() to individual tests. That allowed many tests to work, but this solution seems unsatisfactory, because IMHO, if it's necessary to do that for PSM/NSS to work correctly, then I think it should be done right after PSM init (in the C++ code, e.g. by calling PK11_InitPin("")).

I suggested that to Bob, but Bob said it shouldn't be necessary. That confuses me. I don't understand how else this would work. Either Bob misunderstood, or I'm misunderstanding.

As Bob explained in another bug, in the past, Firefox had deliberately kept NSS databases in the "uninitialized" state, so Firefox could detect this state, and suggest the user to set a master password on the first occassion.

This state is apparently no longer used, and it seems unlikely it ever will be used again. Today's Firefox saves passwords without ever asking the user to set a master password (although those passwords are encrypted by Firefox "secret decoder ring/SDR" using a symmetric key stored in the NSS database, which by default isn't protected).

So, PSM ocassionally wants to change the NSS database. Some of these operations require the SQL database to be logged in, and the logged in state cannot be reached without having the database initialized yet.

If Firefox doesn't want to prompt the user for a master password, consequently Firefox must initialize the NSS database with an empty password.

Firefox has the choice to just unconditionally initialize the key database (by setting the empty password using PK11_InitPin) very early after the NSS_init library call, or perform this just right before each operation that could potentially modify the NSS database.

With the SQL database, there seems to be a behavioral change, and this is required for a larger amount of scenarios.

I'm worried that trying to identify all possible scenarios that required it might be futile, and work intensive, and the init by default seems more appropriate to me. But it would be good to have feedback from Bob why he doesn't consider this approach ideal (or to see if we misunderstood each other).

We're apparently still in the exploration phase which approach would be best to achieve that all PSM tests work.

I'll shortly attach a patch, which shows the changes I've done so far.

My most recent experiments were limited to xpcshell tests, which mostly work, except the one blocked by bug 1379273.

We need to continue by investigating the reasons for the remaining test failures.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: wip-783994-v0.1.patch

Comment 21

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #20)
> Created attachment 8888839 [details] [diff] [review]
> wip-783994-v0.1.patch

much better than before, but still lots of orange

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

Comment 22

[Robert Relyea]

> Bob, how can Firefox ensure that no runtime operations will fail, that depend on the SQL token database being logged
> in (which previously didn't require to be logged in with DBM) ?

I'm not sure what you are asking. Are you asking if there is a subset of operations we can identify that do not require logging in to execute? Are conversely are you asking if there are a subset that requires logging in that didn't before?

The answer to either of those is yes, the subset that requires logging in are basically anything that actually modifies the cert database (importing certs, importing keys, changing the trust flags on certificates). Actually importing keys always required prior login. These operations are pretty much restricted to PSM operations, and most are under user control (or should be). NOTE: we should already be doing the login as modifications to tokens require login to work in general.

> Is it necessary that Firefox attempts to login prior to executing NSS operations?

No the majority of firefox operations would still not require prior login. You can fire up the browser and go to websites all day long without requiring a login.

This is different from FIPS mode, where login is required for the most basic operations (other then cert lookup or prng). NSS automatically forces a login in these situation).

bob

Comment 23

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

Attachment: bug 783994 - add preference for and enable sqlite-backed NSS databases

In the future, bug 1377940 will make the sqlite-backed databases the default,
but until we're sure this will stick we want to make this a Firefox-only change.
The use of a preference to configure which format to use will hopefully allow us
to restore the old behavior quickly and relatively safely if necessary. Note
that doing this should be done with care; any changes made in the sqlite
databases after upgrade migration will not be reflected if we need to go back to
the old database format. Thus, user data (imported CAs, client certificates, and
keys) can be lost.

Review commit: https://reviewboard.mozilla.org/r/177454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177454/

Comment 24

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8905657 [details]
bug 783994 - add preference for and enable sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/177454/#review182478

InitiailizeNSS being main-thread-only is not a new prohibition, right?

Anyway, this looks good to me for forcing the new mode.

Comment 25

[J.C. Jones [:jcj] (he/him)]

If we don't land this very soon, we might want to wait and land this (or pref flip maybe) after we branch 58, per Sylvestre's advice on dev.platform. I don't think this is a killer feature for 57, and it could probably use plenty of testing.

Comment 26

[:Cykesiopka]

Comment on attachment 8905657 [details]
bug 783994 - add preference for and enable sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/177454/#review183018

LGTM, but I think at minimum we should have a test that ensures the new pref does what it's supposed to before we land this.

Comment 27

[:Cykesiopka]

Comment on attachment 8905657 [details]
bug 783994 - add preference for and enable sqlite-backed NSS databases

https://reviewboard.mozilla.org/r/177454/#review183336

::: security/manager/ssl/security-prefs.js:46
(Diff revision 1)
> +// If true, use the modern sqlite-backed certificate and key databases in NSS.
> +// If false, use the old BerkeleyDB format. Changing this requires a restart
> +// to take effect.
> +pref("security.sqldb.enabled", true);

Hmm, actually - this behaviour is only true until Bug 1377940 is fixed. If this is the behaviour we want, maybe it's better to rename the pref so it's clearer that setting it to true forces the SQL DB to be used, and setting it to false uses whatever the default/the environment specifies?

Comment 28

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

(In reply to J.C. Jones [:jcj] from comment #25)
> If we don't land this very soon, we might want to wait and land this (or
> pref flip maybe) after we branch 58, per Sylvestre's advice on dev.platform.
> I don't think this is a killer feature for 57, and it could probably use
> plenty of testing.

You're probably right. I filed bug 1398932 for landing just the ability but not actually switching it on.

(In reply to :Cykesiopka from comment #26)
> Comment on attachment 8905657 [details]
> bug 783994 - add preference for and enable sqlite-backed NSS databases
> 
> https://reviewboard.mozilla.org/r/177454/#review183018
> 
> LGTM, but I think at minimum we should have a test that ensures the new pref
> does what it's supposed to before we land this.

Good call.

(In reply to :Cykesiopka from comment #27)
> Comment on attachment 8905657 [details]
> bug 783994 - add preference for and enable sqlite-backed NSS databases
> 
> https://reviewboard.mozilla.org/r/177454/#review183336
> 
> ::: security/manager/ssl/security-prefs.js:46
> (Diff revision 1)
> > +// If true, use the modern sqlite-backed certificate and key databases in NSS.
> > +// If false, use the old BerkeleyDB format. Changing this requires a restart
> > +// to take effect.
> > +pref("security.sqldb.enabled", true);
> 
> Hmm, actually - this behaviour is only true until Bug 1377940 is fixed. If
> this is the behaviour we want, maybe it's better to rename the pref so it's
> clearer that setting it to true forces the SQL DB to be used, and setting it
> to false uses whatever the default/the environment specifies?

Good point.

Comment 30

[Kai Engert (:KaiE:)]

Attachment: 783994-v0.3.patch

updated work in progress patch.

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e79fd912831869272317d4363156ce253100a3b

Comment 31

[Kai Engert (:KaiE:)]

There's a new failure in the try build, which I believe is a new blocker for this bug, see bug 1401514.

Comment 32

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie:) from comment #31)
> There's a new failure in the try build, which I believe is a new blocker for
> this bug, see bug 1401514.

This was similar to bug 730495, just for shutdown. A fix has been checked in today, so an important blocker has been solved.

Comment 33

[Kai Engert (:KaiE:)]

Attachment: 783994-converted-databases.patch

Comment 34

[Kai Engert (:KaiE:)]

Attachment: 783994-tests-and-filename-references.patch

Comment 35

[Kai Engert (:KaiE:)]

I think that all blocker bugs have been addressed, in mozilla-central and in NSS trunk, which are required for Gecko to be compatible with NSS/sqlite.

This try build looks good to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=630b6df1835dc7d3f6f0de3eb6a7fabc0957bb26

The try build used the fixes from bug 730495, the patches from bug 1401594, bug 1377940, and NSS trunk (post 3.33), plus the two patches I've just attached to this bug.

I think we are ready to change both the NSS default and the Gecko NSS expectations in the same development cycle, for Firefox 58 and NSS 3.34.

If we switch both at the same time, then we don't need "flexible" tests.
The tests need to know which databases to expect, and also, we require converted test databases, so changing both NSS and Gecko tests at the same time simplifies our work.

With this approach, the test security/manager/ssl/tests/unit/test_db_format_pref_old.js fails when we change the default. I sugest to remove it at the same time, because the pref set to false doesn't guarantee that you get cert8.db.

I think our aim should be to ensure Gecko is compatible with NSS/sqlite, in a way that's still comaptible with NSS/dbm (if an environment continues to use that combination for any reason).
This is important for Red Hat, as we don't want to change the NSS default on long term support branches. But Mozilla shouldn't have to worry about that.
I believe we don't require any changes to Gecko that would break compatibility with NSS/dbm, so that part shouldn't require any work.

Note that the timing of the NSS default change is important for distributions like Fedora, where we want to make the switch for the whole OS. (Just FYI: I think we will patch Fedora 27 and older to keep the old default. On Fedora 28 and newer, we'll make the change to NSS, and backport the Gecko application level changes to Firefox and Thunderbird 52.x, to ensure it's safe to make the system NSS switch earlier than the release of Firefox 58 and Thunderbird 59.)

David, what do you think?

I'll be away between Sep 30 and Oct 8. I could land this bug 783994 for Gecko, and bug 1377940 / bug 1401594 for NSS in the week starting on Oct 9, and work on follow-up issues, if any.

Comment 36

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

Thanks for driving this forward, Kai!

(In reply to Kai Engert (:kaie:) from comment #35)
> This try build looks good to me:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=630b6df1835dc7d3f6f0de3eb6a7fabc0957bb26

I'm concerned about the android test failures - have you confirmed those aren't due to these patches?
Also, security/manager/ssl/tests/unit/test_pkcs11_slot.js is failing intermittently. I ran into this as well ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b1d1605ab8281efe21fae976a2f763fa82ac979 ). I think the issue may be a race condition involving loading a module while doing other NSS operations that this test exposes. The patch in https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a6ba0ad13c1d6a15a9685118edb9b589c8da570 seems to address it.
There also may be an issue with  xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_scriptCreated.js (see again those two try links). We probably need to figure out what's going on there before moving forward with this.

> I think we are ready to change both the NSS default and the Gecko NSS
> expectations in the same development cycle, for Firefox 58 and NSS 3.34.

I would be much more comfortable starting by flipping the pref added in bug 1398932 and seeing if there are any unexpected issues before making it the default in NSS. Ideally, though, we would be able to make both changes in the 58 timeframe.

> If we switch both at the same time, then we don't need "flexible" tests.
> The tests need to know which databases to expect, and also, we require
> converted test databases, so changing both NSS and Gecko tests at the same
> time simplifies our work.

I think we may be able to start by flipping the pref and then updating the tests when we land the NSS change, and it'll work out to about the same amount of work overall - let me know what you think about that approach.

> With this approach, the test
> security/manager/ssl/tests/unit/test_db_format_pref_old.js fails when we
> change the default. I sugest to remove it at the same time, because the pref
> set to false doesn't guarantee that you get cert8.db.

That's fine - we definitely don't need it after we switch.

> I think our aim should be to ensure Gecko is compatible with NSS/sqlite, in
> a way that's still comaptible with NSS/dbm (if an environment continues to
> use that combination for any reason).
> This is important for Red Hat, as we don't want to change the NSS default on
> long term support branches. But Mozilla shouldn't have to worry about that.
> I believe we don't require any changes to Gecko that would break
> compatibility with NSS/dbm, so that part shouldn't require any work.

I imagine Red Hat could just ship Firefox with the pref from bug 1398932 set to false for now?

> Note that the timing of the NSS default change is important for
> distributions like Fedora, where we want to make the switch for the whole
> OS. (Just FYI: I think we will patch Fedora 27 and older to keep the old
> default. On Fedora 28 and newer, we'll make the change to NSS, and backport
> the Gecko application level changes to Firefox and Thunderbird 52.x, to
> ensure it's safe to make the system NSS switch earlier than the release of
> Firefox 58 and Thunderbird 59.)
> 
> David, what do you think?
> 
> I'll be away between Sep 30 and Oct 8. I could land this bug 783994 for
> Gecko, and bug 1377940 / bug 1401594 for NSS in the week starting on Oct 9,
> and work on follow-up issues, if any.

Ok - thanks for letting me know. I'll see about working out these test issues.

Comment 37

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

Attachment: bug 783994 - use the sqlite-backed certificate and key DBs

Review commit: https://reviewboard.mozilla.org/r/186102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186102/

Comment 38

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8914818 [details]
bug 783994 - use the sqlite-backed certificate and key DBs

https://reviewboard.mozilla.org/r/186102/#review191114

::: security/manager/ssl/tests/unit/xpcshell.ini:148
(Diff revision 1)
>  [test_pinning_header_parsing.js]
>  [test_sdr.js]
>  [test_sdr_preexisting.js]
> +[test_sdr_preexisting_with_password.js]
> +# Not relevant to Android. See the comment in the test.
> +skip-if = toolkit == 'android'

I realize Android never had this code, but does the test really need to be disabled? It seems like it should work just fine.

Comment 39

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

Comment on attachment 8914818 [details]
bug 783994 - use the sqlite-backed certificate and key DBs

https://reviewboard.mozilla.org/r/186102/#review191114

Thanks!

> I realize Android never had this code, but does the test really need to be disabled? It seems like it should work just fine.

If I recall correctly, the code to upgrade the databases is compiled out, so NSS will just create new databases and the test will fail because it can't access the key.

Comment 40

[J.C. Jones [:jcj] (he/him)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #39)
> If I recall correctly, the code to upgrade the databases is compiled out, so
> NSS will just create new databases and the test will fail because it can't
> access the key.

Ah, well that's a good reason to leave it disabled then. :)

Thanks!

Comment 41

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

Comment on attachment 8914818 [details]
bug 783994 - use the sqlite-backed certificate and key DBs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186102/diff/1-2/

Comment 42

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

I was having a bit of trouble with this on try, but I believe those were all unrelated oranges, because now it looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42fe4bdcdc9da95656587f89177fea8b6727a1ae
So, let's try this and see how it goes.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b47f07ff93be
use the sqlite-backed certificate and key DBs r=jcj

Comment 44

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b47f07ff93be

Comment 45

[Kai Engert (:KaiE:)]

David, thanks for the progress with this bug.

(I'm surprised that your approach works without requiring the changes from my attached patches.)

A local xpcshell tests shows that we'll still require adjustments once the NSS library default is changed, too,
e.g. security/manager/ssl/tests/unit/test_db_format_pref_old.js fails.

I suspect there might be additional changes. I've filed bug 1409708 to track that,
and started a try build (see link in that bug) to find out how much more adjustments will be necessary.

(Maybe some of the changes from my attachments will still be necessary, let's track that in bug 1409708.)

Comment 46

[Kai Engert (:KaiE:)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #36)
> 
> > I think we are ready to change both the NSS default and the Gecko NSS
> > expectations in the same development cycle, for Firefox 58 and NSS 3.34.
> 
> I would be much more comfortable starting by flipping the pref added in bug
> 1398932 and seeing if there are any unexpected issues before making it the
> default in NSS. Ideally, though, we would be able to make both changes in
> the 58 timeframe.

Yes, I makes sense to start testing, let's see what happens during the next few days.

My main worry is, if we ship Firefox 58 with this pref, then users might incorrectly conclude they can use it switch back to dbm. It would be nice to change the NSS default and remove the pref prior to the Firefox 58 release. But we can discuss this later, after we got test feedback.


> > If we switch both at the same time, then we don't need "flexible" tests.
> > The tests need to know which databases to expect, and also, we require
> > converted test databases, so changing both NSS and Gecko tests at the same
> > time simplifies our work.
> 
> I think we may be able to start by flipping the pref and then updating the
> tests when we land the NSS change, and it'll work out to about the same
> amount of work overall - let me know what you think about that approach.

ok. I was too slow in responding, it's good you've started that.


> > With this approach, the test
> > security/manager/ssl/tests/unit/test_db_format_pref_old.js fails when we
> > change the default. I sugest to remove it at the same time, because the pref
> > set to false doesn't guarantee that you get cert8.db.
> 
> That's fine - we definitely don't need it after we switch.

ok


> > I think our aim should be to ensure Gecko is compatible with NSS/sqlite, in
> > a way that's still comaptible with NSS/dbm (if an environment continues to
> > use that combination for any reason).
> > This is important for Red Hat, as we don't want to change the NSS default on
> > long term support branches. But Mozilla shouldn't have to worry about that.
> > I believe we don't require any changes to Gecko that would break
> > compatibility with NSS/dbm, so that part shouldn't require any work.
> 
> I imagine Red Hat could just ship Firefox with the pref from bug 1398932 set
> to false for now?

Yes, we can keep the pref set to false for Firefox and Thunderbird on the OS release branches that shouldn't change.

Comment 47

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

(In reply to Kai Engert (:kaie:) from comment #45)
> I suspect there might be additional changes. I've filed bug 1409708 to track
> that,
> and started a try build (see link in that bug) to find out how much more
> adjustments will be necessary.
> 
> (Maybe some of the changes from my attachments will still be necessary,
> let's track that in bug 1409708.)

Thanks!

(In reply to Kai Engert (:kaie:) from comment #46)
> My main worry is, if we ship Firefox 58 with this pref, then users might
> incorrectly conclude they can use it switch back to dbm. It would be nice to
> change the NSS default and remove the pref prior to the Firefox 58 release.
> But we can discuss this later, after we got test feedback.

That's a good point. We're not exposing the pref in any UI (other than about:config), though, so as long as we remove it soon (when NSS defaults to sqlite), hopefully users won't even know it ever existed. It's more of a temporary escape valve if we need to revert the change quickly.

Comment 48

[Chris H-C :chutten]

This bug may have contributed to a sudden change in the Telemetry probe SSL_SUCCESFUL_CERT_VALIDATION_TIME_MOZILLAPKIX[1] which seems to have occurred in Nightly 20171018[2][3].

There was an overall increase in the length of time measured in the probe. The number of submissions and samples are roughly flat across this change, so it is the measurements themselves that have likely increased meaning a longer time per sample.
This might mean that the length of time validating certs has gotten longer.

Is this an improvement? A regression?

Is this intentional? Is this expected?

Is this probe still measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/40/alerts/?from=2017-10-18&to=2017-10-18
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f78d5947333422ab09ec23e3dab0d48538c9d6ad&tochange=9f8b3fc384ae6fe8053b087548c57020a55f00fd
[3]: https://mzl.la/2lbmOR6

Comment 49

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

Thanks, Chris. This is not entirely unexpected. It is a regression - we're investigating what impact this will have (note that this is happening off the main thread, and it's essentially racing with the network, so as long as we're faster than the network here, we're probably ok). I definitely think the probe is still measuring something useful, as it illustrated that making changes like this can affect certificate verification time :)

Comment 50

[Matt Wobensmith [:mwobensmith][:matt:]]

This just surfaced in the TLS Canary last night. It didn't generate proper TLS errors, so we didn't catch it outright, but it caused connections to hang around past the expected timeout period and our tools indicated something was amiss.