205434 - Fully implement new libPKIX cert verification API from bug 294531

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

We can only enable or disable OCSP checking *globally*
in NSS right now.  We should add an option to disable
OCSP checking for a particular cert verification.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This should be part of the cert verification API design in bug 294531

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, Does the new API defined in bug 294531 satisfy this request? 
If so, then we need a bug that demands the implementation of the API defined 
in bug 294531.  (We may already have such a bug)

Comment 3

[Wan-Teh Chang]

Yes, the new API defined in bug 294531 satisfies this request.
(That API is so flexible that it can solve any cert path
validation problem.)

Comment 4

[Alexei Volkov]

Attachment: Patch v1 - change API flags to remove libpkix performace impact

Proposing to change default setting to revocation flags to remove impact to the performance.


Currently, libpkix does two revocation checks on the same cert: 
    * first, while building the chain upward from the leaf to a trust anchor. Each time the algorithm finds a valid issue of a cert in the chain(EE or intermediate CA), it calls CRL revocation checker to verify, that previous cert in the chain was not revoked by issuing ICA. Since the CRLDP is not yet integrated, the crl revocation check is done only by using CRL cache.
 

    * second, while validating an already built chain going down. This time libpkix calls ocsp checker, that returns cert status by obtaining an info from ocsp cache. If the cache does not have fresh information, ocsp checker fetches a response from ocsp responder.

Once CRLDP code is integrated into libpkix, we will have two types of revocation checkers that have similar parts: OCSP/CRL cache and OCSP responce/CRL network fetching.

As a part of work on implementation of CERT_PKIXVerifyCert revocation API, I plan to modify the way libpkix check cert revocation status: it is to make libpkix use cached revocation data while building the chain to its trust achor, and if fresh information is needed to check revocation, do additional data(ocsp responce/CRL) fetching at the time of complete chain validation after trust anchor have been identified. 

Now, while reviewing the revocation API flags I've found, that current default setting for the flags responsible for defining an order of checking can drawback the performance. These two flags:
   CERT_REV_MI_TEST_EACH_METHOD_SEPARATELY defined as 0L  -  default
   CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST defined as 1L

The flag EACH_METHOD_SEPARATELY tells to use a method completely(first cache and then fetch from a network) before going to a next one. If it is used, cached information from the next method will only be checked after the first method is done with network fetching, thus it will happen late, only at the time when chain is already built. In some cases, it will mean that whole chain will need to be completely rebuilt.

The patch sets the flag TEST_ALL_LOCAL_INFORMATION_FIRST to be a default, making libpkix to do early check on the cached data available for defined revocation methods.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 332548 [details] [diff] [review]
Patch v1 - change API flags to remove libpkix performace impact

Sorry, I think I must reject this patch.

You are reverting the numeric values of CERT_REV_MI_TEST_ALL_LOCAL_INFORMATION_FIRST

and also the other constant.

That constant is already used in Firefox 3.

I think your change breaks binary compatibility.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

We disccussed this on Thursday, and there seemed to be an understanding 
of what the plan was, going forward, between Alexei, Bob and Kai, but 
I missed it.  Would someone care to state here what the new plan is?

Comment 7

[Alexei Volkov]

The conclusion was to test local caches of all methods regardless of the set flags. Any result from a method that indicates that a cert was not revoked or the status is unknown should be treated as positive. In this case we continue to check if next revocation method has any information(looking for negative info) in its cache. If it does,  we will stop, and declare cert to be revoked.

A cert that passed all checks will be added into the chain. Next revocation check will be done, when the whole chain get built. This time we do revocation with possible OCSP response/CRL fetching from the network. 

Comment 8

[Alexei Volkov]

Attachment: Implementation of new revocation API. The whole patch. Not for review

This patch will be split on multiple smaller patches.

The patch includes modifications to multiple libpkix files, some changes in certdb, certhigh and modification on vfychain utility.

Comment 9

[Alexei Volkov]

Attachment: Patch v1 Main logic code. Includes only changes in libpkix/pkix/checker files.

Comment 10

[Alexei Volkov]

Attachment: Suplimental changes in nss/lib to support revocation patch

Comment 11

[Alexei Volkov]

Attachment: vfychain changes related to the new revocation api

Comment 12

[Alexei Volkov]

Attachment: Patch v2 Main logic code. Includes only changes in libpkix/pkix/checker files.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 340250 [details] [diff] [review]
Patch v2 Main logic code. Includes only changes in libpkix/pkix/checker files.

Alexei & I reviewed this by phone.  I have given him all the review comments verbally.

Comment 14

[Alexei Volkov]

Attachment: Suplimental changes in nss/lib to support revocation patch. v2

Resubmitting the patch. The old one had wrong files.

Comment 15

[Alexei Volkov]

Attachment: Suplimental changes in nss/lib to support revocation patch. v2 part 1

First part of the big patch. It has changes to the underlying pkix_pl layer needed for revocation API implementation to work.

Comment 16

[Alexei Volkov]

Attachment: Suplimental changes in nss/lib to support revocation patch. v2 part 2

Usage of newly implemented revocation API within libpkix. Revocation params propagation into libpkix.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 340010 [details] [diff] [review]
vfychain changes related to the new revocation api

r-. I gave Alexei review feeback by phone. (this comment applies to 3 patches.

Comment 18

[Alexei Volkov]

Attachment: Patch v3 for Main logic code. Includes only changes in libpkix/pkix/checker files.

Patch with changes requested during review.

Comment 19

[Alexei Volkov]

Attachment: Patch v2: vfychain changes related to the new revocation api

Changes to the patch according the review.

Comment 20

[Alexei Volkov]

Attachment: Supplemental changes in nss/lib to support revocation patch. v3 part 1

Patch changes according to the review.

Comment 21

[Alexei Volkov]

Attachment: Supplemental changes in nss/lib to support revocation patch. v3 part 2

Patch changes according to the review.

Comment 22

[Alexei Volkov]

Attachment: Supplemental changes to new files in nss/lib to support revocation patch. v3 part 3

Changes to new files.

Comment 23

[Alexei Volkov]

Comment on attachment 342913 [details] [diff] [review]
Patch v3 for Main logic code. Includes only changes in libpkix/pkix/checker files.

Main functionality is implemented in function PKIX_RevocationChecker_Check. It is responsible for tracking revocation method independent flags. Control over rev method dependent flags is shared between PKIX_RevocationChecker_Check function and rev method related functions. That is pkix_CrlChecker_CheckLocal and pkix_CrlChecker_CheckExternal for crl and pkix_OcspChecker_CheckLocal and pkix_OcspChecker_CheckExternal for ocsp.

Kai, Bob, please review.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 342915 [details] [diff] [review]
Patch v2: vfychain changes related to the new revocation api

In today's review, we made a few more changes to this patch.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 342920 [details] [diff] [review]
Supplemental changes in nss/lib to support revocation patch. v3 part 1

r+, with one requested change.  In file
lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c
in the function to be changed as follows:

>         if (PKIX_ERROR_RECEIVED){
>+            if (ocspResponse) {
>                 PKIX_DECREF(ocspResponse);
>+            } else {
>+                if (serverSession) 
>+                    hcv1->freeSessionFcn(serverSession);
>+                if (requestSession)
>+                    hcv1->freeFcn(requestSession);
>+            }
>         }

Please rename "requestSession" to just "request" 
all throughout that function.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 342922 [details] [diff] [review]
Supplemental changes to new files in nss/lib to support revocation patch. v3 part 3

r=nelson

Comment 27

[Alexei Volkov]

Attachment: Patch v1 Missing pkix_revocationmethod.c and .h

New files missing from main code patch.

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 343470 [details] [diff] [review]
Patch v1 Missing pkix_revocationmethod.c and .h

r-, this call merely copies the values from method to itself.

>+        PKIX_CHECK(
>+            pkix_RevocationMethod_Init(method,
>+                                       method->methodType,
>+                                       method->flags,
>+                                       method->priority,
>+                                       method->localRevChecker,
>+                                       method->externalRevChecker,
>+                                       plContext),
>+            PKIX_COULDNOTCREATEREVOCATIONMETHODOBJECT);

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 342921 [details] [diff] [review]
Supplemental changes in nss/lib to support revocation patch. v3 part 2

This is getting very close.  There are a few issues in this file:
>Index: lib/certhigh/certvfypkix.c

1. The following block of code should not be ifdef'ed.  
The comment is incorrect.  The old chain validation algorithm DOES
do CRL checking at each step in the chain. The new code must, also.

>+#ifdef DEBUG_volkov
>+    /* Will not perform CRL revocation on the whole chain, since old chain
>+     * validation algorithm never did it. */
>+
>+    /* add CRL revocation method for other certs in the chain. */
>+    PKIX_CHECK(
>+        PKIX_RevocationChecker_CreateAndAddMethod(revChecker, procParams,
>+                                         PKIX_RevocationMethod_CRL,
>+                                         PKIX_REV_M_TEST_USING_THIS_METHOD |
>+                                         PKIX_REV_M_FORBID_NETWORK_FETCHING |
>+                                         PKIX_REV_M_STOP_TESTING_ON_FRESH_INFO,
>+                                         0, NULL, PKIX_FALSE, plContext),
>+        PKIX_REVOCATIONCHECKERADDMETHODFAILED);
>+#endif

Also, I think that PKIX_REV_M_STOP_TESTING_ON_FRESH_INFO should NOT 
be set for the CRL tests.  For checking leaf certs, the old code 
would check CRLs first, and if the CRL said it was revoked, that 
would stop the test, but if the CRL did not say the cert was revoked,
and an OCSP URL was present, it would also do the OCSP check, even if
the CRL had already been checked.  

2.  In the OCSP test, I believe the old code had a settable global
flag that was equivalent to the new flag named 
PKIX_REV_M_FAIL_ON_MISSING_FRESH_INFO.  So, the new flag should
not unconditionally set that flag, but rather should test that
old global flag, and set the new flag according to the old one.
>+    PKIX_CHECK(
>+        PKIX_RevocationChecker_CreateAndAddMethod(revChecker, procParams,
>+                                     PKIX_RevocationMethod_OCSP,
>+                                     PKIX_REV_M_TEST_USING_THIS_METHOD |
>+                                     PKIX_REV_M_ALLOW_NETWORK_FETCHING |
>+                                     PKIX_REV_M_ALLOW_IMPLICIT_DEFAULT_SOURCE |
>+                                     PKIX_REV_M_SKIP_TEST_ON_MISSING_SOURCE |
>+                                     PKIX_REV_M_FAIL_ON_MISSING_FRESH_INFO |
>+                                     PKIX_REV_M_STOP_TESTING_ON_FRESH_INFO |
>+                                     methodFlags,
>+                                     1, NULL, PKIX_TRUE, plContext),

3. The following comment mentions CRL revocation, but the code that 
it describes is OCSP code, not CRL code.

>+#ifdef DEBUG_volkov
>+    /* Will not perform CRL revocation on the whole chain, since old chain
>+     * validation algorithm never did it. */
>+
>+    /* add OCSP revocation method to check the chain certificates */
>+    PKIX_CHECK(
>+        PKIX_RevocationChecker_CreateAndAddMethod(revChecker, procParams,
>+                                     PKIX_RevocationMethod_OCSP,

Comment 30

[Alexei Volkov]

Attachment: Patch v4 for Main logic code. Includes only changes in libpkix/pkix/checker files.

Comment 31

[Alexei Volkov]

Attachment: Patch v3: vfychain changes related to the new revocation api

Comment 32

[Alexei Volkov]

Comment on attachment 343470 [details] [diff] [review]
Patch v1 Missing pkix_revocationmethod.c and .h

Obsoleted by "patch v4 for Main logic code. Includes only changes in libpkix/pkix/checker files" patch.

Comment 33

[Alexei Volkov]

Attachment: Supplemental changes in nss/lib to support revocation patch. v4 part 2

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 344942 [details] [diff] [review]
Patch v4 for Main logic code. Includes only changes in libpkix/pkix/checker files.

This appears to address the previous review comments. r=nelson

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 344945 [details] [diff] [review]
Patch v3: vfychain changes related to the new revocation api

r=nelson

Comment 36

[Alexei Volkov]

Attachment: new ocsp.c ocspi.h changes. Patch v1

Comment 37

[Alexei Volkov]

Attachment: new ocsp.c ocspi.h changes. Patch v2

Comment 38

[Alexei Volkov]

Attachment: new ocsp.c ocspi.h changes. Patch v3

Comment 39

[Alexei Volkov]

Attachment: new ocsp.c ocspi.h changes. Patch v4

fix declared returned value in the header file.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 344946 [details] [diff] [review]
Supplemental changes in nss/lib to support revocation patch. v4 part 2

r+.  I suggest adding comments to the expressions that 'OR' in symbols whose values are zero.  Add comments pointing out that their values are zero.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 345004 [details] [diff] [review]
new ocsp.c ocspi.h changes. Patch v4


>+/* FUNCTION: ocsp_FetchingFailureIsVerificationFailure
>+ * The function checks the global ocsp settings and
>+ * tells how to treat an ocsp response fetching failure.
>+ * RETURNS:
>+ *   if PR_Bool is returned, then treat fetching as a
>+ *   revoked cert status.

I'm pretty sure that comment wants to say:
  if PR_TRUE is returned, then ...

r=nelson with that change.

I want to see testing results before any of this is 
committed.

Comment 42

[Alexei Volkov]

Attachment: Patch v5 for Main logic code. Includes only changes in libpkix/pkix/checker files.

Additional changes to PKIX_RevocationCheker_Check function

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 345573 [details] [diff] [review]
Patch v5 for Main logic code. Includes only changes in libpkix/pkix/checker files.

r+. Please make a couple of small changes.

>+        /* The following check make sence only for chain

Should be "makes"

>+enum PKIX_RevocationMethodTypeEnum {
>+    PKIX_RevocationMethod_CRL = 0,
>+    PKIX_RevocationMethod_OCSP,
>+    PKIX_RevocationMethod_Count,
>+};

Please change _Count to _MAX.  

Thanks.

Comment 44

[Alexei Volkov]

Attachment: Complete patch.

Comment 45

[Slavomir Katuscak]

Patch for this bug causes build failures on HP/UX:

gmake-3.80[5]: Entering directory `/share/builds/mccrel3/security/securitytip/builds/20081101.1/wozzeck_Solaris8/mozilla/security/nss/lib/libpkix/pkix/certsel'
cc -o HP-UXB.11.11_OPT.OBJ/pkix_certselector.o -c -O -DHPUX10 -Ae +Z -DHPUX -Dhppa -D_HPUX_SOURCE -D_USE_BIG_FDS +DAportable +DS2.0 -DHPUX11 -D_POSIX_C_SOURCE=199506L -DXP_UNIX -UDEBUG -DNDEBUG -DNSS_ENABLE_ECC -DNSS_ECC_MORE_THAN_SUITE_B -DUSE_UTIL_DIRECTLY -I../../../../../../dist/HP-UXB.11.11_OPT.OBJ/include  -I../../../../../../dist/public/nss -I../../../../../../dist/private/nss -I../../../../../../dist/public/dbm  pkix_certselector.c
cc: "../../../../../../dist/private/nss/pkixt.h", line 125: error 1574: Unknown size for "PKIX_RevocationMethodType".
cc: "../../../../../../dist/private/nss/pkixt.h", line 126: error 1574: Unknown size for "PKIX_RevocationStatus".
cc: "../../../../../../dist/private/nss/pkix_revchecker.h", line 178: error 1574: Unknown size for "methodType".
gmake-3.80[5]: *** [HP-UXB.11.11_OPT.OBJ/pkix_certselector.o] Error 1

Comment 46

[Alexei Volkov]

Attachment: Patch v1. Move enums to a public header file

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 346099 [details] [diff] [review]
Patch v1. Move enums to a public header file

r=nelson

Comment 48

[Alexei Volkov]

(In reply to comment #46)
> Created an attachment (id=346099) [details]
> Patch v1. Move enums to a public header file
Committed.