Last Comment Bug 402777 - lib/util can't be built stand-alone.
: lib/util can't be built stand-alone.
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: -- trivial (vote)
: 3.12
Assigned To: Wan-Teh Chang
:
:
Mentors:
http://wiki.mozilla.org/NSS_Refactor_...
Depends on:
Blocks: 360426
  Show dependency treegraph
 
Reported: 2007-11-06 14:57 PST by Wan-Teh Chang
Modified: 2008-03-27 16:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Move CKM_INVALID_MECHANISM and CERTValidity functions (9.47 KB, patch)
2007-11-06 14:57 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Move CKM_INVALID_MECHANISM and CERTValidity functions, v1.1 (11.07 KB, patch)
2007-11-06 15:18 PST, Wan-Teh Chang
rrelyea: review+
Details | Diff | Splinter Review
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (36.92 KB, patch)
2007-11-07 14:42 PST, Wan-Teh Chang
rrelyea: review+
Details | Diff | Splinter Review
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (as checked in) (35.16 KB, patch)
2007-11-09 10:56 PST, Wan-Teh Chang
christophe.ravel.bugs: review+
nelson: superreview-
Details | Diff | Splinter Review
Move all fake PKCS #11 defines from secmodt.h to pkcs11n.h (2.34 KB, patch)
2007-11-09 15:46 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review
Fix FREEBL_VERSION (1.03 KB, patch)
2007-11-10 10:48 PST, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review

Description Wan-Teh Chang 2007-11-06 14:57:48 PST
Created attachment 287599 [details] [diff] [review]
Move CKM_INVALID_MECHANISM and CERTValidity functions

lib/util needs some headers from other NSS directories, therefore it can't
be built stand-alone.

1. pqgutil.h includes blapit.h from lib/freebl, which in turn includes ecl-exp.h
from lib/freebl/ecl.

One may argue that pqgutil.{h,c} is crypto-related and therefore should be
moved out of lib/util so that we can exclude lib/util from FIPS validation.

2. secoid.c includes pkcs11t.h and secmodt.h.  The only thing it needs from
secmodt.h is the CKM_INVALID_MECHANISM macro.  I suggest that we move the
CKM_INVALID_MECHANISM macro definition from secmodt.h to pkcs11n.h.
secmodt.h includes pkcs11t.h, which includes pkcs11n.h, so this change
is backward compatible.

FYI: pkcs11t.h includes pkcs11p.h, pkcs11n.h, and pkcs11u.h.

3. sectime.c includes cert.h because it needs the definition of the
CERTValidity time.  I suggest that we move the three CERTValidity-related
functions to lib/certdb/certdb.c.  These three functions are only used
by libnss3.so and cmd, so moving them to lib/certdb/certdb.c is safe.

The attached patch moves the CKM_INVALID_MECHANISM macro definition and
the three CERTValidity functions.  More work is needed to fix this bug
completely (to deal with the dependency on blapit.h and pkcs11t.h).
Comment 1 Wan-Teh Chang 2007-11-06 15:18:37 PST
Created attachment 287601 [details] [diff] [review]
Move CKM_INVALID_MECHANISM and CERTValidity functions, v1.1

lib/nss/utilwrap.c also needs to be changed.
Comment 2 Julien Pierre 2007-11-06 15:29:11 PST
Wan-Teh,

Why is it a requirement for you to build util without header dependencies ?

re: comment 1,

1. This was brought up during the development of bug 286642 (check for the word blapit in there), and nobody objected. I told Nelson in person about it, and he reviewed the PQG code and said it was OK. All the util PQG code does from util is decoding and is not crypto, despite the name.
What do you propose to do about this ?

2. no objections

3. that change would be fine with me too
Comment 3 Wan-Teh Chang 2007-11-06 15:52:31 PST
This is a prerequisite for building the softoken as a stand-alone component.
The value of that is clear: softoken is subject to FIPS validation, so it'd be
nice if we could use a FIPS-validated version of the softoken with many
versions of NSS.

We can still build lib/util and the softoken as stand-alone components
with header dependencies; it's just messier.  My goal is to see how far
we can go in the right direction without making our lives difficult.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2007-11-06 18:20:21 PST
The method for building the "libs" target in any of the nss/lib/* directories 
requires first building the "export" target in  ALL the nss/lib/* directories.
I don't see that softoken or lib/util are exceptions to this rule. 

If that's really a big deal, then we could add -I options to the INCLUDES
make variables in the makefiles for softoken and lib/util, to pick up the 
include files from the other directories (e.g. freebl).  

I have no objections to Wan-Teh's proposals 2 & 3.
Comment 5 Wan-Teh Chang 2007-11-06 20:40:31 PST
http://wiki.mozilla.org/NSS_Refactor_3_12 is the NSS refactoring plan
that I alluded to.

This bug is not a big deal.  I just wanted to present to the team what's
between us and that goal for lib/util.
Comment 6 Robert Relyea 2007-11-07 10:36:49 PST
Comment on attachment 287601 [details] [diff] [review]
Move CKM_INVALID_MECHANISM and CERTValidity functions, v1.1

r+ = relyea

The other FAKE PKCS #11 defines should probably move as well.

I agree with the sediment. It is likely sometime in the near future we will need to break some of these out as their own components.

Making util only depend on things in freebl & softoken is a good step.

I also would not be against moving the pkcs11*.h files into util as well, since util is currently the 'bottom' of the stack.

bob
Comment 7 Wan-Teh Chang 2007-11-07 13:43:37 PST
Comment on attachment 287601 [details] [diff] [review]
Move CKM_INVALID_MECHANISM and CERTValidity functions, v1.1

I checked in this patch on the NSS trunk for NSS 3.12.

Bob, the other three fake PKCS #11 defines are only used in lib/pk11wrap.
They're not used in lib/util or lib/softoken.  Do you still want me to move
them to pkcs11n.h?
Comment 8 Wan-Teh Chang 2007-11-07 14:42:52 PST
Created attachment 287753 [details] [diff] [review]
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap

This patch is one way to take care of the pqgutil.{h,c} issue,
which includes headers blapit.h and ecl-exp.h from lib/freebl.

It turns out that the functions in pqgutil.{h,c} are only used
in lib/pk11wrap/pk11pqg.c, except for two functions PQG_DestroyParams
and PQG_DestroyVerify.  So I "inlined" all the PQG_xxx functions
in lib/pk11wrap/pk11pqg.c, and copied PQG_DestroyParams and
PQG_DestroyVerify to lib/freebl/pqg.c.  Some additional changes
are needed for lib/freebl to set up the vector of libfreebl
functions.

So only PQG_DestroyParams and PQG_DestroyVerify have two copies.
(The copies in lib/pk11wrap/pk11pqg.c are named
PK11_PQG_DestroyParams and PK11_PQG_DestroyVerify.)

Finally, lib/util/pqgutil.{h,c} are cvs removed.

Does this solution look good?

Nelson, should I bump FREEBL_VERSION from 0x0309 to
0x0310 or 0x030A?

Christophe, could you review the change to the Solaris pkg
file in this patch?
Comment 9 Robert Relyea 2007-11-08 09:28:27 PST
re comment 7

Yes, It's more about where these things should be defined rather than exactly who uses them (though the latter influences the former).

pkcs11n.h are essentially the NSS extensions to pkcs11. These fake params are allocated out of the vendor space, so pkcs11n.h is the reasonable place for them to be.
Comment 10 Robert Relyea 2007-11-08 18:47:56 PST
Comment on attachment 287753 [details] [diff] [review]
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap

r+ with some comments.

I would not be sad if you dropped the DUP functions altogether. They aren't exported and I don't think the PQG interface is one that is heavily used.

I wouldn't be against a clean up that defined PQGParams as SECKEYPQGParams used in crypto hi (except the former is opaque and private and the latter is public (sigh)).

bob
Comment 11 Wan-Teh Chang 2007-11-09 10:56:00 PST
Created attachment 288023 [details] [diff] [review]
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (as checked in)

Bob, I "inlined" PK11_PQG_DupParams and PK11_PQG_DupVerify as you suggested.
I checked in this patch on the NSS trunk for NSS 3.12.

Nelson, if I should bump FREEBL_VERSION from 0x0309 to 0x030A instead of
0x0310, please let me know.  I just found that we didn't bump FREEBL_VERSION
when we added the Camellia functions.  Since NSS 3.12 hasn't been released,
can I just put the Cammellia functions and the two new PQG_DestroyXXX
functions in the same version?

Christophe, please review my change to
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap.  I removed a line
that was commented out.  pqgutil.h should not be exported from the
NSS package, and in this patch I cvs removed it.
Comment 12 Christophe Ravel 2007-11-09 11:07:29 PST
Comment on attachment 288023 [details] [diff] [review]
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (as checked in)

r=christophe
Comment 13 Wan-Teh Chang 2007-11-09 13:40:59 PST
(In reply to comment #10)
>
> I wouldn't be against a clean up that defined PQGParams as SECKEYPQGParams used
> in crypto hi (except the former is opaque and private and the latter is public
> (sigh)).

Bob, I don't quite understand your suggestion here.  Do you mean this?

pk11pqg.h:
    #include "keythi.h"  /* instead of "blapit.h" */
    typedef SECKEYPQGParams PQGParams;

Or do you mean this?

pk11pqg.h:
    /* do not include "keythi.h" or "blapit.h" */
    typedef struct SECKEYPQGParamsStr PQGParams;  /* forward declaration of "opaque" struct*/

pk11pqg.c:
    #include "keythi.h"

Just to make sure I understand the purpose of "blapit.h": should "blapit.h"
only be included by lib/freebl and its direct users (lib/softoken, the bypassPKCS11 code in lib/ssl, and test programs of lib/freebl such as cmd/bltest, and cmd/fipstest)?
Comment 14 Wan-Teh Chang 2007-11-09 15:46:11 PST
Created attachment 288065 [details] [diff] [review]
Move all fake PKCS #11 defines from secmodt.h to pkcs11n.h

I also fixed compilation error of lib/freebl/loader.c, which had two
void functions returning a value.  I've checked in this patch.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2007-11-09 16:25:15 PST
Comment on attachment 288023 [details] [diff] [review]
Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (as checked in)

>-#define FREEBL_VERSION 0x0309
>+#define FREEBL_VERSION 0x0310

>     /* End of Version 3.009. */
>     Camellia_InitContext,
>     Camellia_AllocateContext,
>     Camellia_CreateContext,
>     Camellia_DestroyContext,
>     Camellia_Encrypt,
>     Camellia_Decrypt,
> 
>+    /* End of Version 3.010. */
>+    PQG_DestroyParams,
>+    PQG_DestroyVerify,
>+
> };

Numerous comments about the above:

1) What do you believe/intend to be the version number for the PQG functions,
and the version number for this file?
3.010?  3.011?  3.016?  3.017?

Is it your intent to skip to version 3.016?  or perhaps to 3.017?

0x0310 corresponds to 3.016, not 3.010.  

2) The comment above seems to state that 3.010 added only the Camellia 
functions, and that the PQG functions are added after 3.010.  

3) Please add a comment AFTER the PQG functions, stating the version number
that applies to them, and ensure that it corresponds to FREEBL_VERSION
Comment 16 Wan-Teh Chang 2007-11-10 10:48:30 PST
Created attachment 288155 [details] [diff] [review]
Fix FREEBL_VERSION

I want the FREEBL_VERSION to be 3.010.

We didn't bump FREEBL_VERSION when we added the Camellia
functions.  So this patch puts the Camellia and PQG functions
in the same group.
Comment 17 Nelson Bolyard (seldom reads bugmail) 2007-11-12 18:34:55 PST
Comment on attachment 288155 [details] [diff] [review]
Fix FREEBL_VERSION

Good.  Thanks.
Comment 18 Wan-Teh Chang 2007-11-12 19:14:01 PST
Comment on attachment 288155 [details] [diff] [review]
Fix FREEBL_VERSION

I checked in this patch "Fix FREEBL_VERSION" on the NSS trunk for NSS 3.12.
Comment 19 Wan-Teh Chang 2007-11-20 18:39:32 PST
It turns out that this bug is a duplicate of bug 362278.
I didn't remember at all that I filed that bug :-)

The only remaining issues for building lib/util standalone
are the pkcs11*.h headers (required by secoid.c) and nss.h
(required by nssutil.rc on Windows for the NSS version macros).
These issues do not affect the symbols we export from
libnssutil3 shared library, so we can address them when
we really need to build lib/util as a separate component.

Note You need to log in before you can comment on or make changes to this bug.