945855 - Remove the security.first_network_revocation_method pref

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ This bug was initially created as a clone of Bug #917047 +++

This preference is unnecessary. We're always going to want to try OCSP first.

Note that besides removing the uses of the pref in nsNSSComponent, the corresponding argument and member variable in the CertVerifier class should be removed.

Comment 1

[raymond [:retornam] (inactive)]

Attachment: bug945855.patch

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8342031 [details] [diff] [review]
bug945855.patch

Review of attachment 8342031 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/CertVerifier.cpp
@@ -32,5 @@
>    , mOCSPDownloadEnabled(odc == ocsp_on)
>    , mOCSPStrict(osc == ocsp_strict)
>    , mRequireRevocationInfo(arfc == any_revo_strict)
> -  , mCRLFirst(firstNetworkRevocationMethod != nullptr &&
> -              !strcmp("crl", firstNetworkRevocationMethod))

Besides removing mCRLFirst, you also need to remove the use of mCRLFirst in CertVerifier::VerifyCert.

::: security/manager/ssl/src/CertVerifier.h
@@ -54,5 @@
>    const bool mMissingCertDownloadEnabled;
>    const bool mCRLDownloadEnabled;
>    const bool mOCSPDownloadEnabled;
>    const bool mOCSPStrict;
>    const bool mRequireRevocationInfo;

You should remove mCRLFirst here too.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -1035,5 @@
> -  nsCString firstNetworkRevo =
> -    Preferences::GetCString("security.first_network_revocation_method");
> -  if (firstNetworkRevo.IsEmpty()) {
> -    firstNetworkRevo = FIRST_REVO_METHOD_DEFAULT;
> -  }

This code was recently changed in bug 915937.

Please do "hg pull --rebase" (assuming that you are pulling from mozilla-inbound) with your patch applied and then correct the merge conflict.

Comment 3

[raymond [:retornam] (inactive)]

Attachment: bug945855.patch

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8343239 [details] [diff] [review]
bug945855.patch

Review of attachment 8343239 [details] [diff] [review]:
-----------------------------------------------------------------

Getting close.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1014,3 @@
>    if (firstNetworkRevo.IsEmpty()) {
>      firstNetworkRevo = "ocsp";
>    }

The firstNetworkRevo variable and the code that uses it needs to be removed too.

Comment 5

[raymond [:retornam] (inactive)]

Attachment: bug945855.patch

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8343547 [details] [diff] [review]
bug945855.patch

Review of attachment 8343547 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f685a6ac53

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61f685a6ac53