476979 - Add cert chain tests that will do cert validation for different ku/eku

Status: REOPENED

(NSS :: Tools, enhancement, P2)

Description

[Alexei Volkov]

Attachment: certutil patch v1 - accept ku/eku extension values through command line

Need to have tests that check that old and libpkix validation code paths return same statuses. One of the things that need to test is cert chain validation for different cert usages. 

I suggest we add tests that will check a variety cert usages to our cert chain tests. To simplify the task, I've modify certutil to be able to accept values for key usage and extended key usage extension through the command line. Next step will be to change chains.sh script to use the feature.

Comment 1

[Alexei Volkov]

Attachment: certchain.sh patch v1 - add ability to generate and test certs for different usages

Comment 2

[Alexei Volkov]

Comment on attachment 360642 [details] [diff] [review]
certchain.sh patch v1 - add ability to generate and test certs for different usages

Slavo, the patch adds feature to control ku/eku when cert get created. Also it adds a set of misc options to vfychain. misc options will be use to pass "-u 1"  to the vfychain. Please review.

Comment 3

[Alexei Volkov]

Attachment: chains.sh patch v2 - add possibility to test legacy code with some chains tests.

Comment 4

[Slavomir Katuscak]

Comment on attachment 360646 [details] [diff] [review]
chains.sh patch v2 - add possibility to test legacy code with some chains tests.

Should be OK, just use "legacy" instead of "lagacy".

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360641 [details] [diff] [review]
certutil patch v1 - accept ku/eku extension values through command line

I have not finished the review of this patch, but I have some initial
findings.  This patch has a lot of lines that are longer than 80 characters. 
It also creates lines in the output of the usage message that are longer
than 80 characters.  

Here are examples of lines that are both (I have truncated these).

>+        "\t\t [-f pwfile] [-d certdir] [-P dbprefix] [-1 | --keyUsage [keyUsageK
>+        "\t\t [-2] [-3] [-4] [-5] [-6 | --extKeyUsage [extKeyUsageKeyword,...]]\

It helps a lot to break the strings at the \n characters.  It also helps to 
adjust the spacing of the source lines, so that the end of the \t\t or %-20s 
comes at the offset in the source line where the output will appear when 
displayed.  Then you can judge whether the output will be too long by 
whether the source is too long. E.g. instead of 

>+        "\t\t [-f pwfile] [-d certdir] [-P dbprefix] [-1 | --keyUsage [keyUsageK
>+        "\t\t [-2] [-3] [-4] [-5] [-6 | --extKeyUsage [extKeyUsageKeyword,...]]\

have
  V--16 Spaces---V
>+           "\t\t [-f pwfile] [-d certdir] [-P dbprefix] [-1 | --keyUsage  ...
>+           "\t\t [-2] [-3] [-4] [-5] [-6 | --extKeyUsage                  ...

and instead of 

>+    FPS "%-20s \n%-20s Create key usage extension. Possible keywords: \
>+\"digitalSignature\"\n%-20s \"nonRepudiation\", \"keyEncipherment\", \
>+\"dataEncipherment\", \"keyAgreement\"\n%-20s \"certSigning\", \"crlSigning\", \"critical\"\n",

have this:
  V---- 20 spaces ---v
>+    FPS       "%-20s \n"
>+              "%-20s Create key usage extension. Possible keywords:\n"
>+              "%-20s \"digitalSignature\", \"nonRepudiation\",\n"
>+              "%-20s \"keyEncipherment\", \"dataEncipherment\",\n"
>+              "%-20s \"keyAgreement\", \"certSigning\", \"crlSigning\",\n"
>+              "%-20s \"critical\"\n",

and instead of  This really LOOOONG source line:

>+    FPS "%-20s  \n%-20s Create extended key usage extension. Possible keywords: \"serverAuth\",\n%-20s \"clientAuth\",\"codeSigning\",\"emailProtection\",\"timeStamp\",\"ocspResponder\",\n%-20s \"stepUp\", \"critical\"\n",

have 
  V---- 20 spaces ---v
>+    FPS       "%-20s \n"
>+              "%-20s Create extended key usage extension. Possible keywords:\n"
>+              "%-20s \"serverAuth\", \"clientAuth\", \"codeSigning\",\n" >+              "%-20s \"emailProtection\", \"timeStamp\", \"ocspResponder\",\n"
>+              "%-20s \"stepUp\", \"critical\"\n",

Here are some more long lines truncated

>+        certutil_extns[ext_extKeyUsage].activated =
>+				certutil.options[opt_AddCmdExtKeyUsageExt].activat
>+        if (!certutil_extns[ext_extKeyUsage].activated) {
>+            certutil_extns[ext_extKeyUsage].activated =
>+                                 certutil.options[opt_AddExtKeyUsageExt].activat

>-        certutil_extns[ext_certPolicies] =
>+        certutil_extns[ext_certPolicies].activated =
> 				certutil.options[opt_AddCertPoliciesExt].activated

>-        certutil_extns[ext_policyConstr] =
>+        certutil_extns[ext_policyConstr].activated =
> 				certutil.options[opt_AddPolicyConstrExt].activated

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360641 [details] [diff] [review]
certutil patch v1 - accept ku/eku extension values through command line

Browsers make lousy source code editors.  That was supposed to be:

  V---- 20 spaces ---v
>+    FPS       "%-20s \n"
>+              "%-20s Create extended key usage extension. Possible keywords:\n"
>+              "%-20s \"serverAuth\", \"clientAuth\", \"codeSigning\",\n" 
>+              "%-20s \"emailProtection\", \"timeStamp\", \"ocspResponder\",\n"
>+              "%-20s \"stepUp\", \"critical\"\n",

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360641 [details] [diff] [review]
certutil patch v1 - accept ku/eku extension values through command line

In addition to my previous comments, I have these comments about the
patch to certext.c.

Make strings and string variables const where possible and logical.
For example, 

>+parseNextCmdInput(char **valueArray, int *value,  char **nextPos,

Those should both be const char **, and perhaps valueArray should be
const char * const * (saying that the characters are const, and the
pointers in the array of pointers are also const).

>+    while (valueArray[arrIndex]) {
>+        if (...
>+        arrIndex += 1;
>+    }

Please make that a for loop, e.g. 
      for (arrIndex = 0; valueArray[arrIndex]; arrIndex++) {

>+static char *
>+keyUsageKeyWordArray[] = { "digitalSignature",

mke this static const char * const ...

> static SECStatus 
>-AddKeyUsage (void *extHandle)
>+AddKeyUsage (void *extHandle, const char *userSuppliedValue)

All the values are user supplied, but some are supplied on the 
command line, so maybe the name should say that the value comes
from the command line?

>+            value = PORT_Atoi (buffer);
>+            if (value < 0 || value > 6)
>+                break;
>+            if (value == 0) {
>+                /* Checking that zero value of variable 'value'
>+                 * corresponds to '0' input made by user */
>+                char *chPtr = strchr(buffer, '0');
>+                if (chPtr == NULL) {
>+                    continue;
>+                }
>+            }

That test, to try to differentiate between an invalid input and a 
zero input, is the wrong test, for two reasons.  The big reason is
that PORT_Atoi (which is just atoi) is defined to have UNDEFINED 
behavior on any error.  One cannot count on it to return zero on 
error.  That is the difference between atoi(X) and strtol().  Other 
that that, atoi(x) is equivalent to (int)strtol(X, (char **)NULL, 10).

The second reason is that the test will come to the wrong conclusion
on some inputs.  For example, consider the input "A0B".  According to 
the various man pages (including windows), the right way to detect 
error is to set errno to zero before calling strtol, and then check 
errno for non-zero afterwords.  

>+static char*
>+extKeyUsageKeyWordArray[] = { "serverAuth",

should be const char * const ...

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Oh, one option is to write a new PORT_Atoi function that calls strol, and 
so is fully equivalent to it.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

PORT_Atoi could even set an NSPR error, so that the caller would not need 
to check errno, but could call PORT_GetError.

Comment 10

[Alexei Volkov]

Attachment: certutil patch v2 - address review comments of patch v1

Comment 11

[Alexei Volkov]

Attachment: secport.h - change PORT_Atoi to use strtol

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 361371 [details] [diff] [review]
secport.h - change PORT_Atoi to use strtol


>-#define PORT_Atoi 	atoi
>+#define PORT_Atoi(buff)	strtol(buff, NULL, 10)

Please add an explicit cast to int, e.g. 
                           (int)strtol(buff, NULL, 10)
then r=nelson

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 361369 [details] [diff] [review]
certutil patch v2 - address review comments of patch v1

r=nelson

The only remaining problem I see in this code is the use of strchr to attempt to detect failures of strtol.  Please submit a supplemental patch to deal with that.

Comment 14

[Alexei Volkov]

Attachment: v1 certext.c patch to fix atoi error handling

It also fixes the warning in keystuff.c

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362012 [details] [diff] [review]
v1 certext.c patch to fix atoi error handling

The patch appears to be OK, except for this hunk, which changes 
the behavior in the error case.

>@@ -962,14 +953,11 @@ AddCrlDistPoint(void *extHandle)
>                 buffer, sizeof(buffer)) == SECFailure) {
>             GEN_BREAK(SECFailure);
>         }
>+        errno = 0;
>         intValue = PORT_Atoi (buffer);
>-        if (intValue == 0) {
>-            /* Checking that zero value of variable 'value'
>-             * corresponds to '0' input made by user */
>-            char *chPtr = strchr(buffer, '0');
>-            if (chPtr == NULL) {
>-                intValue = -1;
>-            }
>+        if (errno) {
>+            /* can not parse user input */
>+            continue;
>         }

Comment 16

[Alexei Volkov]

Attachment: v2 certext.c patch to fix atoi error handling

Fix the last hunk and fix a warning in keystuff.c

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 362984 [details] [diff] [review]
v2 certext.c patch to fix atoi error handling

r=nelson

Comment 18

[Alexei Volkov]

integrated.

Comment 19

[Slavomir Katuscak]

Alexei, this patch broke another test:

cert.sh: #438: Certificate Key Usage Extension (11) - Looking for Digital Signature returned 0, expected is 1 - FAILED

See cert.sh - cert_extensions_test() and certext.txt - TEST_11 Certificate Key Usage Extension.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Alexei just reported that Solaris's strtol does not set errno.  :-(
So, we need to have a platform-dependent ifdef, and continue to do it 
the "wrong" way for Solaris only.

Comment 21

[Alexei Volkov]

Slavo, please work on adding these tests as a part of "chains" test suite.

Comment 22

[Slavomir Katuscak]

Attachment: Patch for chains.sh to support ku/eku/ns. (checked in)

Comment 23

[Slavomir Katuscak]

Attachment: Cert usages matrix.

List of all combination to test - Alexei please verify if this matrix is OK, in my tests I got those tests that doesn't match expected results:

SSL_SERVER/KEY ENCIPHERMENT (rsa) x SSL StepUp 
SSL_SERVER/KEY AGREEMENT (dh) x SSL Server
SSL_SERVER/KEY AGREEMENT (dh) x SSL StepUp 
SSL_SERVER/DIGITAL SIGNATURE (dsa) x SSL Server
SSL_SERVER/DIGITAL SIGNATURE (dsa) x SSL StepUp 
SSL_SERVER/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x SSL Server
SSL_SERVER/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x SSL StepUp 
SSL_SERVER/KEY AGREEMENT (dh) x Email recipient
EMAIL/DIGITAL SIGNATURE (dsa) x Email signer 
EMAIL/DIGITAL SIGNATURE (dsa) x Email recipient
EMAIL/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x Email signer 
EMAIL/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x Email recipient
SSL_CA/CERT SIGN x OCSP responder
EMAIL_CA/CERT SIGN x OCSP responder
OBJECT SIGNING CA/CERT SIGN x OCSP responder

Comment 24

[Slavomir Katuscak]

Fix for list of failing tests:

EMAIL/KEY AGREEMENT (dh) x Email recipient
instead of
SSL_SERVER/KEY AGREEMENT (dh) x Email recipient

Comment 25

[Slavomir Katuscak]

Attachment: Script to run the tests.

Requires patch for chains.sh (attachment 384867 [details] [diff] [review]). Expected results are set by attached matrix (failing tests are marked).

For now I don't use rsa/dsa/dh/ecc options, please provide the steps how to use them. I know that rsa is detault and for dsa I can use -k dsa, for ecc I can use -k ecc -q curve (which curve to use ??). I don't know what about the rest (dh ??).

Comment 26

[Alexei Volkov]

Attachment: Fix typo in data for nc cert type extension(committed).

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 385144 [details] [diff] [review]
Fix typo in data for nc cert type extension(committed).

r=nelson

Comment 28

[Slavomir Katuscak]

Comment on attachment 384867 [details] [diff] [review]
Patch for chains.sh to support ku/eku/ns. (checked in)

Checking in chains.sh;
/cvsroot/mozilla/security/nss/tests/chains/chains.sh,v  <--  chains.sh
new revision: 1.20; previous revision: 1.19
done

Comment 29

[Slavomir Katuscak]

Alexei, please verify if expected results are correct (see Cert usages matrix - attachment), results differs from expected in those tests:

SSL_SERVER/KEY ENCIPHERMENT (rsa) x SSL StepUp 
SSL_SERVER/KEY AGREEMENT (dh) x SSL Server
SSL_SERVER/KEY AGREEMENT (dh) x SSL StepUp 
SSL_SERVER/DIGITAL SIGNATURE (dsa) x SSL Server
SSL_SERVER/DIGITAL SIGNATURE (dsa) x SSL StepUp 
SSL_SERVER/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x SSL Server
SSL_SERVER/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x SSL StepUp 
EMAIL/KEY AGREEMENT (dh) x Email recipient
EMAIL/DIGITAL SIGNATURE (dsa) x Email signer 
EMAIL/DIGITAL SIGNATURE (dsa) x Email recipient
EMAIL/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x Email signer 
EMAIL/DIGITAL SIGNATURE or AGREEMENT (ecc dsa/dh) x Email recipient
SSL_CA/CERT SIGN x OCSP responder
EMAIL_CA/CERT SIGN x OCSP responder
OBJECT SIGNING CA/CERT SIGN x OCSP responder

Comment 30

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

These tests look useful we should consider rebasing them on top of the current NSS trunk and checking them in.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 32

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.