Open
Bug 481763
Opened 16 years ago
Updated 2 years ago
Implement CERT_PKIXSetDefaults
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
NEW
People
(Reporter: KaiE, Assigned: KaiE)
References
(Depends on 1 open bug)
Details
(Whiteboard: PKIX)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
wtc
:
superreview-
|
Details | Diff | Splinter Review |
Header cert.h already declares CERT_PKIXSetDefaults We need an implementation.
Assignee | ||
Comment 1•16 years ago
|
||
Function currently has interface and comments: /* * This function changes the application defaults for the Verify function. * It should be called once at app initialization time, and only changes * if the default configuration changes. * * This changes the default values for the parameters specified. These * defaults can be overridden in CERT_PKIXVerifyCert() by explicitly * setting the value in paramsIn. */ extern SECStatus CERT_PKIXSetDefaults(CERTValInParam *paramsIn);
Assignee | ||
Comment 2•16 years ago
|
||
How should NSS remember the defaults? It's not possible to make a deep copy of paramsIn, because the input params may contain void* pointers. The existing code in cert_pkixSetParam does lots of transformations to the input params, I don't see a simple way to remember the effects of parameter processing. It would require to read all existing parameter setting code. I don't know if procParams contains all transformation results. But even if it does, I don't know how to merge two procParams objects. Therefore I propose a simple approach: - the application must ensure the pointer given to CERT_PKIXSetDefaults remains valid until another call to CERT_PKIXSetDefaults sets a new object (or removes the defaults by passing NULL) - CERT_PKIXVerifyCert will iterate over the default set of params first, then iterate over the function call specific params. - we can be smart, when we iterate the default params, we can skip the param types that are found in the function specific params, too (thereby avoiding setting the same parameter type twice)
Assignee | ||
Comment 3•16 years ago
|
||
I believe Bob wrote much of the nearby code.
Attachment #365801 -
Flags: review?(rrelyea)
Comment 4•16 years ago
|
||
Comment on attachment 365801 [details] [diff] [review] Patch v1 r- for only one issue: Successive calls to CERT_PKIXSetDefaults should copy the paramsIn, and free any old defaultPkixVerifyParamsIn. Currently successive calls could result in memory leaks if the parameters which are passed in are not statics. (The interface should probably take a const CERTValInParm as well) This is the only reason I gave this an r-. Other comments. 1) as implemented, successive calls to PKIXSetdefaults will replace all defaults. As I think about it, that's the right semantic, but we also need a 'GetDefaults' so we can change one default without perturbing the others. 2) The algorithm for handling defaults is order n-squared. I believe n is small enough to get away with this. If the number of options start getting large, we may need to revisit this (the interface could support an order 2n processing, but that would require some code rejiggering). Good first cut. bob
Attachment #365801 -
Flags: review?(rrelyea) → review+
Comment 5•16 years ago
|
||
On other comment, if this makes friday, the nss.def patch should put the symbol in NSS 3.12.3. bob
Comment 6•16 years ago
|
||
Comment on attachment 365801 [details] [diff] [review] Patch v1 Minor style nit: the new for loops don't follow the NSS coding style.
Updated•16 years ago
|
Whiteboard: PKIX
Assignee | ||
Comment 7•15 years ago
|
||
Bob, in your comment you say reviewed denied (r-) and request changes. But you marked the flag with the opposite, r+ I conclude the r+ was set by mistake and this is not yet ready to check in.
Updated•15 years ago
|
Attachment #365801 -
Flags: review+ → review-
Assignee | ||
Comment 9•15 years ago
|
||
Bob, now I remember why I didn't immediately work on a new patch. I should have said this before: You requested that all parameters should get copied. I think that's not possible, because the variable structure allows void* ! We are not able to copy the contents of void*
Assignee | ||
Comment 10•14 years ago
|
||
Bob, ping? The answer to my question in comment 9 is quite important for proceeding.
Assignee | ||
Updated•14 years ago
|
Attachment #365801 -
Flags: review?(wtc)
Comment 11•14 years ago
|
||
I would say just copy the value of the void *. In this case the caller is responsible for freeing whatever void * pointer they sent us. If they call set default again. We should document which inparams this affects in the header file. The pointer types we know about we should copy directly. (this means the copy will have to switch on the inparam type value). bob
Comment 12•14 years ago
|
||
Comment on attachment 365801 [details] [diff] [review] Patch v1 Kai, Bob, I have two suggestions. 1. CERT_PKIXSetDefaults should not copy paramsIn. The caller should ensure that paramsIn stays valid. 2. The N-squared issue (or rather, MxN issue) is not serious because N is small. In Google Chrome, N is 5. You can build an array of size cert_pi_max (13) and store the default parameters in it. This will speed up the check for default (an array lookup). >+ for (i=0; >+ defaultPkixVerifyParamsIn[i].type != cert_pi_end; >+ ++i) { This can be written in one line. Same for the other two for loops i this patch.
Attachment #365801 -
Flags: review?(wtc) → review-
Assignee | ||
Comment 13•14 years ago
|
||
I was nearly convinced to write the deep copy code. But then I saw the need to make deep copies of CERTCertList. That seems easy on first sight, and I started to implement a new CERT_CopyCertList(CERTCertList *src) But then I noticed, the Nodes in the list can store application data in addition to a cert. Sorry, we're running into the same issue again. We are not able to copy the app data. Yes, we could implement a copy function that ignores the application data, in the hope that for this use case (pkix verify) we never need app data in those lists. But I don't like that. It's a hack.
Assignee | ||
Comment 14•14 years ago
|
||
In my opinion: - we should implement helper functions that simplify the construction of the deep CERTValParamInValue - we should implement a default function that destroys the data that NSS knows about - NSS has no knowledge about the data used by the application, and should never attempt to make deep copies. - CERT_PKIXSetDefaults should be extended to take 2 parameters, the params, and a pointer to a destruction function. The application function can either provide - NULL (app owns, do not destroy) - default function (app uses standard data) - app provided function (app uses nonstandard data, wants to cleanup on its own, app may decided to call the default function from within there)
Assignee | ||
Comment 15•14 years ago
|
||
The interface for function CERT_PKIXSetDefaults has already been defined in our headers, for several versions. However, the function has never been implemented. Am I allowed to change the interface of CERT_PKIXSetDefaults ?
Assignee | ||
Comment 16•14 years ago
|
||
Yet another argument, why deep copying was not designed originally. CERTValInParam * contains several CONST pointers. The parameter structure was defined to be strictly "for reference only", owned by someone else. I noticed just now. I have implemented some default cleanup code for this function, but the compiler warns me that I'm trying to destroy const pointers... Not sure what I should do now. If we define, if caller of CERT_PKIXSetDefaults specifies a destructor function, then he strictly transfers ownership to the function (and allowing to cast away const). The caller has the option to specify NULL as the destructor function. If he does so, he can call CERT_PKIXSetDefaults to obtain the previous pointer, and erase on his own.
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #365801 -
Attachment is obsolete: true
Attachment #521851 -
Flags: superreview?(wtc)
Attachment #521851 -
Flags: review?(rrelyea)
Comment 18•14 years ago
|
||
Comment on attachment 521851 [details] [diff] [review] Patch v2 Kai, Thank you very much for writing this patch. The alloc and destroy functions seem difficult to use. I now think the caller of CERT_PKIXSetDefaults should keep paramsIn alive until the next CERT_PKIXSetDefaults call. Another problem is that only CERT_PKIXVerifyCert uses these defaults. The old CERT_VerifyCert* functions in libpkix mode (if usePKIXValidationEngine is true) don't call CERT_PKIXVerifyCert. So this patch has no effect on them. But, I think we should remove CERT_PKIXSetDefaults. NSS can be used by other libraries. If those libraries' CERT_PKIXVerifyCert calls may change behavior because of the application's CERT_PKIXSetDefaults call, that can be confusing. Libraries that need predictable behavior will be forced to specify every input parameter for their CERT_PKIXVerifyCert calls. So I propose that we mark this bug WONTFIX, and simply remove the declaration of CERT_PKIXSetDefaults from cert.h.
Attachment #521851 -
Flags: superreview?(wtc) → superreview-
Comment 19•14 years ago
|
||
> Am I allowed to change the interface of CERT_PKIXSetDefaults ?
Until it's exported in nss.def it's fair game. Any macros you defined, however...
bob
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #19) > > Am I allowed to change the interface of CERT_PKIXSetDefaults ? > > Until it's exported in nss.def it's fair game. Any macros you defined, > however... Sorry, I'm not sure about the meaning of "it's fair game", do you say "yes" or "no"?
Assignee | ||
Comment 21•14 years ago
|
||
(In reply to comment #18) > Comment on attachment 521851 [details] [diff] [review] > Patch v2 > > Kai, > > Thank you very much for writing this patch. The alloc and > destroy functions seem difficult to use. I now think the caller > of CERT_PKIXSetDefaults should keep paramsIn alive until the > next CERT_PKIXSetDefaults call. Did you see that my patch allows that? The caller can simply use NULL as the "dtor" parameter, and the caller can simply use whatever allocation he wants, including static variables. > Another problem is that only CERT_PKIXVerifyCert uses these > defaults. The old CERT_VerifyCert* functions in libpkix mode > (if usePKIXValidationEngine is true) don't call > CERT_PKIXVerifyCert. So this patch has no effect on them. Maybe today, these defaults only affect the new API CERT_PKIXVerifyCert. But this means, that the routing from the old verification API to the PKIX implementation must use SOME defaults. Is there any way to influence those defaults? If not, maybe we'll need this API to allow the application to influence the behaviour, and make other portions of libPKIX use these defaults, too? > But, I think we should remove CERT_PKIXSetDefaults. NSS can be > used by other libraries. If those libraries' CERT_PKIXVerifyCert > calls may change behavior because of the application's > CERT_PKIXSetDefaults call, that can be confusing. I'm not sure this argument is convincing. We already have the same situation with, e.g.: - CERT_SetOCSPFailureMode - CERT_EnableOCSPChecking / CERT_DisableOCSPChecking > Libraries that > need predictable behavior will be forced to specify every input > parameter for their CERT_PKIXVerifyCert calls. In my understanding it's good practice that one library using another library shouldn't specify global defaults, that's the decision of an application. > So I propose that we mark this bug WONTFIX, and simply remove > the declaration of CERT_PKIXSetDefaults from cert.h. Well, I guess I shouldn't invest more time on working on patches until we make a final decision on this...
Assignee | ||
Comment 22•14 years ago
|
||
(In reply to comment #18) > > The alloc and > destroy functions seem difficult to use. Maybe you'll think differently after you look at the patch in bug 481764, that uses the allocation functions. The complexity is with the CERTValInParam and it's nested dynamically allocated members. I would have hoped the proposed allocation functions simplify dealing with the API.
Assignee | ||
Comment 23•14 years ago
|
||
Currently the thinking is, we don't need this. The app should just create it's own parameter objects, keep them alive, and reuse them for calls into the PKIX verification function. We also found, the defaults are not yet being by anything but the new PKIX API. So, having these defaults wouldn't be sufficient anyway. Removing blocking-status for 479393
No longer blocks: psm-pkix
Assignee | ||
Updated•14 years ago
|
Attachment #521851 -
Flags: review?(rrelyea)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•