983853 - Add GenerateOCSPResponses tool needed by OCSP unit tests

Status: RESOLVED INCOMPLETE

(Core :: Security: PSM, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #916629 +++

Splitting this out of bug 916629 so that other bugs don't get blocked on all the reviews of tests in bug 916629.

This bug will be about modifying the build configuration to support the building of the GenerateOCSPResponses tool only, including the minimal functionality of GenerateOCSPResponses. GenerateOCSPResponses won't actually be used until bug 916629 is fixed.

Comment 1

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

Attachment: Part 1: build configuration changes (reviewed by gps in bug 916629)

Comment 2

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

Attachment: rename-CreateEncodedOCSPresponse.patch

r=keeler from IRC:

<briansmith> I think it would be better if we renamed the current pkixtestutil.cpp to CreateEncodedOCSPResponse.cpp and then added the new junk into a new pkixtestutil.cpp
<briansmith> does that seem reasonable to you?
<keeler> sure - that sounds fine

Comment 3

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

Attachment: add-GenerateOCSPResponses.patch

David, most of the build config stuff has been reviewed by GPS in bug 916629, but it would be good for you to have a look at it.

Camilo reviewed the GenerateOCSPResponses.cpp code in bug 916629, too, but he had had trouble getting it to build because of the dependency on secutil. I fixed the linkage problem with secutil so things should be all good now.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8391554 [details] [diff] [review]
add-GenerateOCSPResponses.patch

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

LGTM. Just some nits/questions.

::: security/insanity/test/cmd/moz.build
@@ +28,5 @@
> +    '../../include',
> +    '../lib',
> +]
> +
> +CFLAGS += CONFIG['TK_CFLAGS']

Isn't this just for UI stuff? Are we cargo-culting here?

::: security/insanity/test/lib/pkixtestutil.cpp
@@ +37,5 @@
> +  ScopedFILE file;
> +#ifdef _MSC_VER
> +  {
> +    FILE* rawFile;
> +    if (fopen_s(&rawFile, path.get(), mode)) {

!= 0?

::: security/manager/ssl/tests/unit/tlsserver/cmd/Makefile.in
@@ +13,5 @@
>    ../../../../../../insanity/$(LIB_PREFIX)insanitypkix.$(LIB_SUFFIX) \
>    ../../../../../../insanity/test/lib/$(LIB_PREFIX)pkixtestutil.$(LIB_SUFFIX) \
>    ../lib/$(LIB_PREFIX)tlsserver.$(LIB_SUFFIX) \
>    $(NULL)
>  

not a big deal, but this leaves an unnecessary empty line at the end of this file

::: security/manager/ssl/tests/unit/tlsserver/cmd/moz.build
@@ +19,5 @@
>  LOCAL_INCLUDES += [
>      '../lib',
>  ]
>  
> +CFLAGS += CONFIG['TK_CFLAGS']

Again, necessary?

Comment 5

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

Thanks for the review David.

I abandoned this approach and redid the OCSP response generation and test cert generation based on your work on pkixtestutil.cpp. Now, it is no longer necessary for us to have a separate process for signing the OCSP responses because we can (will be able to) sign OCSP responses without creating CERTCertificate objects that would polute the NSS CERTCertificate caches. Avoiding the need for the separate process simplifies things considerably.