Refactor CertVerifier to prepare for insanity::pkix integration

RESOLVED FIXED in mozilla29

Status

()

Core
Security: PSM
P1
enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 12 obsolete attachments)

7.12 KB, text/html
Details
46.32 KB, patch
keeler
: review+
cviecco
: feedback-
Details | Diff | Splinter Review
140.65 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.63 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
101.12 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
21.40 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
7.38 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
23.93 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
2.84 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
24.96 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Created attachment 772257 [details]
README.html

For various reasons (as described in the attached README.html), we need to be able to build the certificate verification part of Gecko separately from the rest of Gecko.

On #developers, some of us discussed whether we should really modify the Mozilla build system to accommodate this, or whether we should just have people issue specific "./mach build <directory>" commands, or whether we should just have people build NSPR and NSS standalone and then build CertVerifier. However, only the first option (modifying the build system to accomodate building CertVerifier as a subset of libxul) seems acceptable to me, because a big part of the project is to get the configuration of NSPR, NSS, and CertVerifier to *exactly* match how they are built, without requiring people using it to do (half-)hour long builds of libxul. I have made my build system changes as unobtrusive as possible to facilitate this.
Created attachment 772259 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent
Attachment #772259 - Flags: review?(dkeeler)
Attachment #772259 - Flags: review?(cviecco)
Comment on attachment 772259 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent

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

I don't have any concerns with this.

::: security/manager/ssl/src/CertVerifier.h
@@ +4,5 @@
>  
>  #ifndef mozilla_psm__CertVerifier_h
>  #define mozilla_psm__CertVerifier_h
>  
> +#include "mozilla/NullPtr.h"

Is this #include necessary?

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +448,5 @@
>                          const void * fdForLogging,
>                          uint32_t providerFlags,
>                          PRTime now)
>  {
>    MOZ_ASSERT(infoObject);

Should there be a MOZ_ASSERT(certVerifier)?

@@ +897,5 @@
> +      case CertVerifier::classic:
> +        successTelemetry
> +          = Telemetry::SSL_SUCCESFUL_CERT_VALIDATION_TIME_CLASSIC;
> +        failureTelemetry
> +          = Telemetry::SSL_INITIAL_FAILED_CERT_VALIDATION_TIME_LIBPKIX;

Shouldn't this be SSL_INITIAL_FAILED_CERT_VALIDATION_TIME_CLASSIC? (and vice versa, below)

@@ +989,5 @@
> +  if (!certVerifier) {
> +    PR_SetError(SEC_ERROR_NOT_INITIALIZED, 0);
> +    return SECFailure;
> +  }
> +  

whitespace

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +944,5 @@
>                             : ocspMode_FailureIsNotAVerificationFailure);
>  
> +  CertVerifier::implementation_config certVerifierImplementation
> +    = CertVerifier::classic;
> + 

whitespace
Attachment #772259 - Flags: review?(dkeeler) → review+
Created attachment 772345 [details] [diff] [review]
Part 2: Build System Changes
Attachment #772345 - Flags: review?(gps)
Attachment #772345 - Flags: review?(cviecco)
Attachment #772259 - Attachment description: remove CertVerifier's dependency on nsNSSComponent → Part 1: Remove CertVerifier's dependency on nsNSSComponent

Comment 4

5 years ago
Comment on attachment 772345 [details] [diff] [review]
Part 2: Build System Changes

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

I think I'm largely OK with this. Some if it feels a bit hacky, but such is how applications are defined in our build config. I would like a 2nd opinion to ensure I'm not missing something obvious. Off to glandium for a 2nd review.

Holding off on r+ because I'd really like to not bury important general build config bits in /security/certverified (although we already bury things in /toolkit/toolkit.mozbuild, so perhaps I'm being unfair).

::: toolkit/toolkit.mozbuild
@@ +6,5 @@
>  if CONFIG['LIBXUL_SDK']:
>      error('toolkit.mozbuild is not compatible with --enable-libxul-sdk=')
>  
> +# NSPR, SQLite, NSS, CertVerifier
> +include('/security/certverifier/certverifier.mozbuild')

I don't like burying nspr and nss bits behind this included file. At the risk of violating DRY, I'd just copy the add_tier_dir() lines into security/certverifier/app.mozbuild. Or, I'd factor everything into /config/nss-common.mozbuild (or something) and have everything include that file.

So many ugly choices to choose from...
Attachment #772345 - Flags: review?(mh+mozilla)
Attachment #772345 - Flags: review?(gps)
Attachment #772345 - Flags: feedback+
Comment on attachment 772345 [details] [diff] [review]
Part 2: Build System Changes

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

I agree with gps, the nss/nspr/sqlite stuff shouldn't live under security/certverifier.

::: build/dumbmake-dependencies
@@ +18,4 @@
>    security/manager
> +    security/certverifier
> +      security/build
> +        security/nss

While you're here, i don't think security/nss should be in this file. It can't be built directly.

::: python/mozbuild/mozbuild/mach_commands.py
@@ +347,5 @@
>          # Only for full builds because incremental builders likely don't
>          # need to be burdened with this.
>          if not what:
>              # Fennec doesn't have useful output from just building. We should
> +            # arguably make the build action useful for Fennec. 

Trailing whitespace.

::: security/certverifier/debug.mozconfig
@@ +1,2 @@
> +ac_add_options --enable-application=security/certverifier
> +ac_add_options --enable-debug 

trailing whitespace

::: security/certverifier/release.mozconfig
@@ +1,3 @@
> +ac_add_options --enable-application=security/certverifier
> +ac_add_options --enable-warnings-as-errors
> +ac_add_options --enable-tests

Not sure these mozconfig files should be here.
Attachment #772345 - Flags: review?(mh+mozilla)
Attachment #772259 - Flags: review?(cviecco) → review+
Bug 531067 removes the builtin default OCSP responders. In the unlikely even that that change gets backed out, we would need to update the standalone CertVerifier code to initialize the builtin default OCSP responders.
Depends on: 531067
Comment on attachment 772345 [details] [diff] [review]
Part 2: Build System Changes

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

I dont see any red flags (though I am no build expert)
Attachment #772345 - Flags: review?(cviecco) → review+
Created attachment 807371 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent [v2]

Review comments addressed.
Attachment #772259 - Attachment is obsolete: true
Attachment #807371 - Flags: review+
Created attachment 807372 [details] [diff] [review]
Part 2: Build system changes [v2] (without standalone certverifier)

I moved the changes necessary for the separate, standalone, certverifier library into a separate patch.
Attachment #772345 - Attachment is obsolete: true
Attachment #807372 - Flags: review?(mh+mozilla)
Created attachment 807422 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier
Attachment #807422 - Flags: review?(dkeeler)
Attachment #807422 - Flags: review?(cviecco)
Created attachment 807423 [details] [diff] [review]
Part 3: Move more cert-related initialization of NSS to security/certverifier
Attachment #807423 - Flags: review?(dkeeler)
Attachment #807423 - Flags: review?(cviecco)
Created attachment 807426 [details] [diff] [review]
Fix indention of code moved from nsNSSComponent.cpp to match the rest of NSSCertDBTrustDomain (r=me, a=whitespace only)
Attachment #807426 - Flags: review+
Comment on attachment 807372 [details] [diff] [review]
Part 2: Build system changes [v2] (without standalone certverifier)

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

::: security/certverifier/Makefile.in
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXPORT_LIBRARY	= 1

Please move this to moz.build (and use True for the value)

@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +EXPORT_LIBRARY	= 1
> +MOZILLA_INTERNAL_API = 1

Remove MOZILLA_INTERNAL_API, that's implied by LIBXUL_LIBRARY.
Attachment #807372 - Flags: review?(mh+mozilla) → review+
Comment on attachment 807422 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

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

Mostly looks good, but I think there are a few issues. r- for now.

::: security/certverifier/ExtendedValidation.cpp
@@ +24,5 @@
>    const char *dotted_oid;
>    const char *oid_name; // Set this to null to signal an invalid structure,
>                    // (We can't have an empty list, so we'll use a dummy entry)
>    SECOidTag oid_tag;
> +  const unsigned char ev_root_sha1_fingerprint[20];

This change (and the associated changes to the array of EV information) seems unrelated to this bug - I think it would be best to do this in a separate bug or at least a separate patch on this bug.

@@ +907,5 @@
> +    // If an entry is missing in the NSS root database, it may be because the
> +    // root database is out of sync with what we expect (e.g. a different
> +    // version of system NSS is installed). We will just silently avoid
> +    // treating that root cert as EV.
> +    if (!entry.cert) {

Something is wrong here - the above "if (!entry.cert) { continue; }" makes this dead code, right?

@@ +908,5 @@
> +    // root database is out of sync with what we expect (e.g. a different
> +    // version of system NSS is installed). We will just silently avoid
> +    // treating that root cert as EV.
> +    if (!entry.cert) {
> +      // The debug CA info is at position 0, and is NOT on the NSS root db

But only #ifdef DEBUG, right? Isn't this handled above?

@@ +963,5 @@
> +
> +void
> +CleanupIdentityInfo()
> +{
> +  for (size_t iEV=0; iEV < (sizeof(myTrustedEVInfos)/sizeof(nsMyTrustedEVInfo)); ++iEV) {

Isn't there a mozilla::ArraySize or something that will do this?

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1479,5 @@
> +nsresult
> +nsNSSCertificate::hasValidEVOidTag(SECOidTag &resultOidTag, bool &validEV)
> +{
> +  nsNSSShutDownPreventionLock locker;
> +  if (isAlreadyShutDown())

I know this all is copy-pasted code, but would this be a good opportunity to fix up the style? (In particular, using {} for all conditionals).

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1979,5 @@
>    }
>   loser:
>    return rv;
>  }
> +

I'm not sure the blank lines added in this file are necessary.
Attachment #807422 - Flags: review?(dkeeler) → review-
Comment on attachment 807423 [details] [diff] [review]
Part 3: Move more cert-related initialization of NSS to security/certverifier

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

Looks good. My one complaint is things like "const char * modNameUTF8" in NSSCertDBTrustDomain. Even though a lot of the pre-existing code isn't consistent, let's try and standardize on the placement of that * ("const char *modNameUTF8" seems to be most common).

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +26,3 @@
>  
> +namespace {
> + 

unnecessary whitespace
Attachment #807423 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) from comment #14)
> ::: security/certverifier/ExtendedValidation.cpp
> @@ +24,5 @@
> >    const char *dotted_oid;
> >    const char *oid_name; // Set this to null to signal an invalid structure,
> >                    // (We can't have an empty list, so we'll use a dummy entry)
> >    SECOidTag oid_tag;
> > +  const unsigned char ev_root_sha1_fingerprint[20];
> 
> This change (and the associated changes to the array of EV information)
> seems unrelated to this bug - I think it would be best to do this in a
> separate bug or at least a separate patch on this bug.

The goal of this bug is to enable the gecko-independent library in bug 918629. This change was required to remove one of the gecko dependencies (ns[AC]String and more, IIRC), so I would like to keep it if possible. Sound reasonable?

I would have liked to postpone making most of the changes in this bug, actually, but the insanity::pkix integration is based on top of these changes.

I will look into the remaining issues that you mentioned.
Comment on attachment 807422 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

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

The extended validation changes should not live here. In particular the change of the formatting of the internal representation.

::: security/certverifier/ExtendedValidation.cpp
@@ +24,5 @@
>    const char *dotted_oid;
>    const char *oid_name; // Set this to null to signal an invalid structure,
>                    // (We can't have an empty list, so we'll use a dummy entry)
>    SECOidTag oid_tag;
> +  const unsigned char ev_root_sha1_fingerprint[20];

I concurr with keeler. This does not belong to this patch. Is is independent and should not be a prereq to any of the other things in our plates.

@@ +963,5 @@
> +
> +void
> +CleanupIdentityInfo()
> +{
> +  for (size_t iEV=0; iEV < (sizeof(myTrustedEVInfos)/sizeof(nsMyTrustedEVInfo)); ++iEV) {

This is fixed at compile time.

@@ +978,3 @@
>  // Find the first policy OID that is known to be an EV policy OID.
> +SECStatus
> +GetFirstEVPolicy(CERTCertificate *cert, SECOidTag &outOidTag)

Why the rename to upper case?
Attachment #807422 - Flags: review?(cviecco) → review-
Comment on attachment 807423 [details] [diff] [review]
Part 3: Move more cert-related initialization of NSS to security/certverifier

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

Functionality ++ but I think the new code is more confusing than needed.

::: security/certverifier/CertVerifier.h
@@ +67,5 @@
> +// Controls the OCSP fetching behavior of the classic verification mode. In the
> +// classic mode, the OCSP fetching behavior is set globally instead of per
> +// validation.
> +void
> +SetClassicOCSPBehavior(CertVerifier::ocsp_download_config enabled,

I only see it defined in NSSCertDBTRustDomain.cpp.... Feels strange

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +62,5 @@
>  
> +} // unnamed namespace
> +
> +SECStatus
> +InitializeNSS(const char * dir, bool readOnly)

Why move this here and not leave it in NSSComponent?
Attachment #807423 - Flags: review?(cviecco) → review-
Depends on: 917047
No longer depends on: 917047
Created attachment 812878 [details] [diff] [review]
Part 2: Build system changes [v3]

Review comments adddressed
Attachment #807372 - Attachment is obsolete: true
Attachment #812878 - Flags: review+
Created attachment 812886 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

Addressed review comments. I talked to cviecco about why the ev_root_sha1_fingerprint changes were made and why InitializeNSS was moved to NSSCertDBTrustDomain; this is done to remove ExtendedValidation.cpp's XPCOM dependencies so that the standalone certverifier doesn't depend on XPCOM.
Attachment #807422 - Attachment is obsolete: true
Attachment #812886 - Flags: review?(dkeeler)
Attachment #812886 - Flags: review?(cviecco)
Created attachment 812928 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier
Attachment #807423 - Attachment is obsolete: true
Attachment #812928 - Flags: review?(cviecco)
Created attachment 812929 [details] [diff] [review]
Bug 891066, Part 4: Fix indention, r=me, a=whitespace-only
Attachment #812929 - Flags: review+
Comment on attachment 812886 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

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

I would r+ this, but I'm confused about the debug EV entry.

::: security/certverifier/ExtendedValidation.cpp
@@ +793,5 @@
>      "YAGXt0an6rS0mtZLL/eQ+w==",
>      nullptr
>    },
> +//{
> +//  // OU=Sample Certification Authority,O=\"Sample, Inc.\",C=US

Don't we need this for tests? If not, just remove it.

@@ +893,5 @@
> +    // version of system NSS is installed). We will just silently avoid
> +    // treating that root cert as EV.
> +    if (!entry.cert) {
> +      // The debug CA info is at position 0, and is NOT on the NSS root db
> +      PR_ASSERT(entry.cert || (DEBUG && iEV == 0));

Because of the !entry.cert above, doesn't this reduce to PR_ASSERT(DEBUG && iEV == 0)? Also, does this mean we're still using the debug entry that's commented out?

@@ +953,5 @@
> +      entry.cert = nullptr;
> +    }
> +  }
> +
> +  memset(&sIdentityInfoCallOnce, 0, sizeof(PRCallOnceType));

Why is this necessary?

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1472,5 @@
>    PR_FREEIF(servername);
>    return nickname;
>  }
>  
> +

nit: is this extra empty line good/necessary?

@@ +1573,5 @@
> +    char* oid_str = CERT_GetOidString(&oid_data->oid);
> +    NS_ENSURE_TRUE(oid_str, MapSECStatus(SECFailure));
> +
> +    outDottedOid = oid_str;
> +    PR_smprintf_free(oid_str);

It looks wrong to assign from oid_str and then free it, even though I assume the assign copies oid_str into outDottedOid's internal storage. Maybe use Assign()?

::: security/manager/ssl/src/nsNSSCertificate.h
@@ +74,5 @@
>    SECOidTag mCachedEVOidTag;
> +  nsresult hasValidEVOidTag(SECOidTag &resultOidTag, bool &validEV,
> +                            const nsNSSShutDownPreventionLock & proofOfLock);
> +  nsresult getValidEVOidTag(SECOidTag &resultOidTag, bool &validEV,
> +                            const nsNSSShutDownPreventionLock & proofOfLock);

snuggle the &s

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1978,5 @@
>    }
>   loser:
>    return rv;
>  }
> +

nit: not sure if this empty line is necessary
Attachment #812886 - Flags: review?(dkeeler) → review-
Comment on attachment 812886 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

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

I was going to go for r+ until I realized CA's would be unable to test EV certs with the new code. What options would we be offering to CAs?

::: security/certverifier/CertVerifier.cpp
@@ +4,5 @@
>  
>  #include "CertVerifier.h"
> +#include <stdint.h>
> +#include "mozilla/Assertions.h"
> +#include "insanity/pkixtypes.h"

When has this landed?

@@ +11,5 @@
>  #include "secerr.h"
>  #include "prerror.h"
>  
> +// ScopedXXX in this file are insanity::pkix::ScopedXXX, not
> +// mozilla::ScopedXXX.

Since this file lives in security I would prefer to use explicit names on the classes that are defined in multiple namespaces.

::: security/certverifier/ExtendedValidation.cpp
@@ -786,5 @@
>    od.supportedExtension = INVALID_CERT_EXTENSION;
>    return SECOID_AddEntry(&od);
>  }
>  
> -#ifdef PSM_ENABLE_TEST_EV_ROOTS

We need to have a mechanism for CA's to test new EV certs. What mechanism should CA's use now?

@@ +893,5 @@
> +    // version of system NSS is installed). We will just silently avoid
> +    // treating that root cert as EV.
> +    if (!entry.cert) {
> +      // The debug CA info is at position 0, and is NOT on the NSS root db
> +      PR_ASSERT(entry.cert || (DEBUG && iEV == 0));

Agree with keeler, remove the entry.cert part of the conditional.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -220,5 @@
> -#ifndef NSS_NO_LIBPKIX
> -  // In order to keep startup time lower, we delay loading and 
> -  // registering all identity data until first needed.
> -  memset(&mIdentityInfoCallOnce, 0, sizeof(PRCallOnceType));
> -#endif

Is it safe to remove this?
Attachment #812886 - Flags: review?(cviecco) → review-
Depends on: 916632
Comment on attachment 812886 [details] [diff] [review]
Part 2 (code changes): Move CertVerifier to security/certverifier

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

::: security/certverifier/CertVerifier.cpp
@@ +4,5 @@
>  
>  #include "CertVerifier.h"
> +#include <stdint.h>
> +#include "mozilla/Assertions.h"
> +#include "insanity/pkixtypes.h"

Obviously, those will have to land before this patch lands.

::: security/certverifier/ExtendedValidation.cpp
@@ -786,5 @@
>    od.supportedExtension = INVALID_CERT_EXTENSION;
>    return SECOID_AddEntry(&od);
>  }
>  
> -#ifdef PSM_ENABLE_TEST_EV_ROOTS

They will be able to use the embeddable cert verifier, at least eventually. That is one of the reasons for creating it. This is not a urgent problem to solve anyway.

@@ +953,5 @@
> +      entry.cert = nullptr;
> +    }
> +  }
> +
> +  memset(&sIdentityInfoCallOnce, 0, sizeof(PRCallOnceType));

Zeroing sIdentityInfoCallOnce will cause EnsureIdentityInfoLoaded to call IdentityInfoInit again. i.e. It undoes the bit that causes PR_CallOnce to detect duplicate calls. (Whether or not it is correct is another story, but that is what the existing code does.)

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +1573,5 @@
> +    char* oid_str = CERT_GetOidString(&oid_data->oid);
> +    NS_ENSURE_TRUE(oid_str, MapSECStatus(SECFailure));
> +
> +    outDottedOid = oid_str;
> +    PR_smprintf_free(oid_str);

Your assumption is correct. Not sure that Assign() is any better. Normally we would use Adopt() to avoid the copy+free, but Adopt() expects a string allocated with new/malloc and PR_smprintf doesn't guarantee that new/malloc were used.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ -220,5 @@
> -#ifndef NSS_NO_LIBPKIX
> -  // In order to keep startup time lower, we delay loading and 
> -  // registering all identity data until first needed.
> -  memset(&mIdentityInfoCallOnce, 0, sizeof(PRCallOnceType));
> -#endif

Yes. sIdentityInfoCallOnce is zero-initialized according to C++ semantics.
Attachment #812928 - Flags: review?(cviecco) → review?(dkeeler)
Comment on attachment 812928 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier

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

Just some small nits.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +10,4 @@
>  
> +#include "insanity/ScopedPtr.h"
> +#include "mozilla/Assertions.h"
> +#include "cert.h"

style nit: apparently these are supposed to be sorted. I see what you're doing by sorting the nss includes separately, though. Maybe have a blank line before cert.h to be clear about it?

@@ +62,5 @@
>  
> +} // unnamed namespace
> +
> +SECStatus
> +InitializeNSS(const char* dir, bool readOnly)

I'm not sure it makes sense to have this function here. To me it makes more sense to have the call to NSS_Initialize in nsNSSComponent. I agree the root loading should be here, however.

@@ +149,5 @@
>      CERT_DisableOCSPChecking(CERT_GetDefaultCertDB());
>      CERT_DisableOCSPDefaultResponder(CERT_GetDefaultCertDB());
>    } else {
>      CERT_EnableOCSPChecking(CERT_GetDefaultCertDB());
>      CERT_DisableOCSPDefaultResponder(CERT_GetDefaultCertDB());

Small nit: while we're here, it seems like we can factor out the DisableOCSPDefaultResponder call since it gets called either way.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +11,5 @@
> +#include "CertVerifier.h"
> +
> +namespace mozilla { namespace psm {
> +
> +SECStatus InitializeNSS(const char * dir, bool readOnly);

const char* dir here and elsewhere
Attachment #812928 - Flags: review?(dkeeler) → review+

Comment 27

5 years ago
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #25)
> >  
> > -#ifdef PSM_ENABLE_TEST_EV_ROOTS
> 
> They will be able to use the embeddable cert verifier, at least eventually.
> That is one of the reasons for creating it. This is not a urgent problem to
> solve anyway.
> 


Then please don't remove the EV Testing capability until *after* there is a replacement. I am using this, and I will have to update https://wiki.mozilla.org/PSM:EV_Testing_Easy_Version when the replacement is available.
Comment on attachment 812928 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +10,4 @@
>  
> +#include "insanity/ScopedPtr.h"
> +#include "mozilla/Assertions.h"
> +#include "cert.h"

OK, I will add the blank line for now. We should decide (via email) on whether we're going to sort all the includes in PSM.

@@ +62,5 @@
>  
> +} // unnamed namespace
> +
> +SECStatus
> +InitializeNSS(const char* dir, bool readOnly)

This is something cviecco also mentioned. Here's the problem I'm trying to solve: I want us to give CAs and others a library that they can use from Python (etc.) that does certificate verification like Firefox as closely as possible. In order to do that, I have to give them a function to initialize NSS; I want them to share the code that initializes NSS between the EmbeddedCertVerifier library and Firefox to make sure they stay in sync.

I could factor the initialization and shutdown of NSS into some third library that both the EmbeddedCertVerifier and Gecko share. That will make the code more complicated (more files) and will make the build system more complicated and slower. I didn't think it was justified. Do you think that is important to do now? We have about 40 more patches to go in the insanity::pkix series so it would be great if we could defer this refactoring until it becomes acutely important and focus on the work we need to get done immediately. But, if you still disagree, I can look into factoring this out.
See Also: → bug 926599
(In reply to Kathleen Wilson from comment #27)
> (In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment
> #25)
> > > -#ifdef PSM_ENABLE_TEST_EV_ROOTS
> > 
> > They will be able to use the embeddable cert verifier, at least eventually.
> > That is one of the reasons for creating it. This is not a urgent problem to
> > solve anyway.
> 
> Then please don't remove the EV Testing capability until *after* there is a
> replacement. I am using this, and I will have to update
> https://wiki.mozilla.org/PSM:EV_Testing_Easy_Version when the replacement is
> available.

I filed bug 926242 about updating the documentation.
I filed bug 926599 about resurrecting this functionality.

I don't think it is worth slowing down progress to bring that functionality back right now. In the interim, until we have something better, we can just patch the actual code and build a variant of Firefox that uses the updated real list for testing. Just file a bug in this component and one of us can write the patch and kick of the tryserver build for you to download. Alternatively, CAs who have programmers can just write the patch themselves and rebuild Firefox themselves. They can even contribute the patch in a bug for us.
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #28)
> This is something cviecco also mentioned. Here's the problem I'm trying to
> solve: I want us to give CAs and others a library that they can use from
> Python (etc.) that does certificate verification like Firefox as closely as
> possible. In order to do that, I have to give them a function to initialize
> NSS; I want them to share the code that initializes NSS between the
> EmbeddedCertVerifier library and Firefox to make sure they stay in sync.
> 
> I could factor the initialization and shutdown of NSS into some third
> library that both the EmbeddedCertVerifier and Gecko share. That will make
> the code more complicated (more files) and will make the build system more
> complicated and slower. I didn't think it was justified. Do you think that
> is important to do now? We have about 40 more patches to go in the
> insanity::pkix series so it would be great if we could defer this
> refactoring until it becomes acutely important and focus on the work we need
> to get done immediately. But, if you still disagree, I can look into
> factoring this out.

I see. Let's leave it as is and refactor later.

Comment 31

5 years ago
(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #29)
> I don't think it is worth slowing down progress to bring that functionality
> back right now. 

Please explain to me what is difficult about putting that part of the code back in, and approximately how much time you think it would take.


> In the interim, until we have something better, we can just
> patch the actual code and build a variant of Firefox that uses the updated
> real list for testing. Just file a bug in this component and one of us can
> write the patch and kick of the tryserver build for you to download.

So, for every CA that wants to test during their request for EV-treatment, I will have to file a bug and get a special build created for them to download?

I'd be better off having someone store one of the current builds for me on a server where people can download it to test with the current EV-testing capability. 

Of course, the only problem with that would be the CAs that want to test with DSA2 roots (they'll need a debug version of Firefox that has bug #925591 fixed in it).
Created attachment 8363397 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent [v3]

Rebased on top of the current m-i. Lots of changes on m-i since this was first reviewed so I'm asking for a re-review of the rebased patches. Also, there are some changes--e.g. splitting out SharedCertVerifier from CertVerifier. The splitting of SharedCertVerifier from CertVerifier was done because AtomicRefCounted apparently changed in a way that broke the previous build, and because my goal is to have security/certverifier completely independent from the reset of Gecko, except for NSS and NSPR.
Attachment #807371 - Attachment is obsolete: true
Attachment #8363397 - Flags: review?(dkeeler)
Created attachment 8363439 [details] [diff] [review]
Part 2: Move CertVerifier to security/certverifier

Same deal: Rebased on top of recent m-i with long-ago review comments addressed.

Note that the change to how CERT_DisableOCSPDefaultResponder is called in nsNSSComponent.cpp is actually a change to address a review comment from Part 1. But, since these are all going to land together, I'm not going to take time to move that change to the part 1 patch.
Attachment #812878 - Attachment is obsolete: true
Attachment #812886 - Attachment is obsolete: true
Attachment #8363439 - Flags: review?(dkeeler)
Comment on attachment 8363439 [details] [diff] [review]
Part 2: Move CertVerifier to security/certverifier

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

::: security/certverifier/ExtendedValidation.cpp
@@ -802,5 @@
>      CERT_AddCertToListTail(certList, CERT_DupCertificate(cert));
>    }
>  }
>  
> -#ifdef PSM_ENABLE_TEST_EV_ROOTS

NOTE: I talked to Kathleen Wilson about this, face-to-face, and we agreed it is OK to remove this EV testing support code for now. There's already a follow-up bug, bug 926599, for adding it back in a different, better, form later. Until then, CAs can use debug builds of ESR24 or Firefox 26/27/28 to EV testing. This will give us plenty of time to re-add the EV support code before anybody is actually affected by it. The new code should be much simpler than this code.
Created attachment 8363445 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier

Note that the indention changes for nss_addEscape are done in the next patch.
Attachment #812928 - Attachment is obsolete: true
Attachment #8363445 - Flags: review?(dkeeler)
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: --- → mozilla29
Created attachment 8363446 [details] [diff] [review]
Bug 891066, Part 4: Fix indention, r=me, a=whitespace-only
Attachment #812929 - Attachment is obsolete: true
Attachment #8363446 - Flags: review+
Attachment #807426 - Attachment is obsolete: true
Created attachment 8363460 [details] [diff] [review]
Bug 891066, Part 5: Switch to insanity::pkix::ScopedCERTCertificate and insanity::pkix::ScopedCERTCertList

Because insanity::pkix uses insanity::pkix::ScopedCERTCertificate, this ends up permeating CertVerifier, and it ends up being easier to must switch everything to use insanity::pkix::ScopedCERTCertificate instead of mozilla::psm::ScopedCERTCertificate. Due to different compilers understanding namespaces differently (AFAICT), I could not get "using" to do the right thing without these explicit namespace qualifications. Also, because some non-PSM stuff is using mozilla::psm::ScopedCERTCertificate, I could not yet remove mozilla::psm::ScopedCERTCertificate, though my plan is to do exactly that. Once we have only one ScopedCERTCertificate, we'll remove the other one from the tree.
Attachment #8363460 - Flags: review?(cviecco)
Created attachment 8363462 [details] [diff] [review]
Part 6: Move SSL server cert verification logic to security/certverifier
Attachment #8363462 - Flags: review?(cviecco)
Created attachment 8363463 [details] [diff] [review]
Part 7: Give CertVerifier its own NSPR logging module
Attachment #8363463 - Flags: review?(cviecco)
Summary: Allow CertVerifier to be built without the rest of Gecko → Refactor CertVerifier to prepare for insanity::pkix integration
Created attachment 8363475 [details] [diff] [review]
Part 8: Pass additional information needed by insanity::pkix into CertVerifier

1. insanity::pkix will accept the stapled OCSP response as an input into the BuildCertChain function (its analog to CERT_VerifyCert), so the stapled OCSP response needs to be passed into CertVerifier.

2. insanity::pkix avoids calling PR_Now() completely, and needs to have the current time passed into it. We should actually be using a consistent instant of time for all validation-related stuff anyway, even without the insanity::pkix integration.
Attachment #8363475 - Flags: review?(cviecco)
Attachment #8363475 - Attachment description: Pass additional information needed by insanity::pkix into CertVerifier → Part 8: Pass additional information needed by insanity::pkix into CertVerifier
Created attachment 8363476 [details] [diff] [review]
Part 9: Move DisableMD5 to certverifier/NSSCertDBTrustDomain
Attachment #8363476 - Flags: review?(cviecco)
Comment on attachment 8363397 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent [v3]

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

Looks good. Just a few questions.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +464,5 @@
>  
>  // Returns null with the error code (PR_GetError()) set if it does not create
>  // the CertErrorRunnable.
>  CertErrorRunnable*
> +CreateCertErrorRunnable(CertVerifier& certVerifier,

What's the rationale for using a reference here but a RefPtr<SharedCertVerifier> elsewhere?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +1903,1 @@
>  //#ifndef NSS_NO_LIBPKIX

I imagine this line can be removed? (as a separate style-changing patch, if you like)
Attachment #8363397 - Flags: review?(dkeeler) → review+
Comment on attachment 8363439 [details] [diff] [review]
Part 2: Move CertVerifier to security/certverifier

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

Overall, looks good.
Is there a security/certverifier/CertVerifier.h I'm missing?

::: build/dumbmake-dependencies
@@ -22,3 @@
>    security/manager
> -  security/dbm
> -  security/nss

What's the implication of removing security/dbm and security/nss from this list?

::: security/certverifier/ExtendedValidation.cpp
@@ -802,5 @@
>      CERT_AddCertToListTail(certList, CERT_DupCertificate(cert));
>    }
>  }
>  
> -#ifdef PSM_ENABLE_TEST_EV_ROOTS

Sounds good.

@@ +834,4 @@
>  static bool
>  isEVPolicy(SECOidTag policyOIDTag)
>  {
> +  for (size_t iEV=0; iEV < PR_ARRAY_SIZE(myTrustedEVInfos); ++iEV) {

Since you're already changing this line, I'd add spaces around the index variable declaration/assignment to 0 as well.

::: toolkit/toolkit.mozbuild
@@ +16,5 @@
>          add_tier_dir('nss', 'security/build')
>  
>      include('/config/js/js.mozbuild')
>  
> +    # Keep certverifier in sync with security/certverifier/library/app.mozbuild

Is this a file that's supposed to be in this patch?
Attachment #8363439 - Flags: review?(dkeeler) → review+
Comment on attachment 8363445 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier

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

Looks good. Again, just a couple of questions on things I'm not clear on.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +97,5 @@
> +  if (!escaped_fullLibraryPath) {
> +    return SECFailure;
> +  }
> +
> +  /* If a module exists with the same name, delete it. */

nit: c-style comment

@@ +134,5 @@
> +  }
> +}
> +
> +void
> +SetClassicOCSPBehavior(CertVerifier::ocsp_download_config enabled,

I'm a little confused here, since we now have both this and setNonPkixOcspEnabled (unless I missed where you get rid of it).

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +234,5 @@
> +
> +  SetClassicOCSPBehavior(*odc, *osc, *ogc);
> +
> +  if (*odc == CertVerifier::ocsp_on) {
> +    SSL_ClearSessionCache();

Do we want to clear the session cache for ocsp_strict as well?

@@ +673,5 @@
>        if (!mozFile) {
>          continue;
>        }
>  
> +      if NS_FAILED(mozFile->GetNativePath(libDir)) {

if (NS_FAILED(...)) {

@@ -997,2 @@
>  {
> -  nsNSSShutDownPreventionLock locker;

Do we not need the lock for calling CERT_SetOCSPTimeout?

@@ +1004,5 @@
>  #endif
>  
> +  CertVerifier::ocsp_download_config odc;
> +  CertVerifier::ocsp_strict_config osc;
> +  CertVerifier::ocsp_get_config ogc; 

nit: trailing space

@@ -1110,5 @@
>  
>  NS_IMETHODIMP
>  nsNSSComponent::SkipOcspOff()
>  {
> -  nsNSSShutDownPreventionLock locker;

Again - we don't need this anymore? (not that it did the right thing anyway, since isAlreadyShutDown() wasn't checked...)
Attachment #8363445 - Flags: review?(dkeeler) → review+
Comment on attachment 8363397 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent [v3]

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

I think I am surprised by the behavior of the FLAG_NO_DV_FALLBACK_FOR_EV I think it is confusing (the way I read it is: if EV fails return early with failure). Can you point to the patch where this change was made?

Marking feedback- to get your attention. In reality this is an r+

::: security/manager/ssl/src/CertVerifier.cpp
@@ +123,5 @@
>                           /*optional out*/ CERTVerifyLog* verifyLog)
>  {
> +  if (!cert ||
> +      ((flags & FLAG_NO_DV_FALLBACK_FOR_EV) &&
> +      (usage != certificateUsageSSLServer || !evOidPolicy)))

why !evOidPolicy? the internal api states of as optional (why do we want to push ev logic into the consumers of this call?)

@@ +321,1 @@
>      return SECSuccess;

return SECFailure (the verification failed) unless we are changing the definition of this function and if this is true I propose a rename.

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1287,5 @@
>    validEV = false;
>    resultOidTag = SEC_OID_UNKNOWN;
>  
>    uint32_t flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY |
>                     mozilla::psm::CertVerifier::FLAG_NO_DV_FALLBACK_FOR_EV;

Can you give tha patch for this change the addition of the FLAG_NO_DV_FALLBACK_FOR_EV flag?
Attachment #8363397 - Flags: feedback-
Comment on attachment 8363397 [details] [diff] [review]
Part 1: Remove CertVerifier's dependency on nsNSSComponent [v3]

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

::: security/manager/ssl/src/CertVerifier.cpp
@@ +123,5 @@
>                           /*optional out*/ CERTVerifyLog* verifyLog)
>  {
> +  if (!cert ||
> +      ((flags & FLAG_NO_DV_FALLBACK_FOR_EV) &&
> +      (usage != certificateUsageSSLServer || !evOidPolicy)))

David, can you write a patch to add comments that document this more clearly in the header file, and also can you add some "// XXX this sucks" type comments to the code that implements this strange thing.

Camilo: I agree it is strange. However, we need it temporarily for bug 950240. Once we switch from NSS to insanity::pkix, we can remove this hack.

@@ +321,1 @@
>      return SECSuccess;

Please see bug 950240. This is how it is supposed to be, though I understand it is confusing. Basically, nobody should be using FLAG_NO_DV_FALLBACK_FOR_EV except nsNSSCertificate::IsExtendedValidation.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +464,5 @@
>  
>  // Returns null with the error code (PR_GetError()) set if it does not create
>  // the CertErrorRunnable.
>  CertErrorRunnable*
> +CreateCertErrorRunnable(CertVerifier& certVerifier,

RefPtr<SharedCertVerifier>& is used when we may need to make a copy (increment the reference count). CertVerifier& is used when we don't need copying.

::: security/manager/ssl/src/nsIdentityChecking.cpp
@@ +1287,5 @@
>    validEV = false;
>    resultOidTag = SEC_OID_UNKNOWN;
>  
>    uint32_t flags = mozilla::psm::CertVerifier::FLAG_LOCAL_ONLY |
>                     mozilla::psm::CertVerifier::FLAG_NO_DV_FALLBACK_FOR_EV;

bug 950240.
Comment on attachment 8363462 [details] [diff] [review]
Part 6: Move SSL server cert verification logic to security/certverifier

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +697,5 @@
>    for ( i = 0; i < numcerts; i++ ) {
>      rawCerts[i] = &certCollection->rawCerts[i];
>    }
>  
> +  serverNickname = DefaultServerNicknameForCert(cert.get());

Why not use the namespace?
Attachment #8363462 - Flags: review?(cviecco) → review+
Comment on attachment 8363439 [details] [diff] [review]
Part 2: Move CertVerifier to security/certverifier

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

::: build/dumbmake-dependencies
@@ -22,3 @@
>    security/manager
> -  security/dbm
> -  security/nss

See comment 5. The file was wrong. security/dbm doesn't exist any more and security/nss is built by security/build.

::: toolkit/toolkit.mozbuild
@@ +16,5 @@
>          add_tier_dir('nss', 'security/build')
>  
>      include('/config/js/js.mozbuild')
>  
> +    # Keep certverifier in sync with security/certverifier/library/app.mozbuild

Thanks. I will remove this comment, moving it to the patch that adds that file.
Comment on attachment 8363463 [details] [diff] [review]
Part 7: Give CertVerifier its own NSPR logging module

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

Rationale for making this static verifier? (what was wrong with using PSM , YAGNI?)
Attachment #8363463 - Flags: review?(cviecco) → review+
Comment on attachment 8363475 [details] [diff] [review]
Part 8: Pass additional information needed by insanity::pkix into CertVerifier

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

::: security/certverifier/CertVerifier.h
@@ +23,5 @@
>  
>    // *evOidPolicy == SEC_OID_UNKNOWN means the cert is NOT EV
>    // Only one usage per verification is supported.
>    SECStatus VerifyCert(CERTCertificate* cert,
> +          /*optional*/ const SECItem* stapledOCSPResponse,

add a default nullptr and move to the optional parameters section please. I would even put it at the end of the parameters.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +796,5 @@
>  
>    insanity::pkix::ScopedCERTCertList certList;
>    SECOidTag evOidPolicy;
> +  rv = certVerifier.VerifySSLServerCert(cert, stapledOCSPResponse,
> +                                        time, infoObject,

This change - does not match the attachmentment name. This, if needed should go on a different patch. (ditto with the change to the function signature)
Attachment #8363475 - Flags: review?(cviecco) → review-
Attachment #8363476 - Flags: review?(cviecco) → review+
Comment on attachment 8363460 [details] [diff] [review]
Bug 891066, Part 5: Switch to insanity::pkix::ScopedCERTCertificate and insanity::pkix::ScopedCERTCertList

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

r+ assuming the final state we only have one set of scoped classes.
Comment on attachment 8363460 [details] [diff] [review]
Bug 891066, Part 5: Switch to insanity::pkix::ScopedCERTCertificate and insanity::pkix::ScopedCERTCertList

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

assuming the final state is a single set of scoped classes.
Attachment #8363460 - Flags: review?(cviecco) → review+
(In reply to Camilo Viecco (:cviecco) from comment #49)
> Review of attachment 8363463 [details] [diff] [review]:
> -----------------------------------------------------------------
> Rationale for making this static verifier? (what was wrong with using PSM ,
> YAGNI?)

I don't understand what you mean here. Please clarify.

(In reply to Camilo Viecco (:cviecco) from comment #50)
> Review of attachment 8363475 [details] [diff] [review]:
> -----------------------------------------------------------------
> add a default nullptr and move to the optional parameters section please. I
> would even put it at the end of the parameters.

OK, I will attempt this.

> This change - does not match the attachmentment name. This, if needed should
> go on a different patch. (ditto with the change to the function signature)

You mean the change to pass a single time through the functions, instead of calling PR_Now() in multiple places? I don't think it is worth splitting into a separate patch. I admit that this patch is a little strange without the insanity::pkix code being integrated, but replacing the different uses of PR_Now() is good to do even without insanity::pkix.
Flags: needinfo?(cviecco)
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #53)
> (In reply to Camilo Viecco (:cviecco) from comment #49)
> > Review of attachment 8363463 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > Rationale for making this static verifier? (what was wrong with using PSM ,
> > YAGNI?)
> 
> I don't understand what you mean here. Please clarify.

Dont make another logger until you actually need to have it independent. 

> 
> (In reply to Camilo Viecco (:cviecco) from comment #50)
> > Review of attachment 8363475 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > add a default nullptr and move to the optional parameters section please. I
> > would even put it at the end of the parameters.
> 
> OK, I will attempt this.
> 
> > This change - does not match the attachmentment name. This, if needed should
> > go on a different patch. (ditto with the change to the function signature)
> 
> You mean the change to pass a single time through the functions, instead of
> calling PR_Now() in multiple places? I don't think it is worth splitting
> into a separate patch. I admit that this patch is a little strange without
> the insanity::pkix code being integrated, but replacing the different uses
> of PR_Now() is good to do even without insanity::pkix.

yes, that is why I suggested making it a separate patch. (even a new bug and land it now :)), but will leave it to you.
Flags: needinfo?(cviecco)
Comment on attachment 8363475 [details] [diff] [review]
Part 8: Pass additional information needed by insanity::pkix into CertVerifier

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

r+ with comments addressed (after irc chat)
Attachment #8363475 - Flags: review- → review+
Comment on attachment 8363445 [details] [diff] [review]
Bug 891066, Part 3: Move more initialization of NSS to security/certverifier

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +134,5 @@
> +  }
> +}
> +
> +void
> +SetClassicOCSPBehavior(CertVerifier::ocsp_download_config enabled,

You are right to be confused, because it is wrong. I will submit a new version of the patch with this fixed.

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +234,5 @@
> +
> +  SetClassicOCSPBehavior(*odc, *osc, *ogc);
> +
> +  if (*odc == CertVerifier::ocsp_on) {
> +    SSL_ClearSessionCache();

Since these prefs are almost never changed, we should just do the conservative thing and always clear the session cache. I will do this in the next version of the patch.

@@ -997,2 @@
>  {
> -  nsNSSShutDownPreventionLock locker;

SetValidationOptions can only be called on the main thread, and only when the mutex is held, so I think we're good here.

@@ -1110,5 @@
>  
>  NS_IMETHODIMP
>  nsNSSComponent::SkipOcspOff()
>  {
> -  nsNSSShutDownPreventionLock locker;

Again, because it can only be called on the main thread, we aren't going to be racing with NSS shutdown because NSS shutdown must happen on the main thread.
Created attachment 8364012 [details] [diff] [review]
bug-891066-p3-prime.patch
Attachment #8363445 - Attachment is obsolete: true
Attachment #8364012 - Flags: review?(dkeeler)
Comment on attachment 8364012 [details] [diff] [review]
bug-891066-p3-prime.patch

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

Great!
Attachment #8364012 - Flags: review?(dkeeler) → review+
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #46)
> ::: security/manager/ssl/src/CertVerifier.cpp
> @@ +123,5 @@
> >                           /*optional out*/ CERTVerifyLog* verifyLog)
> >  {
> > +  if (!cert ||
> > +      ((flags & FLAG_NO_DV_FALLBACK_FOR_EV) &&
> > +      (usage != certificateUsageSSLServer || !evOidPolicy)))
> 
> David, can you write a patch to add comments that document this more clearly
> in the header file, and also can you add some "// XXX this sucks" type
> comments to the code that implements this strange thing.

Camilo and I spoke - he's taking care of this in bug 962833.
Comment on attachment 8363462 [details] [diff] [review]
Part 6: Move SSL server cert verification logic to security/certverifier

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

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +697,5 @@
>    for ( i = 0; i < numcerts; i++ ) {
>      rawCerts[i] = &certCollection->rawCerts[i];
>    }
>  
> +  serverNickname = DefaultServerNicknameForCert(cert.get());

You mean, qualify it with mozilla::psm::? Since we already are "using mozilla::psm" it isn't ncessary, and this code should all eventually get moved to mozilla::psm anyway.
(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #53)
> (In reply to Camilo Viecco (:cviecco) from comment #50)
> > Review of attachment 8363475 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > add a default nullptr and move to the optional parameters section please. I
> > would even put it at the end of the parameters.
> 
> OK, I will attempt this.

Sorry, I forgot to comment on this before I landed the changes: I did start to do this, and then I started to feel like I was just spending time making the code worse, so I reverted my changes. It is true that most callers pass in nullptr but IMO it is worth making the caller more explicitly consider whether they need to pass in the stapled OCSP response. Especially with the upcoming must-staple work, we need to make sure we are passing in the stapled OCSP response whenever we have one. So, I ended up not making this change.

Updated

4 years ago
Blocks: 973913

Updated

3 years ago
Blocks: 400088
You need to log in before you can comment on or make changes to this bug.