481763 - Implement CERT_PKIXSetDefaults

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P5)

Description

[Kai Engert [:KaiE:]]

Header cert.h already declares CERT_PKIXSetDefaults

We need an implementation.

Comment 1

[Kai Engert [:KaiE:]]

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);

Comment 2

[Kai Engert [:KaiE:]]

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)

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v1

I believe Bob wrote much of the nearby code.

Comment 4

[Robert Relyea]

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

Comment 5

[Robert Relyea]

On other comment, if this makes friday, the nss.def patch should put the symbol in NSS 3.12.3.

bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 365801 [details] [diff] [review]
Patch v1

Minor style nit: 
the new for loops don't follow the NSS coding style.

Comment 7

[Kai Engert [:KaiE:]]

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.

Comment 8

[Robert Relyea]

ok, thanks kai. I only noted the r+ and reread comment #5.

bob

Comment 9

[Kai Engert [:KaiE:]]

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*

Comment 10

[Kai Engert [:KaiE:]]

Bob, ping? The answer to my question in comment 9 is quite important for proceeding.

Comment 11

[Robert Relyea]

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

[Wan-Teh Chang]

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.

Comment 13

[Kai Engert [:KaiE:]]

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.

Comment 14

[Kai Engert [:KaiE:]]

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)

Comment 15

[Kai Engert [:KaiE:]]

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 ?

Comment 16

[Kai Engert [:KaiE:]]

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.

Comment 17

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Comment 18

[Wan-Teh Chang]

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.

Comment 19

[Robert Relyea]

> 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

Comment 20

[Kai Engert [:KaiE:]]

(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"?

Comment 21

[Kai Engert [:KaiE:]]

(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...

Comment 22

[Kai Engert [:KaiE:]]

(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.

Comment 23

[Kai Engert [:KaiE:]]

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

Comment 24

[Benjamin Beurdouche [:beurdouche]]

Kai, could you please help me triage this ? : ) Thanks !

Comment 25

[Kai Engert [:KaiE:]]

I'd say nobody needs this anymore.
It was originally intended to make the code more complete, but apparently there wasn't much demand, and it was complicated to do it correctly.
It might be fine to wontfix.