363476 - vfyserv needs OCSP option

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

We're starting to get a lot of bugs about OCSP problems in mozilla browsers.
We need an NSS test tool for checking OCSP on various server certs, a tool
that can easily be built and run in a debugger.

We have a test client that exists for one purpose, to validate the cert
chains of remote SSL servers.  It is named vfyserv.  Strangely, it does 
no OCSP checking, and has no option to do so.  

vfyserv should have an option to enable NSS's built-in OCSP checking.
This is fairly urgent.

Comment 1

[Alexei Volkov]

Attachment: patch v1

adding ability to check a cert status through ocsp by enabling ocsp (-o flag) and possibly setting a default responder (-t) and it's location (-l).

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249029 [details] [diff] [review]
patch v1

This is pretty close.  

1. The usage message needs to be made/kept accurate. 
Since you're adding new options, you need to document them
in the usage message too.  While you're at it, please ensure
that the usage documents all the existing options.

2. The new variable named "responder_name" is actually a cert
nickname for the responder's cert.  Please change the variable 
name to make that more clear, and also add a comment somewhere
(e.g. by "case 'l'") explaining that this is the responder's cert.  

3. Finally,

>+	    SECU_PrintError (progName, "options -l <url> and -t "
>+	                     "<responder> should be used together");

Say "must", not "should", since it will be an error to use one 
withou the other.  The usage syntax should show both options in the 
same pair of brackets, e.g. 
  [ -l <url> -t <nickname> ]

Comment 3

[Alexei Volkov]

Attachment: patch addresses review comments

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 252698 [details] [diff] [review]
patch addresses review comments

There are two small issues with this patch that you'll have to fix 
before you commit it.  

1. It conflicts with your patch for bug 363477.  That patch eliminates 
the "-c connections" option, which this patch documents in the extended 
usage message.  

2. The usage for the "-l url" option needs to explain that, if given, 
this URL will be used INSTEAD OF the OCSP responder URL in the cert's 
AIA extension.  We shouldn't call it "default".  It's not a "default"
responder that we use when no other responder is specified.  It is an
override.  When this url is given, it is ALWAYS used instead of the 
ocsp responder URL given in the cert.

Other than those issues, r=nelson

Also, I want to see this fix (and the fix for bug 363477) go into the
NSS 3.11.6 release, as well as on the trunk.

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 252698 [details] [diff] [review]
patch addresses review comments

The code you are adding looks correct to me.

However, I predict that users of vfyserv will be confused.

For OCSP to actually get enabled, you require that option -o is given.

However, you do not list that option in Usage.

Comment 6

[Kai Engert [:KaiE:]]

r=kengert if you add a new patch that adds [-o] to the Usage command line and an explanation "-o: enable OCSP".

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 252698 [details] [diff] [review]
patch addresses review comments

Can you please add a new patch and ask me for review? thanks.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Kai, Alexei has two patches to the same program, attached to two separate bugs.
The Usage message for [-o] is present in the patch for bug 363477, but not
in this patch.  That's another usage message conflict between the two patches,
in addition to the conflict I reported in comment 4.

Alexei, I suggest you create a new "super patch" that combines the patches
for bug 363477 and the patch for this bug, and attach it to one of these
two bugs, and ask me and Kai for review.  Thanks.

Comment 9

[Alexei Volkov]

Attachment: superpatch for fixes of 363476 and 363477

Comment 10

[Kai Engert [:KaiE:]]

r=kengert

Comment 11

[Neil Williams]

Comment on attachment 253824 [details] [diff] [review]
superpatch for fixes of 363476 and 363477

dumpChain should be initialized to PR_FALSE in vfyserv.c, I think. The only assignment I see is in the switch for command options. Otherwise, r+

Comment 12

[Alexei Volkov]

Checked into 3.11 branch:

/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.11.2.3; previous revision: 1.11.2.2
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyutil.c,v  <--  vfyutil.c
new revision: 1.10.2.2; previous revision: 1.10.2.1

Comment 13

[Alexei Volkov]

Checked into trunk:

/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.14; previous revision: 1.13
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyutil.c,v  <--  vfyutil.c
new revision: 1.12; previous revision: 1.11