811317 - Add code to create a signed OCSP response

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Kai Engert [:KaiE:]]

We should have code to create a signed OCSP response.

It can help us with testing.
It can enable us to enhance selfserv to support OCSP stapling without connections to the outside world.
It can enable us to enhance httpserv to act as a OCSP server.

I have a patch that seems to work right.

Note there is some history in bug 663733. I'll morph that bug into a different subject, because the work that happened in that bug was mostly attempted cleanup.

However, I have copied "ocsp_CertStatusTemplate" from the patches from bug 663733.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1 (on top of p29 from bug 360420)

Comment 2

[Kai Engert [:KaiE:]]

Attachment: test code - not for checkin

I used this code to test patch v1.

I used this patch, and a database with the certs from mozilla/security/nss/tests/libpkix/certs (the place that has our current certs for testing against the  public OCSP server at kuix.de) and including the private keys.

This test code produces an OCSP response that looks equivalent to the data that we receive from the deployed OCSP server at kuix.de

Comment 3

[Kai Engert [:KaiE:]]

Attachment: known good OCSP response

Comment 4

[Kai Engert [:KaiE:]]

Attachment: OCSP response created using patches v1 + test code

The only difference is that I excluded the optional "nextUpdate" field.

Comment 5

[Robert Relyea]

Comment on attachment 681052 [details] [diff] [review]
Patch v1 (on top of p29 from bug 360420)

r- For just a few reasons:

1) OCSP_CreateSuccessEncodedBasicResponseV1() needs to take a pw arg for PK11_FindKeyByAnyCert(). NSS functions are not allowed to cheat on the pw_arg. Apps can because they know whether or not they can tolerate a NULL password, but if your function calls something that needs a pw_arg (also called a win_cx), you need to supply it either from your caller, or from data structure passed t you (like a key).

2) NIT: The comment /* copied from elswer .. convert len-in-bits to len-in-bytes is backwards. The code is converting length in bytes to length in bits. This make sense since you are about to pass that structure to the ASN1 encoder.

3) Don't use the deprecated DER_SetInteger, use the ASN1_ commands instead (and don't export it from nssutil).

4) arena management. Your function takes an input arena from the application, but then uses it to allocate a bunch of temparary data structures to hold intermediate results. You should allocate your own private arena for all the temp data structures and free it at the end. Only use  the user's passed in arena to allocate the results you are returning... (result = SEC_ASN1EncodeItem(arena, NULL, response, ocsp_OCSPResponseTemplate); ).

5) Question, why did you remove CERTOCSPCertStatus from the public header ocsp.h?

6) NIT: I would put the revocationTime parameter at the end of OCSP_CreateSingleReponseRevoked. It just makes it more consistent with the other two (when you see calls to all three together, they will match in their first 4 arguments, so the memory load on the programmer using these functions is just a little bit lower).

7) consistent with 3, don't export DER_SetInteger.

bob

Comment 6

[Robert Relyea]

RE DER_SetInteger. You can see nss/lib/pk11wrap/pk11pbe.c for examples on how to use SEC_ASN1EncodeInteger to encode elements that are decoded with DER_GetInteger (which is exported).

bob

Comment 7

[Kai Engert [:KaiE:]]

Thanks for the review.

I addressed all of your comments, made additional changes, and added testing!


I renamed
  OCSP_CreateSuccessEncodedBasicResponseV1
to
  OCSP_CreateSuccessResponseEncodedBasicV1
because
I find it more readable that "success response" are next to each other, and also it clarifies that it's a "basic response v1".

(I used "encoded" to the function name, just in case we eventually need a function that populates an unencoded structure.)



re 5)

I removed the typedef, because the types don't exist anywhere else, besides in this typedef.

http://mxr.mozilla.org/mozilla-central/ident?i=CERTOCSPCertStatus

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v2-A -- library patch

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v2-B -- new self-test tool, ran during tests

Comment 10

[Robert Relyea]

Comment on attachment 682621 [details] [diff] [review]
Patch v2-A -- library patch

r+,

Though please make the wincx argument the last argument. (If you don't you will have the only NSS function that I know of that doesn't have wincx last).

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch as checked in (including test)

Checking in cmd/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/manifest.mn,v  <--  manifest.mn
new revision: 1.39; previous revision: 1.38
done
RCS file: /cvsroot/mozilla/security/nss/cmd/ocspresp/Makefile,v
done
Checking in cmd/ocspresp/Makefile;
/cvsroot/mozilla/security/nss/cmd/ocspresp/Makefile,v  <--  Makefile
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/ocspresp/manifest.mn,v
done
Checking in cmd/ocspresp/manifest.mn;
/cvsroot/mozilla/security/nss/cmd/ocspresp/manifest.mn,v  <--  manifest.mn
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/cmd/ocspresp/ocspresp.c,v
done
Checking in cmd/ocspresp/ocspresp.c;
/cvsroot/mozilla/security/nss/cmd/ocspresp/ocspresp.c,v  <--  ocspresp.c
initial revision: 1.1
done
Checking in lib/certhigh/manifest.mn;
/cvsroot/mozilla/security/nss/lib/certhigh/manifest.mn,v  <--  manifest.mn
new revision: 1.10; previous revision: 1.9
done
Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.74; previous revision: 1.73
done
Checking in lib/certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.23; previous revision: 1.22
done
RCS file: /cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v
done
Checking in lib/certhigh/ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
initial revision: 1.1
done
Checking in lib/certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.221; previous revision: 1.220
done
Checking in tests/cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.61; previous revision: 1.60
done

Comment 12

[Kai Engert [:KaiE:]]

reopening, because I ran into problems on windows, see bug 813046.

In order to work around bug 813046, I crated local versions of all required templates, and avoided the use of the SEC_ASN1_MKSUB and SEC_ASN1_SUB.

In addition, it was necessary to modify my copies to work around bug 583308.

I have a patch, and I've checked it in for testing purposes.

Checking in ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
new revision: 1.5; previous revision: 1.4
done


(Note that I also checked in somve trivial bustage fixes, moving variable declarations to an earlier place in code blocks, as required by strict C.)

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Incremental patch v11 [checked in]

Comment 14

[Kai Engert [:KaiE:]]

Attachment: incremental cleanup patch, make templates static [checked in]

Windows tinderbox has started to turn green, it seems like it's confirmed that patch v11 has fixed the issue.

In addition I propose this cleanup on top of it - change the new private templates to be static, and remove the DEBUG_kaie blocks.

Comment 15

[Kai Engert [:KaiE:]]

removing dependency on bug 813046 as I've worked around the issue.

Comment 16

[Robert Relyea]

I've reviewed the new function signatures as checked in and they look fine. The only issue in the patch was pointed out in comment 10, and kai did incorporate that change before checking in.

Comment 17

[Robert Relyea]

Comment on attachment 683182 [details] [diff] [review]
Incremental patch v11 [checked in]

r+ for now.

We really should figure out why you can't access these templates rather than have to copy them. Are they just not exported?

Comment 18

[Robert Relyea]

Comment on attachment 683254 [details] [diff] [review]
incremental cleanup patch, make templates static [checked in]

r+ yes, if you copy the templates, you definately need to make them static.

Comment 19

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #17)
> 
> We really should figure out why you can't access these templates rather than
> have to copy them. Are they just not exported?

I don't think the reason was caused by templates not being exported. The templates and the code using them are both located in the same module (certhigh).

Note it was a Windows-specific problem, related to the use of the MK_SUB (etc.) macros, that are defined to use functions on Windows, only. I haven't spent the time to fully analyze, but I got unexpected results while encoding, and my theory was that the accessor functions are somehow incompatible with the ENcoder.

I agree it would be good to analyze what's going on, but given the lack of documentation and the complexity of the code, I probably saved a lot of time by copying the templates, completely avoiding the use of the accessor functions, and using the necessary nesting style that is required to work around the known encoders bugs.

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 683254 [details] [diff] [review]
incremental cleanup patch, make templates static [checked in]

Checking in ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
new revision: 1.6; previous revision: 1.5
done

Comment 21

[Kai Engert [:KaiE:]]

fixed in 3.14.1

Comment 22

[Robert Relyea]

Hmmm.. I'm not completely convinced. I know the accessor functions work when used correctly (we have lots of examples within NSS itself). I'm OK with copying the templates to get something working, but I would at least a bug written to fix the code so we don't have the copies here. The templates in util are meant to be used by others, util doesn't necessarily use those templates themselves.

bob

Comment 23

[Wan-Teh Chang]

Attachment: Suggested API changes

Kai,

I suggest some changes to the new OCSP response encoding functions.
This patch includes only the API changes so you can see them clearly.

1. Use the CERT_ prefix. We're already exported 29 OCSP-related
functions in nss.def. All of them use the CERT_ prefix. The new
functions should be consistent.

2. Add 'const' the the optional nextUpdate argument. This makes it
clear that nextUpdate is not an output argument.

3. Add an optional revocationReason argument to
CERT_CreateOCSPSingleResponseRevoked. This allows us to encode a
complete OCSP RevokedInfo structure in the future:

   RevokedInfo ::= SEQUENCE {
       revocationTime              GeneralizedTime,
       revocationReason    [0]     EXPLICIT CRLReason OPTIONAL }

4. Remove "BasicV1" from the function name. We are unlikely to have
a non-basic response type or a newer version of the basic response
type. New functionality can be supported by using extensions. The
basic response type has extensions in two places, which provide a
lot of flexibility.

5. Replace the boolean idByName argument with an enum argument. A
true/false argument is usually hard to understand without a comment.
The enum type ocspResponderIDType already exists. We just need to
export it.

6. Optional: use the ocspResponseStatus enum type instead of PRErrorCode
to specify the status of an error response. The enum type already exists
(ocspResponseStatus) and automatically constrains the errors that may be
used with CERT_CreateEncodedOCSPErrorResponse. But after I made this
change, I discovered that the corresponding decoding function,
CERT_GetOCSPResponseStatus, uses the error code to report the OCSP
response status. So I am no longer sure about this change. What do you
think?

Once you approve the API changes, I will attach the complete patch
for review. (I already have a patch that implement all the changes
described above.) Thank you.

Comment 24

[Kai Engert [:KaiE:]]

Wan-Teh, I agree to all of your proposals.

r=kaie


(In reply to Wan-Teh Chang from comment #23)
> use the ocspResponseStatus enum type instead of PRErrorCode
> ...
> CERT_GetOCSPResponseStatus, uses the error code

That existing code was probably the reason why I used PRErrorCode
for the new API.

I don't have a strong opinion, given that both approaches work.
Up to you.


> Once you approve the API changes, I will attach the complete patch
> for review. (I already have a patch that implement all the changes
> described above.) 

Thank you for your contribution to this work.

Comment 25

[Wan-Teh Chang]

Attachment: Implement API changes

Kai: this patch implements the API changes. Please review it. Thanks.

1. Use the CERT_ prefix.

2. Add 'const' the the optional nextUpdate argument to make it
clear that nextUpdate is not an output argument.

3. Add an optional revocationReason argument to
CERT_CreateOCSPSingleResponseRevoked.

4. Remove "BasicV1" from the function name.

5. Replace the boolean idByName argument with an enum argument.
The enum type ocspResponderIDType is renamed CERTOCSPResponderIDType
and exported.

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 691141 [details] [diff] [review]
Implement API changes

Wan-Teh, thank you for the cleanup and for adding additional comments.

r=kaie

Comment 27

[Wan-Teh Chang]

Comment on attachment 691141 [details] [diff] [review]
Implement API changes

Kai, thank you for the review.

Patch checked in on the NSS trunk and NSS_3_14_1_BRANCH (NSS 3.14.1).

Checking in cmd/ocspresp/ocspresp.c;
/cvsroot/mozilla/security/nss/cmd/ocspresp/ocspresp.c,v  <--  ocspresp.c
new revision: 1.3; previous revision: 1.2
done
Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.75; previous revision: 1.74
done
Checking in lib/certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.24; previous revision: 1.23
done
Checking in lib/certhigh/ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
new revision: 1.9; previous revision: 1.8
done
Checking in lib/certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.12; previous revision: 1.11
done
Checking in lib/certhigh/ocspti.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.9; previous revision: 1.8
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.222; previous revision: 1.221
done

Checking in cmd/ocspresp/ocspresp.c;
/cvsroot/mozilla/security/nss/cmd/ocspresp/ocspresp.c,v  <--  ocspresp.c
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in lib/certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.74.2.1; previous revision: 1.74
done
Checking in lib/certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.23.2.1; previous revision: 1.23
done
Checking in lib/certhigh/ocspsig.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspsig.c,v  <--  ocspsig.c
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in lib/certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.11.2.1; previous revision: 1.11
done
Checking in lib/certhigh/ocspti.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v  <--  ocspti.h
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.221.2.1; previous revision: 1.221
done

Comment 28

[Kai Engert [:KaiE:]]

Comment on attachment 691141 [details] [diff] [review]
Implement API changes

>Index: mozilla/security/nss/lib/certhigh/ocspti.h
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/certhigh/ocspti.h,v
>retrieving revision 1.8
>diff -u -p -r1.8 ocspti.h
>--- mozilla/security/nss/lib/certhigh/ocspti.h	25 Apr 2012 14:49:27 -0000	1.8
>+++ mozilla/security/nss/lib/certhigh/ocspti.h	12 Dec 2012 00:21:29 -0000
>@@ -189,14 +189,14 @@ struct CERTOCSPCertIDStr {
>  * }
>  */
> typedef enum {
>+    ocspResponse_other = -1,		/* unknown/unrecognized value */
>     ocspResponse_successful = 0,
>     ocspResponse_malformedRequest = 1,
>     ocspResponse_internalError = 2,
>     ocspResponse_tryLater = 3,
>     ocspResponse_unused = 4,
>     ocspResponse_sigRequired = 5,
>-    ocspResponse_unauthorized = 6,
>-    ocspResponse_other			/* unknown/unrecognized value */
>+    ocspResponse_unauthorized = 6
> } ocspResponseStatus;
> 
> /*

This change introduced a crash (assertion failure) in ocspclnt, which tries to assert that a valid status is <= ocspResponse_other

Wan-Teh, do you prefer to revert the fix or to adjust the tool? (Besides ocspclnt I also ran into this problem with my ssltap enhancements for OCSP stapling)

Comment 29

[Wan-Teh Chang]

Kai: it would be better to add an ocspResponse_max or ocspResponse_endOfList
enum value for this purpose, and leave ocspResponse_other as -1 to make it
clear it is an invalid value. What do you think?

Comment 30

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #29)
> Kai: it would be better to add an ocspResponse_max or ocspResponse_endOfList
> enum value for this purpose, and leave ocspResponse_other as -1 to make it
> clear it is an invalid value. What do you think?

I agree. I've filed bug 833857.

Wan-Teh, could you please review in that bug? Thank you in advance.

Comment 31

[Wan-Teh Chang]

Note: after looking at the ocspResponse_other problem in more detail, I now
recommend reverting my change and optionally making alternative changes.
Please follow up in bug 833857.