358785 - Merge NSS_LIBPKIX_BRANCH back to trunk

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

The code developed on NSS_LIBPKIX_BRANCH should be merged into the trunk of NSS development, and once it is done, work on the branch should stop.

It would be good to get this done this week.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Diff between automated merge (w conflicts) and after attempting to resolve

This diff compares two directories on my system.

The left hand side was the result of using "cvs update -j" on the trunk. It contains about 20 conflicts.

The right hand side is the result of my manual attempt to clean up the conflicts.

The biggest code conflict in code was in file genname.c, my attempt to resolve the conflict is most likely wrong...?
I have already asked Julien to help and produce a merged version of that file.

Another larger conflict is the code of the new HTTP client callback API.
It seems cvs was not smart merging those portions, several sections were duplicated, maybe there were too many whitespace changes.
I believe I merged it correctly.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Diff showing changes to existing files only (with conflicts)

When merging the pkix branch to the trunk, and still having the conflicts in the tree, this diff lists all the changes to the existing files. It ignores new files.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: Diff showing changes to existing files only (most conflicts removed, genname pending)

This diff was produced after my attempt to remove the conflicts.
It still requires a correct genname.c merge.

Comment 4

[Wan-Teh Chang]

Comment on attachment 244112 [details] [diff] [review]
Diff showing changes to existing files only (most conflicts removed, genname pending)

All the changes to mozilla/security/coreconf should be ignored.

Comment 5

[Wan-Teh Chang]

Attachment: Bug fixes for secoid.c

Fix the x509PolicyConstraints OID (you can verify by searching
for id-ce-policyConstraints in RFC 3280).  Fix errors in a comment.

Comment 6

[Wan-Teh Chang]

Attachment: List of new files added by libpkix

I suggest that we land libpkix's new files on the NSS trunk
first.  This will make it easy to review them and the changes
to existing files.

Please use review+ to indicate your approval of checking in
the new files listed in this attachment on the NSS trunk.
The contents of the new files will be taken from the
NSS_LIBPKIX_BRANCH, with the license header replaced by the
MPL/GPL/LGPL triple license.

Comment 7

[Wan-Teh Chang]

Attachment: List of existing files modified by libpkix

As this attachment shows, libpkix only modifies a small number
of existing files.  I will attach the patch for these files next.

Comment 8

[Wan-Teh Chang]

Attachment: libpkix patch for existing files

I examined all the changes to existing files made by libpkix.
I resolved all the merging conflicts, including the conflicts
in lib/certdb/genname.c.  I only made a small number of editing
changes to make the coding style blend in better with existing
NSS code.  I plan to implement a better solution for exporting
the PKIX symbols from nss.def for the PKIX unit tests, but you
can start reviewing and experimenting with this patch.

I do have some suggested changes based on my review.

1. secoidt.h and secoid.c

I suggest that we change inhibitAny to inhibitAnyPolicy.

Note that the patch changes three policy-related extensions
from UNSUPPORTED to SUPPORTED.

2. cert.h

Note that the patch changes the function prototype of
CERT_FindCRLNumberExten.  This function is not exported
in nss.def, even though it is declared in the public
header cert.h.

3. certt.h

I suggest that we shorten the following policy-related
type names.

CERTCertificatePolicyMappings => CERTPolicyMappings
CERTCertificatePolicyConstraints => CERTPolicyConstraints
CERTCertificateInhibitAny => CERTInhibitAnyPolicy

In CERTCRLEntryReasonCode (CRL entry reason code),
"Unspecified" is called "unused" in RFC 3280, and
"certificatedHold" should be renamed "CertificateHold".
RFC 3280 doesn't have "RemoveFromCRL".  The extra
reason code "RemoveFromCRL" caused the values of
"PrivilegeWithdrawn" and "AcCompromise" to be one
greater than their values in RFC 3280.

4. ocsp.h and nss.def

Note that we export the function GetRegisteredHttpClient.
This function, whose name doesn't have a prefix, doesn't
match our naming convention for public functions.

Comment 9

[Wan-Teh Chang]

Attachment: libpkix patch for existing files, v2

I found that I missed four CERT_ functions that the libpkix
branch exports from nss.def, so I added them to the NSS_3.12
section.  I can build and run all the test programs without
exporting those four CERT_ functions, so I don't know why they
are exported.  Note that one of these four CERT_ functions is
CERT_FindCRLNumberExten, whose function prototype is changed
by libpkix.

I also found that the new libpkix files already use the
MPL/GPL/LGPL triple license.  The only problem is that they
say:
  The Original Code is the Netscape security libraries.

  The Initial Developer of the Original Code is
  Netscape Communications Corporation.

  Copyright (C) 1994-2000

Ideally these should be changed to reflect the fact that
Sun wrote the original code (libpkix) starting in 2004.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 244619 [details]
List of new files added by libpkix

I've never been asked to review a list of file names before.  I'm not sure how to review them.  
Here are some comments.

It seems like this list has a lot of directories, each of which holds just a single .c file, a manifest, and a Makefile.  
Are these necessary?
Does each build a separate executable?
If not, I'd surely prefer that the .c files be consolidated into fewer directories.  Remember that, once added, directories can never be removed from the CVS repository or workareas pulled therefrom.

Also, up until now, we've carefully avoided having checked-in copies of cert and key DBs for testing.  Is this now unavoidable?

Comment 11

[Robert Relyea]

Comment on attachment 244522 [details] [diff] [review]
Bug fixes for secoid.c

r=relyea

Comment 12

[Robert Relyea]

Comment on attachment 244619 [details]
List of new files added by libpkix

These files are all in libpkix directories, r=relyea to check in.
No review of the content has been make.

Comment 13

[Wan-Teh Chang]

Comment on attachment 244522 [details] [diff] [review]
Bug fixes for secoid.c

I checked in the patch "bug fixes for secoid.c" on the NSS trunk (NSS 3.12).

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.33; previous revision: 1.32
done

Comment 14

[Wan-Teh Chang]

Comment on attachment 244619 [details]
List of new files added by libpkix

I merged the mozilla/security/nss/lib/libpkix directory onto the
NSS trunk.  It is not being built yet.

Comment 15

[Wan-Teh Chang]

Attachment: Proposed cmd/libpkix file list

Nelson, in comment 10 you noted that many directories under
cmd/libpkix contain only Makefile, manifest.mn, and a single .c file.

I collapsed one level of these directories.  This is the list of files.
Please review it.  Note that I kept the subdirectories under
cmd/libpkix/{pkix,pkix_pl} so that they match the subdirectories
under lib/libpkix/{pkix,pkix_pl}.  A small number of these subdirectories
have only one or two .c files.

Do you want me to further collapse the subdirectories under
cmd/libpkix{pkix,pkix_pl}?  Or is this list good enough?

Comment 16

[Wan-Teh Chang]

Comment on attachment 256239 [details]
Proposed cmd/libpkix file list

This consolidation of test programs reduced the number of new files
in cmd/libpkix from 239 to 109.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256239 [details]
Proposed cmd/libpkix file list

Wan-Teh, this is a HUGE improvement.  I think this is fine.  Thank you very much!

Comment 18

[Wan-Teh Chang]

Comment on attachment 256239 [details]
Proposed cmd/libpkix file list

I added the new files listed in this patch on the
NSS trunk (NSS 3.12).  They are all under the directory
mozilla/security/nss/cmd/libpkix.

Comment 19

[Wan-Teh Chang]

Attachment: libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs)

I will provide two solutions for giving the libpkix test programs
access to the LIBPKIX functions.

This patch is solution 1.  By default, we build the regular libnss3.so
and don't build and run the libpkix test programs.  If the environment
variable BUILD_LIBPKIX_TESTS is set to 1, we build the special libnss3.so
that exports the LIBPKIX functions, and build and run the libpkix test
programs.

Tomorrow I will attach another patch for solution 2: link the libpkix
test programs with NSS static libraries.

Comment 20

[Wan-Teh Chang]

Attachment: libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v2

I cleaned up the patch a little.

Comment 21

[Wan-Teh Chang]

Comment on attachment 256690 [details] [diff] [review]
libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v2

The main code cleanup is that the newly exported function
GetRegisteredHttpClient (defined in ocsp.c) is renamed
SEC_GetRegisteredHttpClient.

Comment 22

[Wan-Teh Chang]

Attachment: libpkix patch for existing files (link libpkix test programs with static libraries)

In this alternative patch, we link the libpkix test programs with
NSS static libraries, so that we don't need to export the libpkix
functions from libnss3.so/nss3.dll.

Comment 23

[Julien Pierre]

Wan-Teh,

Using static NSS libs with the many libpkix test programs will cause issues for builds. The files will be much bigger, and build times will be higher. When you multiply the disk space by each platform, this becomes significant. Also, many official builds and tinderboxes are done on network drives concurrently, and the network and disk bandwidth requirements will go up in addition to the space. I would advise using dynamic libraries.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

maybe you shoul prefix the names with double-underscore to signify that 
they're private symbols.

Comment 25

[Wan-Teh Chang]

I landed the libpkix test scripts and data in the
mozilla/security/nss/tests/libpkix directory on the
NSS trunk (NSS 3.12).

Three directories
(mozilla/security/nss/tests/libpkix/{pkix_pl_tests/module,pkix_tests/top,sample_apps/cert8.db})
contain NSS database files (cert8.db,key3.db,secmod.db).
We'll deal with them later.

Comment 26

[Wan-Teh Chang]

I made a minor mistake.  The three directories should be
mozilla/security/nss/tests/libpkix/{pkix_pl_tests/module,pkix_tests/top,sample_apps}.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256690 [details] [diff] [review]
libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v2

Alexei, please review this patch and the next one attached, and choose one.

Comment 28

[Alexei Volkov]

Attachment: libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v3

code cleanup. changes in crlv2.c, policyxtn.c. Julien please review crl.c changes in cache functions.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 244522 [details] [diff] [review]
Bug fixes for secoid.c

This corrective patch should also have been applied to the 3.11 branch, IMO.  
I will make this correction on the branch as part of another patch.

Comment 30

[Julien Pierre]

Comment on attachment 263373 [details] [diff] [review]
 libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v3

I gave Alexei my review feedback in person. Some changes are needed.

Comment 31

[Alexei Volkov]

Attachment: libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v4

changes for problems found during review.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 265969 [details] [diff] [review]
libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v4

I think Alexei meant to ask Julien for this review.

Comment 33

[Julien Pierre]

Comment on attachment 265969 [details] [diff] [review]
libpkix patch for existing files (set BUILD_LIBPKIX_TESTS for building libpkix test programs) v4

There is still a problem in CERT_FinCRLEntryReasonExten . wrapperItem needs to be initialized . I believe the assignment as written only initializes tmpItem .

Comment 34

[Julien Pierre]

Other comments :

- Even though many existing destroy functions return void, I think returning void is a bad idea for public functions in general. So, I recommend we don't propagate this error because of history, and make CERT_DestroyPolicyMappingsExtension return SECStatus instead of void . If relevant errors pop up in the future, we won't be able to tell the caller about them if the function is made public and returning void now.

- A little merge work is needed for nss.def / secoid.c / secoidt.h for this patch to apply . I understand the patch was created against an old tree which you kept at my request in order to diff the 2 patches. But the next submission should probably made against the trunk.

- Re: the typo in the field of a public structure, DERPermited , I am of the opinion that it's OK to rename it to DERPermitted . This won't break binary compatibility. It may break source compatibility, but in that unlikely event, the application would have an easy 1-letter fix .

Comment 35

[Alexei Volkov]

Attachment: patch v5. Fix about last Julien's comments(except one about DERpremited)

builds and passes all nss tests

Comment 36

[Alexei Volkov]

Attachment: last patch failed to fix initialization of wrapperItem. Fixing it now. v6

Comment 37

[Julien Pierre]

Comment on attachment 266034 [details] [diff] [review]
last patch failed to fix initialization of wrapperItem. Fixing it now. v6

This looks good, Alexei. Thank you !

Comment 38

[Alexei Volkov]

v6 is checked into trunk
/cvsroot/mozilla/security/nss/cmd/Makefile,v  <--  Makefile
new revision: 1.7; previous revision: 1.6
/cvsroot/mozilla/security/nss/cmd/platlibs.mk,v  <--  platlibs.mk
new revision: 1.48; previous revision: 1.47
/cvsroot/mozilla/security/nss/lib/manifest.mn,v  <--  manifest.mn
new revision: 1.17; previous revision: 1.16
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.57; previous revision: 1.56
/cvsroot/mozilla/security/nss/lib/certdb/certi.h,v  <--  certi.h
new revision: 1.17; previous revision: 1.16
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.36; previous revision: 1.35
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.56; previous revision: 1.55
/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.32; previous revision: 1.31
/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.7; previous revision: 1.6
/cvsroot/mozilla/security/nss/lib/certhigh/crlv2.c,v  <--  crlv2.c
new revision: 1.5; previous revision: 1.4
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.42; previous revision: 1.41
/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.h,v  <--  ocsp.h
new revision: 1.11; previous revision: 1.10
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/certsel/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/crlsel/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/params/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/results/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/store/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/top/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/util/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_aiamgr.c,v  <--  pkix_pl_aiamgr.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpcertstore.c,v  <--  pkix_pl_httpcertstore.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_generalname.c,v  <--  pkix_pl_generalname.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_nameconstraints.c,v  <--  pkix_pl_nameconstraints.c
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_ocspresponse.c,v  <--  pkix_pl_ocspresponse.c
new revision: 1.4; previous revision: 1.3
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/system/manifest.mn,v  <--  manifest.mn
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/security/nss/lib/nss/Makefile,v  <--  Makefile
new revision: 1.5; previous revision: 1.4
/cvsroot/mozilla/security/nss/lib/nss/config.mk,v  <--  config.mk
new revision: 1.28; previous revision: 1.27
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.175; previous revision: 1.174
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.80; previous revision: 1.79
RCS file: /cvsroot/mozilla/security/nss/lib/nss/pkixpriv.def,v
/cvsroot/mozilla/security/nss/lib/nss/pkixpriv.def,v  <--  pkixpriv.def
initial revision: 1.1
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.37; previous revision: 1.36
/cvsroot/mozilla/security/nss/tests/all.sh,v  <--  all.sh
new revision: 1.28; previous revision: 1.27

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

> Re: the typo in the field of a public structure, DERPermited , I am of the
> opinion that it's OK to rename it to DERPermitted . This won't break binary
> compatibility. It may break source compatibility, but in that unlikely event,
> the application would have an easy 1-letter fix .

Julien, I think you have forgotten the vehemence with which Sun server teams
chastised us over changes of this nature in the past.  

The safe way to make such changes for the names of structure members 
is shown in this pseudo patch.

 struct foo {
 ...
-    int oldname;
+    int newname;
 ...
 }
+#define oldname newname

Comment 40

[Julien Pierre]

There is a build failure on HP-UX in the pkixutil.sl shared library.

grep -v ';+' pkixutil.def | grep -v ';-' | sed -e 's; DATA ;;' -e 's,;;,,' -e 's,;.*,,' -e 's,^,+e ,' > HP-UXB.11.11_DBG.
OBJ/pkixutil.def
rm -f HP-UXB.11.11_DBG.OBJ/libpkixutil.sl
ld -b +h libpkixutil.sl -c HP-UXB.11.11_DBG.OBJ/pkixutil.def -o HP-UXB.11.11_DBG.OBJ/libpkixutil.sl HP-UXB.11.11_DBG.OBJ/
testutil_nss.o HP-UXB.11.11_DBG.OBJ/testutil.o    ../../../../../dist/HP-UXB.11.11_DBG.OBJ/lib/libsectool.a  -Wl,+b,'$ORI
GIN/../lib' -L../../../../../dist/HP-UXB.11.11_DBG.OBJ/lib -lssl3 -lsmime3 -lnss3 -lnssutil3 -L../../../../../dist/HP-UXB
.11.11_DBG.OBJ/lib -lplc4 -lplds4 -lnspr4  -lpthread -lm -lrt
ld: Unrecognized argument: -Wl,+b,$ORIGIN/../lib
ld: Usage:  ld [options] [flags] files
gmake-3.80[2]: *** [HP-UXB.11.11_DBG.OBJ/libpkixutil.sl] Error 1
gmake-3.80[2]: Leaving directory `/net/monstre/export/home/julien/nss/tip2/mozilla/security/nss/cmd/libpkix/testutil'
gmake-3.80[1]: *** [libs] Error 2
gmake-3.80[1]: Leaving directory `/net/monstre/export/home/julien/nss/tip2/mozilla/security/nss/cmd/libpkix'
gmake-3.80: *** [libs] Error 2

Comment 41

[Julien Pierre]

Note that this failure goes undetected in Christophe's builds or tinderbox because they do not set BUILD_LIBPKIX_TESTS . But I have that set in my .cshrc .

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Any WHY don't they build libpkix tests?

Comment 43

[Julien Pierre]

Nelson,

Building libpkix tests currently causes a different NSS shared library to be built, one that has all the pkix symbols made public, so that the libpkix test programs can call them. That is not fit for release.

I think it should be OK for the tinderboxes to build and test this way, since we don't release from tinderbox.

For the nightly builds, I have suggested before that we build two versions of libnss3, one with the symbols public and another with them private.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

What part of this bug remains undone?  
What code from the branch has not yet been merged to the trunk?