Closed
Bug 402777
Opened 17 years ago
Closed 17 years ago
lib/util can't be built stand-alone.
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.12
People
(Reporter: wtc, Assigned: wtc)
References
()
Details
Attachments
(4 files, 2 obsolete files)
11.07 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
35.16 KB,
patch
|
christophe.ravel.bugs
:
review+
nelson
:
superreview-
|
Details | Diff | Splinter Review |
2.34 KB,
patch
|
Details | Diff | Splinter Review | |
1.03 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
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).
Attachment #287599 -
Flags: review?(rrelyea)
Assignee | ||
Comment 1•17 years ago
|
||
lib/nss/utilwrap.c also needs to be changed.
Attachment #287599 -
Attachment is obsolete: true
Attachment #287601 -
Flags: review?(rrelyea)
Attachment #287599 -
Flags: review?(rrelyea)
Comment 2•17 years ago
|
||
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
Assignee | ||
Comment 3•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 5•17 years ago
|
||
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•17 years ago
|
||
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
Attachment #287601 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 7•17 years ago
|
||
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?
Assignee | ||
Comment 8•17 years ago
|
||
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?
Attachment #287753 -
Flags: superreview?(nelson)
Attachment #287753 -
Flags: review?(rrelyea)
Comment 9•17 years ago
|
||
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•17 years ago
|
||
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
Attachment #287753 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 11•17 years ago
|
||
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.
Attachment #287753 -
Attachment is obsolete: true
Attachment #288023 -
Flags: superreview?(nelson)
Attachment #288023 -
Flags: review?(christophe.ravel.bugs)
Attachment #287753 -
Flags: superreview?(nelson)
Comment 12•17 years ago
|
||
Comment on attachment 288023 [details] [diff] [review] Move pqgutil.{h.c} to lib/freebl and lib/pk11wrap (as checked in) r=christophe
Attachment #288023 -
Flags: review?(christophe.ravel.bugs) → review+
Assignee | ||
Comment 13•17 years ago
|
||
(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)?
Assignee | ||
Comment 14•17 years ago
|
||
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•17 years ago
|
||
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
Attachment #288023 -
Flags: superreview?(nelson) → superreview-
Assignee | ||
Comment 16•17 years ago
|
||
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.
Attachment #288155 -
Flags: review?(nelson)
Comment 17•17 years ago
|
||
Comment on attachment 288155 [details] [diff] [review] Fix FREEBL_VERSION Good. Thanks.
Attachment #288155 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 18•17 years ago
|
||
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.
Assignee | ||
Comment 19•17 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in
before you can comment on or make changes to this bug.
Description
•