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)
Tracking
(Not tracked)
VERIFIED
FIXED
psm2.2
People
(Reporter: KaiE, Assigned: ssaux)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
wtc
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
javi
:
review+
ssaux
:
superreview+
|
Details | Diff | Splinter Review |
This bugs tracks the list of problems that need to be resolved before NSS 3.4
can be used on the Mozilla trunk.
Reporter | ||
Comment 1•23 years ago
|
||
Making this bug dependent on the following bugs:
115861, 115683, 115684, 115692, 115697, 115699, 115839, 115927, 116330, 116332
Reporter | ||
Comment 2•23 years ago
|
||
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.
Reporter | ||
Comment 3•23 years ago
|
||
Dependency for 115861 was a mistake, removing...
No longer depends on: 115861
Assignee | ||
Updated•23 years ago
|
Priority: -- → P1
Target Milestone: --- → 2.2
Comment 4•23 years ago
|
||
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.
Reporter | ||
Comment 5•23 years ago
|
||
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
Comment 6•23 years ago
|
||
Yes, we should.
It's not listed with an _ in it so servers probably use it as well.
bob
Reporter | ||
Comment 7•23 years ago
|
||
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!
Comment 8•23 years ago
|
||
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.
Comment 9•23 years ago
|
||
Adding Javi to cc list
Comment 10•23 years ago
|
||
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.
Reporter | ||
Comment 11•23 years ago
|
||
I like the #ifdef idea. I will create a patch.
Reporter | ||
Comment 12•23 years ago
|
||
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
Reporter | ||
Comment 13•23 years ago
|
||
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 14•23 years ago
|
||
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 15•23 years ago
|
||
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?
Reporter | ||
Comment 16•23 years ago
|
||
> 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 17•23 years ago
|
||
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+
Comment 18•23 years ago
|
||
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.
Reporter | ||
Comment 19•23 years ago
|
||
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.
Comment 20•23 years ago
|
||
Kai: yes. The additional issues I mentioned does not need to be
addressed by your patch.
Comment 21•23 years ago
|
||
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)).
Reporter | ||
Comment 22•23 years ago
|
||
Re-adding dependencies, somehow they got lost when Bob added his comment. Might
be a bugzilla bug?
Reporter | ||
Comment 23•23 years ago
|
||
>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).
Assignee | ||
Comment 24•23 years ago
|
||
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).
Assignee | ||
Comment 25•23 years ago
|
||
One question regarding finding smime certs: does NSS still support attaching an
email address to an arbitrary cert when storing it in the db?
Comment 26•23 years ago
|
||
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
Assignee | ||
Comment 27•23 years ago
|
||
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 28•23 years ago
|
||
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 29•23 years ago
|
||
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.
Reporter | ||
Comment 30•23 years ago
|
||
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
Comment 32•23 years ago
|
||
Comment 33•23 years ago
|
||
We still need a patch to set NSS_3_4 to 1 in some Unix
and Windows makefiles.
Reporter | ||
Comment 34•23 years ago
|
||
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 35•23 years ago
|
||
Comment on attachment 67187 [details] [diff] [review]
Patch for Unix/Windows
r=wtc.
Attachment #67187 -
Flags: review+
Comment 36•23 years ago
|
||
Comment on attachment 67185 [details] [diff] [review]
Patch required for Mac landing.
r=wtc.
Attachment #67185 -
Flags: review+
Reporter | ||
Comment 37•23 years ago
|
||
*** Bug 118833 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 38•23 years ago
|
||
This updated patch has the patch from bug 118833 merged in.
Attachment #67187 -
Attachment is obsolete: true
Reporter | ||
Comment 39•23 years ago
|
||
Oh well, this patch has also the changes from bug 120856 in it.
Comment 40•23 years ago
|
||
Comment on attachment 67193 [details] [diff] [review]
Updated makefile changes
r=wtc.
cls, could you review this patch? Thanks.
Attachment #67193 -
Flags: review+
Comment 41•23 years ago
|
||
cls, could you please review this patch (attachment 67193 [details] [diff] [review])? Thanks.
Comment 42•23 years ago
|
||
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 43•23 years ago
|
||
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.
Updated•23 years ago
|
Comment 44•23 years ago
|
||
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Ó¿H</VALUE></SETTING>
CodeWarrior bug causes this. You should remove that change.
Other changes are good.
Comment 45•23 years ago
|
||
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 46•23 years ago
|
||
Comment on attachment 67193 [details] [diff] [review]
Updated makefile changes
In that case, r=cls .
Attachment #67193 -
Flags: needs-work+
Reporter | ||
Comment 47•23 years ago
|
||
*** Bug 120856 has been marked as a duplicate of this bug. ***
Comment 48•23 years ago
|
||
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 49•23 years ago
|
||
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+
Comment 50•23 years ago
|
||
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+
Assignee | ||
Comment 51•23 years ago
|
||
I've filed bug 123066 to address Kin's comment #50
Assignee | ||
Comment 52•23 years ago
|
||
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
Assignee | ||
Comment 53•23 years ago
|
||
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+
Reporter | ||
Comment 54•23 years ago
|
||
Mozilla now uses NSS 3.4, marking fixed.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 55•23 years ago
|
||
Now that this Bug (116334) has landed is the branch
NSS3.4:memoryfootprintreduction http://komodo.mozilla.org/planning/branches.cgi
going to be retired?
Comment 56•23 years ago
|
||
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.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•