186619 - Export the remaining OCSP functions declared in ocsp.h

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Wan-Teh Chang]

We should review the three functions declared in ocsp.h
that are not yet exported and see if they can be exported.
They are:
CERT_DecodeOCSPRequest
CERT_GetEncodedOCSPResponse
CERT_CheckOCSPStatus

They are only used by our own ocspclnt test program right
now.  If they are exported, we will be able to link ocspclnt
with our shared libraries.

But the purpose of this bug is to be more proactive in
exporting our public functions.  We have had a couple of
patch releases whose main purpose was to export some
functions that were already implemented.  I want to avoid
that.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P2.  I'm interpreting all the recent activity in reviewing our OCSP
capabilities as a sign of demand for OCSP features.

Comment 2

[Konstantin Andreev]

One more function, declared in ocsp.h, but not exported: 
  CERT_ParseURL

Comment 3

[Elio Maldonado]

I was about to enter a similar bug when I found this one. Please do export these functions. 
See https://bugzilla.redhat.com/show_bug.cgi?id=689918#c31 and subsequent comments.

Comment 4

[Elio Maldonado]

Attachment: Exports OCSP functions

Comment 5

[Wan-Teh Chang]

Comment on attachment 627319 [details] [diff] [review]
Exports OCSP functions

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

r=wtc.  Please create another patch to make the following
changes, before exporting these functions.

Will this allow us to link the ocspclnt program with NSS
shared libraries/DLL?

::: ./mozilla/security/nss/lib/nss/nss.def
@@ +1002,5 @@
>  ;+};
> +;+NSS_3.14 {    # NSS 3.14 release
> +;+    global:
> +CERT_CheckOCSPStatus
> +CERT_DecodeOCSPRequest

Can you add 'const' to the "SECItem *src" argument?

@@ +1003,5 @@
> +;+NSS_3.14 {    # NSS 3.14 release
> +;+    global:
> +CERT_CheckOCSPStatus
> +CERT_DecodeOCSPRequest
> +CERT_GetEncodedOCSPResponse

Can you add 'const' to the "char *location" argument?

@@ +1004,5 @@
> +;+    global:
> +CERT_CheckOCSPStatus
> +CERT_DecodeOCSPRequest
> +CERT_GetEncodedOCSPResponse
> +CERT_ParseURL

Why do you need this function exported?

NOTE: this function is not OCSP-specific.  I suggest that
we move the declaration from ocsp.h to cert.h, if you need
to export it.  (It probably can even be moved to lib/util.)

Comment 6

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #5)
> Comment on attachment 627319 [details] [diff] [review]
> Exports OCSP functions
> 
> Review of attachment 627319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=wtc.  Please create another patch to make the following
> changes, before exporting these functions.
> 
> Will this allow us to link the ocspclnt program with NSS
> shared libraries/DLL?
This will and on second round of testing it seems I can link okay even without it. But it;s preferable that ocspclnt and most tools rely on exported functions. Some tools, such as blapitest and fipstest, by their very nature need to poke inside but that's not the case with ocsptclnt.
> 
> ::: ./mozilla/security/nss/lib/nss/nss.def
> @@ +1002,5 @@
> >  ;+};
> > +;+NSS_3.14 {    # NSS 3.14 release
> > +;+    global:
> > +CERT_CheckOCSPStatus
> > +CERT_DecodeOCSPRequest
> 
> Can you add 'const' to the "SECItem *src" argument?
Definitely.
> 
> @@ +1003,5 @@
> > +;+NSS_3.14 {    # NSS 3.14 release
> > +;+    global:
> > +CERT_CheckOCSPStatus
> > +CERT_DecodeOCSPRequest
> > +CERT_GetEncodedOCSPResponse
> 
> Can you add 'const' to the "char *location" argument?
Yes indeed.
> 
> @@ +1004,5 @@
> > +;+    global:
> > +CERT_CheckOCSPStatus
> > +CERT_DecodeOCSPRequest
> > +CERT_GetEncodedOCSPResponse
> > +CERT_ParseURL
> 
> Why do you need this function exported?
Same as first comment, tool higiene perhaps. This bug stands in its own merit for the original reasons. 
> 
> NOTE: this function is not OCSP-specific.  I suggest that
> we move the declaration from ocsp.h to cert.h, if you need
> to export it.  (It probably can even be moved to lib/util.)

Agrree, cert.h is a better home than ocsp.h for this function. 

Moving to lib/util, though possible, has a couple problems. 
1) There is no existing util public header which would be a good home for CERT_ParseURL
2) parsing a URL is a higher level service of NSS. We do want to keep util as the provider of basic low level utility functions for both nss (higher layers) and softoken (with freebl), the later we now package stand-alone in fedora.

Comment 7

[Elio Maldonado]

Attachment: V2: Exports OCSP functions

Address wtc's review comments and added missing semi-colons.

Comment 8

[Wan-Teh Chang]

Comment on attachment 627769 [details] [diff] [review]
V2: Exports OCSP functions

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

r=wtc.  Thanks.

::: ./mozilla/security/nss/lib/nss/nss.def
@@ +1004,5 @@
> +;+    global:
> +CERT_CheckOCSPStatus;
> +CERT_DecodeOCSPRequest;
> +CERT_GetEncodedOCSPResponse;
> +CERT_ParseURL;

I'm not sure if CERT_ParseURL needs to be exported.
This MXR query shows that none of the NSS cmds is
using it: http://mxr.mozilla.org/security/ident?i=CERT_ParseURL

Since you moved the declaration of CERT_ParseURL to
a different header, please check for compiler warnings.

Comment 9

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #8)
> Comment on attachment 627769 [details] [diff] [review]
> V2: Exports OCSP functions
> 
> Review of attachment 627769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=wtc.  Thanks.
> 
> ::: ./mozilla/security/nss/lib/nss/nss.def
> @@ +1004,5 @@
> > +;+    global:
> > +CERT_CheckOCSPStatus;
> > +CERT_DecodeOCSPRequest;
> > +CERT_GetEncodedOCSPResponse;
> > +CERT_ParseURL;
> 
> I'm not sure if CERT_ParseURL needs to be exported.
> This MXR query shows that none of the NSS cmds is
> using it: http://mxr.mozilla.org/security/ident?i=CERT_ParseURL

I'm not sure we need to do it either. Also, this function looks a bit out of place in cert.h. Would you mind if I don't export it right away?

Comment 10

[Elio Maldonado]

Attachment: V3: Export OCSP functions - the ones we are sure we need

For the time being I feel more confident checking in this one instead.

Comment 11

[Wan-Teh Chang]

Comment on attachment 628411 [details] [diff] [review]
V3: Export OCSP functions - the ones we are sure we need

r=wtc.  I am fine with not doing anything with CERT_ParseURL.

(CERT_ParseURL was requested to be exported by Konstantin Andreev
in comment 2.)

I think CERT_ParseURL can be moved to an internal header such as
certi.h, and renamed with a lower case prefix: cert_ParseURL.  We
can deal with that later.

Comment 12

[Elio Maldonado]

Changes committed on the trunk:
Checking in ./mozilla/security/nss/lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.71; previous revision: 1.70
done
Checking in ./mozilla/security/nss/lib/certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.21; previous revision: 1.20
done
Checking in ./mozilla/security/nss/lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.218; previous revision: 1.217
done

Comment 13

[Wan-Teh Chang]

Attachment: Fix compiler warnings and comment errors

We need to propagate 'const' to the functions that receive
the 'const char *location' argument. Also, the comments need
to be updated to match the current 'const char *' type.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.72; previous revision: 1.71
done
Checking in ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.22; previous revision: 1.21
done