Closed
Bug 152426
Opened 22 years ago
Closed 19 years ago
delegation of HTTP download for OCSP
Categories
(NSS :: Libraries, enhancement, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: julien.pierre, Assigned: KaiE)
References
()
Details
(Keywords: fixed1.8.1)
Attachments
(1 file, 5 obsolete files)
21.90 KB,
patch
|
KaiE
:
review+
KaiE
:
superreview+
wtc
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•22 years ago
|
Severity: normal → enhancement
Comment 1•22 years ago
|
||
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•22 years ago
|
||
*** Bug 163717 has been marked as a duplicate of this bug. ***
Comment 4•22 years ago
|
||
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.
Updated•20 years ago
|
Flags: blocking-aviary1.1?
Comment 5•20 years ago
|
||
Was any progress made on this issue since the meeting 11 months ago ?
Updated•20 years ago
|
QA Contact: bishakhabanerjee → jason.m.reid
Updated•19 years ago
|
Flags: blocking-aviary1.5? → blocking-aviary1.5-
Reporter | ||
Comment 6•19 years ago
|
||
We will deal with this problem in NSS 3.12 with libpkix .
Target Milestone: --- → 3.12
Assignee | ||
Comment 7•19 years ago
|
||
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) ?
Reporter | ||
Comment 8•19 years ago
|
||
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 .
Assignee | ||
Comment 9•19 years ago
|
||
> 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?
Reporter | ||
Comment 10•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Attachment #204060 -
Attachment is obsolete: true
Comment 11•19 years ago
|
||
*** Bug 323872 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 12•19 years ago
|
||
Attachment #209372 -
Flags: review?
Assignee | ||
Updated•19 years ago
|
Attachment #209372 -
Flags: review? → review?(julien.pierre.bugs)
Reporter | ||
Comment 13•19 years ago
|
||
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.
Attachment #209372 -
Flags: review?(julien.pierre.bugs) → review-
Assignee | ||
Comment 14•19 years ago
|
||
> 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.
Attachment #209372 -
Attachment is obsolete: true
Attachment #209574 -
Flags: review?(julien.pierre.bugs)
Reporter | ||
Comment 15•19 years ago
|
||
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.
Attachment #209574 -
Flags: review?(julien.pierre.bugs) → review+
Assignee | ||
Comment 16•19 years ago
|
||
>- 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.
Attachment #209574 -
Attachment is obsolete: true
Attachment #209678 -
Flags: review?(julien.pierre.bugs)
Assignee | ||
Comment 17•19 years ago
|
||
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.
Attachment #209678 -
Attachment description: Patch v3 → Patch v4
Updated•19 years ago
|
Assignee: wtchang → kengert
Priority: P2 → P1
Target Milestone: 3.12 → 3.11.1
Reporter | ||
Comment 18•19 years ago
|
||
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.
Attachment #209678 -
Flags: review?(julien.pierre.bugs) → review+
Assignee | ||
Comment 19•19 years ago
|
||
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.
Attachment #209678 -
Attachment is obsolete: true
Attachment #210432 -
Flags: review+
Assignee | ||
Comment 20•19 years ago
|
||
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?
Attachment #210432 -
Flags: superreview?(rrelyea)
Comment 21•19 years ago
|
||
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
Attachment #210432 -
Flags: superreview?(rrelyea) → superreview+
Assignee | ||
Comment 22•19 years ago
|
||
> 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);
Assignee | ||
Comment 23•19 years ago
|
||
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.
Attachment #210615 -
Flags: superreview+
Attachment #210615 -
Flags: review+
Assignee | ||
Updated•19 years ago
|
Attachment #210432 -
Attachment is obsolete: true
Assignee | ||
Comment 24•19 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 25•19 years ago
|
||
Maybe we should have a "SEC_ERROR_NOT_INITIALIZED" ?
Assignee | ||
Comment 26•19 years ago
|
||
Updated•19 years ago
|
Attachment #210615 -
Flags: approval-branch-1.8.1+
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•