Closed Bug 116334 Opened 23 years ago Closed 23 years ago

NSS3.4 / Tracking bug for landing NSS 3.4 in Mozilla

Categories

(Core Graveyard :: Security: UI, defect, P1)

Other Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED
psm2.2

People

(Reporter: KaiE, Assigned: ssaux)

References

Details

Attachments

(2 files, 7 obsolete files)

This bugs tracks the list of problems that need to be resolved before NSS 3.4 can be used on the Mozilla trunk.
Making this bug dependent on the following bugs: 115861, 115683, 115684, 115692, 115697, 115699, 115839, 115927, 116330, 116332
Attached patch Patch to PSM with required changes (obsolete) (deleted) — Splinter Review
This is an intermediate patch that contains code from Bob's branch PSM_FOR_NSS_3_4, as well some additional required code, to make Mozilla compile with NSS 3.4.
Depends on: 115326
Dependency for 115861 was a mistake, removing...
No longer depends on: 115861
Depends on: 116401
Priority: -- → P1
Target Milestone: --- → 2.2
Bob, There are still some references to CERT_AddTempCertToPerm in PSM. See http://lxr.mozilla.org/mozilla/ident?i=CERT_AddTempCertToPerm I ran across this when trying to download a new server cert. In this code, http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSIOLayer.cpp#891 PSM tries to save it, but since CERT_AddTempCertToPerm currently does nothing in NSS 3.4, it just fails silently. The question is, should we implement the fix in NSS or PSM? I vote for implementing CERT_AddTempCertToPerm in 3.4, since that way we know everbody will do it the same way.
Ian, thanks for this hint. At the very moment I was trying to trace bug 116417. As nsNSSCertificateDB::handleCACertDownload is also calling CERT_AddTempCertToPerm, this seams to be the reason why it does not work.
Depends on: 116417
Yes, we should. It's not listed with an _ in it so servers probably use it as well. bob
Bob, Ian, Wan-Teh: The attached patch contains all of Bob's work on PSM. While we work on this bug, and if you should realize that additional changes to PSM are required, just let us know in this bug, and we will create updated patches. Thanks!
Attached patch additional PSM patch to ssl/src (obsolete) (deleted) — Splinter Review
This is another patch that is needed for NSS 3.4/PSM. The first part fixes deletion of certificates. Now all certs, including internal, have a valid slot pointer, so it is not sufficient to check cert->slot to determine if the cert was a builtin. The second part is actually an enhancement, it narrows the search for PK11_ListCerts, which reduces a lot of overhead.
Adding Javi to cc list
Depends on: 118593
Kai wrote: > Bob, Ian, Wan-Teh: The attached patch contains all of Bob's > work on PSM. While we work on this bug, and if you should > realize that additional changes to PSM are required, just > let us know in this bug, and we will create updated patches. I'd like to propose another solution to maintain the PSM changes for NSS 3.4: use ifdef's in the PSM files. In .c or .cpp files, you would say #include "nss.h" /* for NSS version macros */ and then ifdef NSS 3.4 specific changes as follows: #if NSS_VMAJOR == 3L && NSS_VMINOR == 4L /* NSS 3.4 specific changes here */ #else /* We are using NSS 3.3.x. */ #endif For makefiles, we would test an environment variable, say NSS_3_4. On Unix, we would say ifdef NSS_3_4 # Using NSS 3.4 else # Using NSS 3.3.x endif The advantage of this solution is that we can land the PSM changes for NSS 3.4 by simply adding NSS_3_4 = 1 to the PSM makefiles that have NSS 3.4 specific changes. We'll remove the ifdefs when the tree stabilizes after our landing. If you don't like the ifdef solution, we propose that you check in the PSM patch into the NSS source tree as mozilla/security/nss/psm-patch. Please let us know what you think of these two proposals.
I like the #ifdef idea. I will create a patch.
Attached patch Updated consolidated patch (obsolete) (deleted) — Splinter Review
This patch: - combines the previous patches, and adds the changes that wtc suggested. - can be applied to the current Mozilla trunk without causing any changes in functionality.
Attachment #62481 - Attachment is obsolete: true
Attachment #63823 - Attachment is obsolete: true
In order to fetch and compile Mozilla with the tip of NSS, you can apply this patch. In your build enviroment, define NSS_3_4=1 to cause the correct makefiles to be used.
Comment on attachment 63901 [details] [diff] [review] Updated consolidated patch 1. mozilla/security/manager/Makefile.in Your changes to this file can be simplfied as follows: - Leave the original definition of LOADABLE_ROOT_MODULE as is. - Add an ifdef NSS_3_4 section in which you define NSS3_LIB, SMIME3_LIB, SSL3_LIB, and SOFTOKEN3_LIB, for example: NSS3_LIB = $(LIB_PREFIX)nss3$(DLL_SUFFIX) Note the use of LIB_PREFIX, which avoids the special case for OS2 and WINNT. 2. mozilla/security/manager/ssl/src/Makefile.in >@@ -94,9 +94,34 @@ > jar \ > unicharutil \ > pipboot \ >+ timer \ > $(NULL) This change has nothing to do with NSS 3.4, right? Define NSS3_LIB, SSL3_LIB, etc. the same way in this file to avoid the if case for OS2 and WINNT. The use of $(DIST)/lib/libcrmf.$(LIB_SUFFIX) in EXTRA_LIBS might break OS2. OS2 may not have the "lib" prefix before "crmf". 3. In several .cpp files, you have >+#include "nss.h" >+#if NSS_VMAJOR == 3L && NSS_VMINOR == 4L >+#ifndef NSS_3_4 >+#define NSS_3_4 >+#endif >+#endif I propose that we delete these and simply add this to the Makefile.in's (and similar stuff for makefile.win's): ifdef NSS_3_4 DEFINES += -DNSS_3_4 endif
Attachment #63901 - Flags: needs-work+
Comment on attachment 63902 [details] [diff] [review] (do not review) Patch for Mozilla to fetch the tip of NSS Perhaps we should also have mozilla/client.mk and mozilla/client.mak check the NSS_3_4 environment variable and pull either NSS_CLIENT_TAG or the tip of NSS?
Attached patch New patch with wtc's suggestions applied (obsolete) (deleted) — Splinter Review
> 1. mozilla/security/manager/Makefile.in > > Your changes to this file can be simplfied as follows: done > 2. mozilla/security/manager/ssl/src/Makefile.in > > >+ timer \ > > This change has nothing to do with NSS 3.4, right? I took this from the special branch, but indeed, it is not needed. Removed. > Define NSS3_LIB, SSL3_LIB, etc. the same way in this file > to avoid the if case for OS2 and WINNT. done > The use of $(DIST)/lib/libcrmf.$(LIB_SUFFIX) in EXTRA_LIBS > might break OS2. OS2 may not have the "lib" prefix before > "crmf". now using $(LIB_PREFIX) > 3. In several .cpp files, you have > > ... > > I propose that we delete these and simply add this to > the Makefile.in's (and similar stuff for makefile.win's): > > ifdef NSS_3_4 > DEFINES += -DNSS_3_4 done, both for Unix and Window (not yet tested for Windows, but will do) > Perhaps we should also have mozilla/client.mk and mozilla/client.mak > check the NSS_3_4 environment variable and pull either NSS_CLIENT_TAG > or the tip of NSS? Good suggestion. Done.
Attachment #63901 - Attachment is obsolete: true
Attachment #63902 - Attachment is obsolete: true
Comment on attachment 63988 [details] [diff] [review] New patch with wtc's suggestions applied r=wtc. This patch is good, although it also reveals several problems that we have yet to address. 1. We also need to figure out how to cause the NSS_3_4 macro to be defined on the Mac. 2. I noticed that several NSS header files are included in an extern "C" block. We should fix them so that they can be included safely by a C++ file. 3. PSM is using __CERT_NewTempCertificate and __CERT_AddTempCertToPerm. Bob, should we simply make these officially exported functions? 4. mozilla/security/manager/ssl/src/nsNSSCertificate.cpp has a comment that says "fix this... rjr". Bob, is that you? Should we fix that? 5. We need to make sure that Mozilla's "static build" works with NSS 3.4.
Attachment #63988 - Flags: review+
For the Mac, any defines you want to add to an NSS project, you just add #define VARIABLE value to NSSCommon.h in mozilla/security/nss/macbuild/NSSComon.h I've got that part working in my tree already.
Depends on: 118833
Comment on attachment 63988 [details] [diff] [review] New patch with wtc's suggestions applied wtc: I guess the additional things you mention do not stop us from landing this patch on the trunk, as it only changes things for those explicitly defining NSS_3_4 in their environment. I'll request superreview and check it in asap.
Kai: yes. The additional issues I mentioned does not need to be addressed by your patch.
Depends on: 118821
I like Kai's latest patches. Wan-Teh, are there similar defines in coreconf? we have a fair about of ifdef windows/ else unix in NSS to handle the different library names on link lines. Eventually it would be nice to consolidate the NSS library dependencies in a single makefile that gets included everywhere for PSM, but I don't see that as a necessity at this stage since the number of makefiles affected are small, and totally contained in PSM. re: __NewTempCert and __AddTempToPerm.... I'd still rather keep them private. They have some pretty specific usages, but since we have implemented them in 3.4, and there isn't an easy way around their use there, we should keep them in PSM. (I was the one who initially added the defines to get PSM to build with shared libraries). re: fix this rjr.. If S/Mime is working correctly, we should be OK. There's a lot of talk out there about storing the email address and cert with the address book. If this is do, we don't have to worry about this function. re: Mac define: Javi, the goal we have here is to check in the 3.4 changes without switching to 3.4 quite yet. That means that we don't want to define NSS_3_4 for all mac projects, only those we've set up to do 3.4 builds. I'm not sure how to do that in the currrent mac environment (maybe we can't and we'll just have to let it stew until we check in the whole tree (sigh)).
No longer depends on: 118821, 118833
Depends on: 118813
Re-adding dependencies, somehow they got lost when Bob added his comment. Might be a bugzilla bug?
Depends on: 118821, 118833
>re: fix this rjr.. >If S/Mime is working correctly, we should be OK. There's a lot of talk out there >about storing the email address and cert with the address book. If this is do, >we don't have to worry about this function. At least we have not yet done it. Stephane, what do you think? We are talking about the function that retrieves the available certs for an email address from the NSS database. There is no such function yet in NSS 3.4 (that's my understanding).
Depends on: 118818
I think we need to have the capability to find a s/mime cert in the cert db. I don't think we'll have the time to implement anything different. I don't mind implementing this directly in PSM using NSS primitives (something like Find cert by usage). It may help us to interleave ldap searches and db manipulation code (which may make sense) and also implement a smime cache automatic cleanup functionality (remove old certs).
One question regarding finding smime certs: does NSS still support attaching an email address to an arbitrary cert when storing it in the db?
Depends on: 118864
The issue here is that we still have a database limitation that says: One email address maps to exactly one subject List. This means that 1) you can't have multiple email addresses pointing to the same certificate, and 2) you can have multiple certificates (with different subjects) pointing to the same email address. NSS has never allowed arbitrarily setting the email address for a cert (though we can put the support in to NSS 3.4 as long as you obey the above restrictions. bob
Right, I understand the limitation. Using the old method is not perfect, but it's not incompatible with adding new methods. The client will look for recipient certs in ldap, and the db. In the future it could look for recipient certs, in ldap, an address book, the db. However, we are still working on nailing the s/mime design for the next release.
Comment on attachment 63988 [details] [diff] [review] New patch with wtc's suggestions applied sr=blizzard Looks fine to me. The __FUNCTION names for the macros are pretty ugly but they won't be the end of me.
Attachment #63988 - Flags: superreview+
Comment on attachment 63988 [details] [diff] [review] New patch with wtc's suggestions applied Another option is to use all-caps macro names, for example: +#ifdef NSS_3_4 +#define PK11_PQG_PARAM_GEN PK11_PQG_ParamGen +#define PK11_PQG_DESTROY_VERIFY PK11_PQG_DestroyVerify +#define PK11_PQG_DESTROY_PARAMS PK11_PQG_DestroyParams +#else +#define PK11_PQG_PARAM_GEN PQG_ParamGen +#define PK11_PQG_DESTROY_VERIFY PQG_DestroyVerify +#define PK11_PQG_DESTROY_PARSMS PQG_DestroyParams +#endif This is the coding style we use in NSPR. But since these macros will eventually be removed, I wouldn't worry too much about their ugliness.
Depends on: 119074
Comment on attachment 63988 [details] [diff] [review] New patch with wtc's suggestions applied Patch checked in. I'm marking it as obsolete, as we keep the bug open.
Attachment #63988 - Attachment is obsolete: true
Depends on: 119086
Depends on: 116400
Depends on: 119359
Adding bug# 120173 [CRL Import Failure] to the list
Depends on: 120173
Depends on: 120208
Depends on: 120596
Depends on: 120593
Depends on: 120627
Depends on: 120633
Depends on: 120645
Depends on: 120647
Depends on: 120856
No longer depends on: 120856
Depends on: 121315
Depends on: 121322
Depends on: 121326
Blocks: 120885
Depends on: 120856
Depends on: 120857
Depends on: 121384
Depends on: 121388
Depends on: 121663
Depends on: 121487
Depends on: 121698
Depends on: 121703
Depends on: 121708
No longer depends on: 119086
Depends on: 121866
No longer depends on: 120596
Depends on: 122633
Attached patch Patch required for Mac landing. (obsolete) (deleted) — Splinter Review
We still need a patch to set NSS_3_4 to 1 in some Unix and Windows makefiles.
Attached patch Patch for Unix/Windows (obsolete) (deleted) — Splinter Review
This patch should land during carpool. The plan is: Default Mozilla builds will continue to check out NSS_CLIENT_TAG, therefore the special checkout rules need to be removed, and NSS_3_4=1 be always defined when building PSM.
Comment on attachment 67187 [details] [diff] [review] Patch for Unix/Windows r=wtc.
Attachment #67187 - Flags: review+
Comment on attachment 67185 [details] [diff] [review] Patch required for Mac landing. r=wtc.
Attachment #67185 - Flags: review+
No longer depends on: 121487
No longer depends on: 121326
No longer depends on: 115326
No longer depends on: 118593
*** Bug 118833 has been marked as a duplicate of this bug. ***
Attached patch Updated makefile changes (deleted) — Splinter Review
This updated patch has the patch from bug 118833 merged in.
Attachment #67187 - Attachment is obsolete: true
Oh well, this patch has also the changes from bug 120856 in it.
Comment on attachment 67193 [details] [diff] [review] Updated makefile changes r=wtc. cls, could you review this patch? Thanks.
Attachment #67193 - Flags: review+
cls, could you please review this patch (attachment 67193 [details] [diff] [review])? Thanks.
Comment on attachment 67193 [details] [diff] [review] Updated makefile changes Rather than setting NSS_3_4 in the individual Makefile.ins & makefile.wins can we just get rid of the non-3.4 pieces completely? We're not going to support going back to 3.3, right?
Attachment #67193 - Flags: needs-work+
Comment on attachment 67193 [details] [diff] [review] Updated makefile changes Chris, That's a good question. We do plan to remove the ifdef NSS_3_4 later. We are leaving them in for now to reduce the size of these patches so that they are easier to review.
Depends on: 121
No longer depends on: 121866, 122633
Comments on attachment 67185 [details] [diff] [review]: + my($def_mod_time) = (-e $source_def_file ? GetFileModDate($source_def_file) : 0); Isn't it an error if $source_def_file does not exist? + CreateNSSExportList(":mozilla:security:nss:lib:softoken:softokn.def", + ":mozilla:security:nss:macbuild:_softoken.mcp.exp"); I have a minor preference for giving the .exp files better names. "_softoken.mcp.exp" is the default name used by CodeWarrior for the project "softoken.mcp", but you can use any name for the .exp file, and add it to the project, to have it take effect. I think "softoken.exp" is better, because it does not imply that the file is generated by the project. If this file is missing, the project will then correctly fail to link, rather than just dumping out a new "_softoken.mcp.exp" which exports all globals. I also wonder why you need these .exp files, rather than controlling the exports via #pragmas (__declspec export) in the code? That's an XP issue though. - <SETTING><NAME>MWFTP_Post_password</NAME><VALUE>0</VALUE></ SETTING> + <SETTING><NAME>MWFTP_Post_password</NAME><VALUE> 2633yp7n633gn320&#211;&#191;H</VALUE></SETTING> CodeWarrior bug causes this. You should remove that change. Other changes are good.
Simon Fraser wrote: > I also wonder why you need these .exp files, rather than > controlling the exports via #pragmas (__declspec export) > in the code? When we added shared library support to NSS in NSS 3.2, it was less work to add these .exp files than going through the source files to add the appropriate compiler directives. Moreover, on some platforms, the .exp files allow us to define interface versions, which cannot be done with compiler directives in the code. So we invented a .def file format as the master export symbol list, and use perl or awk to convert the .def files to the platform-specific export files.
Comment on attachment 67193 [details] [diff] [review] Updated makefile changes In that case, r=cls .
Attachment #67193 - Flags: needs-work+
*** Bug 120856 has been marked as a duplicate of this bug. ***
Depends on: 122633
Attached patch Updated Mac patch (deleted) — Splinter Review
This updated patch incorporates sfraser's suggestions. The changes to make the NSS projects depend on the new .exp files have been checked into the NSS tree (not yet moved onto NSS_CLIENT_TAG.)
Attachment #67185 - Attachment is obsolete: true
Comment on attachment 67286 [details] [diff] [review] Updated Mac patch sfraser's review from earlier still applies to this patch (it now incorporates his suggestions)
Attachment #67286 - Flags: review+
Depends on: 122961
No longer depends on: 122961
Comment on attachment 67193 [details] [diff] [review] Updated makefile changes sr=kin@netscape.com Are you guys going to file a bug regarding the removal of the !NSS_3_4 code so we don't forget?
Attachment #67193 - Flags: superreview+
I've filed bug 123066 to address Kin's comment #50
In comment #44 Simon reviewed the Mac project files (attachment 67185 [details] [diff] [review] and ok'ed them pending minor changes. Javier did implement these changes and also added the patch from bug 122633 to the latest mac attachment 67286 [details] [diff] [review]. Simon sr='ed bug 122633 patch, which is included in attachment 67286 [details] [diff] [review]. I transfer the sr from 122633, and the sr from the previous mac attachment to attachment 67286 [details] [diff] [review]. I remove the dependency on 122633. We're go for Monday.
No longer depends on: 122633
Comment on attachment 67286 [details] [diff] [review] Updated Mac patch sr as per my previous comment. All the patches to this bug will land on Monday's carpool unless somebody objects.
Attachment #67286 - Flags: superreview+
Mozilla now uses NSS 3.4, marking fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Now that this Bug (116334) has landed is the branch NSS3.4:memoryfootprintreduction http://komodo.mozilla.org/planning/branches.cgi going to be retired?
Could this be the cause of bug 123918? Embedding is having a problem starting up and it could be because some of the psm shared libs are missing from the embed config files.
Verified.
Status: RESOLVED → VERIFIED
Product: PSM → Core
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: