152426 - delegation of HTTP download for OCSP

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Julien Pierre]

Currently, NSS connects directly over HTTP to the OCSP responder to make the
request and fetch the response. It does so in a blocking fashion, and also
directly to the responder, bypassing any proxy the application may wish to use
instead. This problem may be resolved by isolating the download code from the
rest of the OCSP processing code in NSS.

Comment 1

[Wan-Teh Chang]

This problem is difficult to solve if the application
is single-threaded and uses an event loop to do
non-blocking or asynchronous I/O.  In this model, no
operation can block for a long time, so it seems
impossible for NSS to do OCSP transparently.

I suggest that we have a meeting to discuss how to
do OCSP in single-threaded NSS clients.

Comment 2

[Wan-Teh Chang]

*** Bug 163717 has been marked as a duplicate of this bug. ***

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Marking P2.  Not a vulnerability.

Comment 4

[Wan-Teh Chang]

At the NSS OCSP meeting on May 1, we decided that it is
better to provide a way for a client of NSS to register
callbacks for talking to an OSCP responder, than to
improve the HTTP client in NSS to handle proxy servers.

Comment 5

[Serge Gautherie (:sgautherie)]

Was any progress made on this issue since the meeting 11 months ago ?

Comment 6

[Julien Pierre]

We will deal with this problem in NSS 3.12 with libpkix .

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v1

I would like to find a short term solution for Firefox, as demand for OCSP within proxy environments is high within corporate environments.


The attached patch provides a simple callback API.

It was tested with patch v1 in bug 111384 and makes OCSP support over proxies work, enabling NSS to make use of Mozilla's internal http/networking engine (Necko).

However, it is a synchronous API.
It assumes that NSS will be able to wait until the OCSP request is done (as it is currently implemented in NSS 3.11).


I talked to Julien. He is concerned an synchronous API might not be appropriate for future versions of NSS 3.x, that will contain libpkix. An asynchronous API would be more desirable.

If I understand correctly, we have the following options:


1)
- Accept this synchronous API now (proposed for 3.11.1)
- Should libpkix require it, add an additional asynchronous callback API later

An application could choose which callbacks it is able to implement, and NSS could detect which client abilitiy is available and behave accordingly.


2)
- Decide that only one callback API should be present in NSS, and that it should be an asynchronous API

Such an API would probably look similar to this: 
a) NSS calls application callback function requestAsyncOcsp(void *userdata)
b) Once the client is done, it calls back into NSS function asyncOcspCompleted(void *userdata, resultCode, responseBlob)

Once that API is defined, in order to provide a short term solution, we add the  API to NSS 3.11.1, but will use it in a synchronous way, i.e. trigger the request, and block until the response arrives.

I think, because an NSS API should not be changed, the tricky part is to define the API now, while making sure it suits all needs of the future NSS+libpkix version. Are you already aware of all the requirements to define the stable async API?


Do you prefer 1) or 2) ?

Comment 8

[Julien Pierre]

Between 1 and 2, I would clearly prefer 2 . 1) will lead to obsolete code, and we can't just drop functionality from NSS .
However, 2) will be very difficult to achieve. Until we have more visibility on the new top-level Cert APIs, which we don't now, it will be hard to freeze the callback API without possibly being constrained and needing to change it before 3.12 is released, which is still many months away. I think we need to take the time to solve this problem correctly in NSS 3.12.

Since you have a short time need that can't wait for 3.12, which is specific to PSM, I would much prefer if you went the following option :

3) implement an OCSP client in PSM using Necko and the existing NSS APIs to encode OCSP requests / decode OCSP responses. 
This way, no changes to NSS 3.11.x are needed . And we only have to define one callback interface, in NSS 3.12 .

Comment 9

[Kai Engert (:KaiE:)]

> 3) implement an OCSP client in PSM using Necko and the existing NSS APIs to
> encode OCSP requests / decode OCSP responses. 

Julien, can you please explain more?

Which specific NSS API, already contained in NSS 3.11, should be used by PSM, to allow NSS to call back into PSM's OCSP client - whenever NSS decides that it wants to OCSP-verify a certificate?

Comment 10

[Julien Pierre]

Kai,

Re: comment #9

I wasn't talking about an OCSP client that NSS would callback into. What I'm suggesting is that you turn off the NSS OCSP client completely. PSM can continue to call CERT_VerifyCertXXX, and when it's done, add a call to PSM_OCSPCheck, which would be implemented using Necko and NSS .
The most relevant NSS APIs this client would use are are CERT_EncodeOCSPRequest and CERT_DecodeOCSPResponse .

You could duplicate the code in NSS to extract the URL from the AIA extension.
Even though this client will be throw-away code for PSM, it is better than encumbering the NSS API with something temporary that will need to be supported long-term yet won't be the right thing for 3.12.

Comment 11

[Peter Renner]

*** Bug 323872 has been marked as a duplicate of this bug. ***

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 13

[Julien Pierre]

Comment on attachment 209372 [details] [diff] [review]
Patch v2

Kai,

In your patch v2, the OCSP_Global structure isn't protected by a lock. I'm thinking it probably should be, and every user that dereferences it should acquire that lock and make a local copy before using it. The lock should only be held during the time to make the copy.

In SEC_RegisterDefaultHttpClient, we should allow the fcnTable argument to be const. In fact, we should require the function table to be a global object in the documentation for the API, so that a call to the OCSP code can still access the old function table after it has made a copy and released the lock. There is no syntactic way to require that the table be persistent, unfortunately, so it just has to be documented in the header comments.

In fetchOcspHttpClientV1, I think the code can be simplified a little bit by making use of goto and having all the variables cleaned up at the end. For example this would allow freeing the session only once. 

myHttpResponsecode shouldn't be unsigned int. We should use an NSPR type such as PRUint16 . This should be corrected in the API definition for TrySendAndReceive.
This also goes for the type of http_response_data_len, and http_data_len in SetPostData .

I'm concerned about the risk of unlimited memory growth when reading the HTTP response. We could make the http_response_data_len an input/output argument to TrySendAndReceive. If set to non-zero, this would be the maximum response size to read. We should allow for a reasonable maximum for the response, perhaps 64 KB ?

I also think the copy of the encodedResponse from a buffer to a newly allocated SECItem is inefficient, but this is less of a problem if we bound the response size.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v3

> In your patch v2, the OCSP_Global structure isn't protected by a lock. I'm
> thinking it probably should be, 

I've added a lock to the structure.
But this adds the requirement to initialize the lock early.
I've added a (non-exported) function to initialize it,
and call it from NSS' init function.


> and every user that dereferences it should
> acquire that lock and make a local copy before using it. 

Fine with me. I propose the struct should just be the container to
hold the reference, and code inside NSS that uses it should refer
to a function to access it.
I've added (non-exported) function GetRegisteredHttpClient().


> The lock should only
> be held during the time to make the copy.

Using function GetRegisteredHttpClient() ensures that.


> In SEC_RegisterDefaultHttpClient, we should allow the fcnTable argument to be
> const. In fact, we should require the function table to be a global object in
> the documentation for the API, so that a call to the OCSP code can still access
> the old function table after it has made a copy and released the lock. There is
> no syntactic way to require that the table be persistent, unfortunately, so it
> just has to be documented in the header comments.

I changed it to be const and added a comment.


> In fetchOcspHttpClientV1, I think the code can be simplified a little bit by
> making use of goto and having all the variables cleaned up at the end. For
> example this would allow freeing the session only once. 

I personally don't like using goto, but I followed your request
and changed the code accordingly.


> myHttpResponsecode shouldn't be unsigned int. We should use an NSPR type such
> as PRUint16 . This should be corrected in the API definition for
> TrySendAndReceive.

Changed.


> This also goes for the type of http_response_data_len, and http_data_len in
> SetPostData .

I changed both of them to PRUint32, because I think we should not restrict
the API to 16 bit sizes.


> I'm concerned about the risk of unlimited memory growth when reading the HTTP
> response. We could make the http_response_data_len an input/output argument to
> TrySendAndReceive. If set to non-zero, this would be the maximum response size
> to read. 

I've changed the API accordingly and added a comment explaining the
in/out parameter.


> We should allow for a reasonable maximum for the response, perhaps 64 KB ?

If you think that's a reasonable size for OCSP responses, fine with me.
I've changed the implementation of NSS that calls the API to use this limit.


> I also think the copy of the encodedResponse from a buffer to a newly allocated
> SECItem is inefficient, but this is less of a problem if we bound the response
> size.

I agree this is not perfect.
However, this is for the initial implementation only.
Later implementations of NSS can optimize based on the fact, that pointers
returned by the HTTP client will remain valid until the sessions get freed.

Comment 15

[Julien Pierre]

Comment on attachment 209574 [details] [diff] [review]
Patch v3

Kai,

Thanks for updating the patch so quickly. V3 looks much better.
I only have a few nits :

- please use the style of the if and { braces prevalent in the rest of the file, ie.
if (condition) {
   code;
}

- there are some unnecessary whitespace lines in SEC_RegisterDefaultHttpClient , InitOCSPGlobal, and many other functions. Generally there is no good reason to have two consecutive white lines in the middle of the code

- for myHttpResponseDataLen, please add a macro with comments rather than just initialize it to 64*1024, so this limit is exposed and can be changed easily

Re: http_data_len and http_response_data_len, I didn't mean to limit them to 16-bit types, just to use NSPR types. Thanks for doing so.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Patch v4

>- please use the style of the if and { braces prevalent in the rest of the
> file, ie.

I changed all braces to your prefered style


> - there are some unnecessary whitespace lines in SEC_RegisterDefaultHttpClient
> , InitOCSPGlobal, and many other functions. Generally there is no good reason
> to have two consecutive white lines in the middle of the code

I was unable to find any occurrence of two consecutive whitespace lines in my patch?!?
I don't know what you consider unnecessary, the existing code (not my patch) seems to use whitespace lines, too, as a matter to increase readability.
I removed some whitespace lines.
If you think I should remove more lines, please edit the patch as a comment, and indicate which lines I should remove.


>- for myHttpResponseDataLen, please add a macro with comments rather than just
> initialize it to 64*1024, so this limit is exposed and can be changed easily

Done.


I made one more last minute change to the API.
We defined that ownership of out parameter strings in TrySendAndReceive shall remain with the http client.
Because of that I made the parameters "const".


Please advise what else needs be done to land this patch on NSS trunk and 3.11 branch. Thanks.

Comment 17

[Kai Engert (:KaiE:)]

Comment on attachment 209678 [details] [diff] [review]
Patch v4

Updating Patch description to say "v4".

I just saw I have a C++ style comment in the patch, which says
  // usually "http"

This needs to be fixed when it gets checked in.

Comment 18

[Julien Pierre]

Comment on attachment 209678 [details] [diff] [review]
Patch v4

Thanks for these changes.
Re: the whitespaces, the two consecutive lines appear in the diff when you click "diff" to look at the patch. This is a bug in attachment.cgi from bugzilla. So you can ignore this comment.

Comment 19

[Kai Engert (:KaiE:)]

Attachment: Patch v4 b

Julien, thanks a lot for your review.

This new version of the patch addresses a request from Richard, inside fetchOcspHttpClientV1 we changed two items to be const.

Not a change to the API.
Carrying forward the review.

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 210432 [details] [diff] [review]
Patch v4 b

Bob, as we would like to land on the 3.11 branch, could you please review, too?

Comment 21

[Robert Relyea]

Comment on attachment 210432 [details] [diff] [review]
Patch v4 b

r+ if you make the following change:

SEC_RegisterDefaultHttpClient
should set an error code on failure (PORT_SetError).
The GetClient function should also register and error code (though it's a static function, and the current code ignores the error).

bob

Comment 22

[Kai Engert (:KaiE:)]

> should set an error code on failure (PORT_SetError).

As this failure will only happen, if initialization has not yet happened, or if we were out of memory at init time, I propose to use PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);

Comment 23

[Kai Engert (:KaiE:)]

Attachment: Patch v4 c

This patch sets PORT_SetError in both suggested locations.
Carrying forward reviews.
I'm about to check this in to both NSS trunk and NSS 3.11 branch.

Comment 24

[Kai Engert (:KaiE:)]

Checked in to NSS trunk:

/cvsroot/mozilla/security/nss/lib/certhigh/manifest.mn,v  <--  manifest.mn
new revision: 1.5; previous revision: 1.4
done
Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.22; previous revision: 1.21
done
Checking in certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.7; previous revision: 1.6
done
Checking in certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.5; previous revision: 1.4
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.160; previous revision: 1.159
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.70; previous revision: 1.69
done
Checking in ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.2; previous revision: 1.1
done



Checked in to NSS 3.11 branch:
Checking in certhigh/manifest.mn;
/cvsroot/mozilla/security/nss/lib/certhigh/manifest.mn,v  <--  manifest.mn
new revision: 1.4.28.1; previous revision: 1.4
done
Checking in certhigh/ocsp.c;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.21.2.2; previous revision: 1.21.2.1
done
Checking in certhigh/ocsp.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.6.28.1; previous revision: 1.6
done
Checking in certhigh/ocspi.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspi.h,v  <--  ocspi.h
new revision: 1.2.2.2; previous revision: 1.2.2.1
done
Checking in certhigh/ocspt.h;
/cvsroot/mozilla/security/nss/lib/certhigh/ocspt.h,v  <--  ocspt.h
new revision: 1.4.28.1; previous revision: 1.4
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.1; previous revision: 1.158
done
Checking in nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.69.2.1; previous revision: 1.69
done

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Maybe we should have a "SEC_ERROR_NOT_INITIALIZED" ?

Comment 26

[Kai Engert (:KaiE:)]

http://developer.mozilla.org/en/docs/HTTP_Delegation