If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Allow optionally building nss without softoken in the tree

RESOLVED FIXED in 3.15

Status

NSS
Build
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Elio Maldonado, Assigned: Elio Maldonado)

Tracking

3.13.5
3.15
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
The NSS buildsystem should allow downstream nss package maintainers to optionally build nss without softoken (freebl/util) in the source tree via suitable NSS build time environment variables.
(Assignee)

Comment 1

5 years ago
Fedora and Red Hat Enterprise Linux, and some of their derived distributions, split off the nss build and packaging into three separate but dependent packages. When nss (the higher layers) is built and the tests are run it needs to rely on softoken and util that are already installed on the system. We need to run the tests with the nss on built from the tree and the softoken libraries already installed. That way the testing will reflect what we actually ship to users. The softoken/freebl shared libraries are likely to be of an earlier version that the rest of nss due to it being the last FIPS 140 version.
(Assignee)

Comment 2

5 years ago
Created attachment 707728 [details] [diff] [review]
Provides option of building nss without softoken sources in the tree
Assignee: nobody → emaldona
Attachment #707728 - Flags: review?(rrelyea)
(Assignee)

Updated

5 years ago
(Assignee)

Comment 3

5 years ago
Originally, I intended to solve Bug 820207 first and make it a blocker of this one but there are some issues of portability I must resolve there. This one on the other hand, I believe can stand on it's own.
(Assignee)

Updated

5 years ago
Priority: -- → P2
Target Milestone: --- → 3.14.3

Comment 4

5 years ago
Comment on attachment 707728 [details] [diff] [review]
Provides option of building nss without softoken sources in the tree

r+ EXCEPT. .../nss/Makefile there should be no need to copy the libraries.
Attachment #707728 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 5

5 years ago
(In reply to Robert Relyea from comment #4)

> r+ EXCEPT. .../nss/Makefile there should be no need to copy the libraries.

The problems is that we still have tools left that still link statically with softoken. I have been chasing down tools that either require internal headers or link statically and have cleaned up things a bit not not completely. The ones left are: certcgi, rsaperf, and ocspclnt.

Comment 6

5 years ago
You can statically link without copying, you just need to include the right -L argument to the static libraries.
(Assignee)

Comment 7

5 years ago
Hhm, there is not softoken static library installed on the system. The reason I gave, though it has the correct facts, is not the right reason. What the makefile target copies are the shared libraries and the .chk files plus binaries like bltest as had suggested on https://bugzilla.redhat.com/show_bug.cgi?id=689918#c51. Let me review what led me to this and do the experiment in fedora of skipping the copying and see the cause of failures if any at all.

Comment 8

5 years ago
If the static libraries aren't on the system, then were are you copying them from?:).

bob
(Assignee)

Comment 9

5 years ago
Created attachment 710466 [details]
This is the copy script

Copies softoken/freebl shared libraries, their .chk files and a couple of tools.
I'm currently testing a variant that doesn't copy the tools as Bob has suggested. By the way, I'll see if I can inline this into the Makefile and saved me a script.
(Assignee)

Comment 10

5 years ago
(In reply to Robert Relyea from comment #8)
No static libraries get copied.
(Assignee)

Comment 11

5 years ago
Did the retesting in fedora and looked at prior versions. The reason for copying the softoken shared libraries is so that the mangling test in test/fips.sh can find the softokn3 shared library to copy into is mangling directory. If it can't find it the test will fail.
(Assignee)

Comment 12

5 years ago
Comment on attachment 707728 [details] [diff] [review]
Provides option of building nss without softoken sources in the tree

Review of attachment 707728 [details] [diff] [review]:
-----------------------------------------------------------------

r-, from self-review and retesting.

::: ./mozilla/security/nss/lib/manifest.mn
@@ +6,5 @@
>  DEPTH      = ../..
>  
> +UTIL_SRCDIR=util
> +FREEBL_SRCDIR=freebl
> +SOFTOKEN_SRCDIR=softoken

Don't set them no non epmty values first and try to conditionally set them to emty afterwards.

@@ +14,5 @@
> +ifdef USE_SYSTEM_FREEBL
> +ifdef USE_SYSTEM_SOFTOKEN
> +UTIL_SRCDIR=
> +FREEB_SRCDIRL=
> +SOFTOKEN_SRCDIR=

They won't set to empty as we want and the build will ail. I tried it.

@@ +18,5 @@
> +SOFTOKEN_SRCDIR=
> +endif
> +endif
> +endif
> +endif

Do it this simpler way that works

+# Check if we are building nss without softoken
+ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
+UTIL_SRCDIR=
+FREEBL_SRCDIR=
+SOFTOKEN_SRCDIR=
+else
+# default is to compile all
+UTIL_SRCDIR = util
+FREEBL_SRCDIR = freebl
+SOFTOKEN_SRCDIR = softoken
+endif
Attachment #707728 - Flags: feedback-
(Assignee)

Comment 13

5 years ago
Created attachment 710874 [details] [diff] [review]
Provides option of building nss with softoken in the tree
Attachment #707728 - Attachment is obsolete: true
Attachment #710466 - Attachment is obsolete: true
Attachment #710874 - Flags: review?(rrelyea)

Comment 14

5 years ago
> Did the retesting in fedora and looked at prior versions. The reason for copying the softoken
> shared libraries is so that the mangling test in test/fips.sh can find the softokn3 share
> library to copy into is mangling directory. If it can't find it the test will fail.

So update the tests. You have the system build environment variables...

bob
(Assignee)

Comment 15

5 years ago
Created attachment 712325 [details] [diff] [review]
Allow optionally building nss without softoken, freebl, or util in the tree

Uses the system libraries as Bob suggested.
Attachment #710874 - Attachment is obsolete: true
Attachment #710874 - Flags: review?(rrelyea)
Attachment #712325 - Flags: review?(rrelyea)
(Assignee)

Updated

5 years ago
Target Milestone: 3.14.3 → 3.14.4
(Assignee)

Updated

5 years ago
Attachment #712325 - Attachment description: support optionally building nss with softoken in the tree → Allow optionally building nss without softoken, freebl, or util in the tree
(Assignee)

Comment 16

5 years ago
Created attachment 720073 [details] [diff] [review]
Allow optionally building nss without softoken - updated for mercurial
Attachment #712325 - Attachment is obsolete: true
Attachment #712325 - Flags: review?(rrelyea)
Attachment #720073 - Flags: review?(rrelyea)

Comment 17

5 years ago
Comment on attachment 720073 [details] [diff] [review]
Allow optionally building nss without softoken - updated for mercurial

r+ But please make the following changes:

Move your BINDIR setting based on NSS_BUILD_WITHOUT_SOFTOKEN to the tests/common and rename it to FREEBL_BINDIR.

Define an environment variable so the FREEBL_BINDIR can be redefined. (some other platform my put the system installed tests somewhere else.
Attachment #720073 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 18

5 years ago
Checked in: https://hg.mozilla.org/projects/nss/rev/351b2d508a32
(Assignee)

Updated

5 years ago
Target Milestone: 3.14.4 → 3.15
(Assignee)

Comment 19

5 years ago
(In reply to Robert Relyea from comment #17)
> Comment on attachment 720073 [details] [diff] [review]
> Allow optionally building nss without softoken - updated for mercurial
> 
> r+ But please make the following changes:
> 
> Move your BINDIR setting based on NSS_BUILD_WITHOUT_SOFTOKEN to the
> tests/common and rename it to FREEBL_BINDIR.
> 
> Define an environment variable so the FREEBL_BINDIR can be redefined. (some
> other platform my put the system installed tests somewhere else.

The same goes for the fips.sh changes at the end where I see the use of /usr/lib{ARCH} to find the system installed version of softokn which is not cross-platform. I'll take advantage of the existing SOFTOKEN_LIB_DIR variable for this.
(Assignee)

Comment 20

5 years ago
Comment on attachment 720073 [details] [diff] [review]
Allow optionally building nss without softoken - updated for mercurial

Review of attachment 720073 [details] [diff] [review]:
-----------------------------------------------------------------

Fix fips.sh so the softoken shared libray can be found in the system in a portable fashion

::: tests/fips/fips.sh
@@ +225,5 @@
>    mkdir ${MANGLEDIR}
> +
> +  # For nss without softoken builds use the system softoken library
> +  [ "${USE_64}" == "1" ] && export ARCH="64" || export ARCH=""
> +  [ ${NSS_BUILD_WITHOUT_SOFTOKEN = "1" ] && FROM_DIR=/usr/lib${ARCH}/unsupported-tools || FROM_DIR=${BINDIR}

/usr/lib${ARCH} is not cross platform.

@@ +230,2 @@
>    for lib in `ls ${LIBDIR}`; do
> +    if [ ${lib} = ${DLL_PREFIX}softokn3.so ${DESTL} ]; then

${DESTL} is wrong and more impprtantly the test will never succeed bacause when ${NSS_BUILD_WITHOUT_SOFTOKEN = "1" `ls ${LIBDIR}` return a list without softoken int it. I don't see why we loop at all given that the manging is done only on the softoken library. In cause the copying is needed for some other reason I'll leave the loop for now and detect the missing library by testing for its existence in the mangling directory and copying it from the system if not there.

@@ +238,4 @@
>    done
>      
>    echo "$SCRIPTNAME: Detect mangled softoken--------------------------"
>    SOFTOKEN=${MANGLEDIR}/${DLL_PREFIX}softokn3.${DLL_SUFFIX}

check whether it is actually there and if not copy the system one.
if [ ! -e ${SOFTOKEN} ]; then
 # downstrean package maintainer would have set SOFTOKEN_LIB_DIR
 cp ${SOFTOKEN_LIB_DIR}/${DLL_PREFIX}softokn3.${DLL_SUFFIX} ${MANGLEDIR}
fi
(Assignee)

Comment 21

5 years ago
Created attachment 742479 [details] [diff] [review]
Address Bob's review requests for portability and fix others flaws
Attachment #720073 - Attachment is obsolete: true
Attachment #742479 - Flags: review?(rrelyea)

Comment 22

5 years ago
Comment on attachment 742479 [details] [diff] [review]
Address Bob's review requests for portability and fix others flaws

r+ rrelyea
Attachment #742479 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 23

5 years ago
Checked in to the tip for nss-3.15
https://hg.mozilla.org/projects/nss/rev/c997ba66a3f2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 24

5 years ago
Comment on attachment 742479 [details] [diff] [review]
Address Bob's review requests for portability and fix others flaws

Review of attachment 742479 [details] [diff] [review]:
-----------------------------------------------------------------

Elio: I have some comments and questions.

The most important problem is the very first comment about whether
the "USE_SYSTEM_FREEBL" in cmd/platlibs.mk is a typo.

::: cmd/platlibs.mk
@@ +35,5 @@
>  ifdef USE_STATIC_LIBS
>  
>  DEFINES += -DNSS_USE_STATIC_LIBS
>  # $(PROGRAM) has explicit dependencies on $(EXTRA_LIBS)
> +ifndef USE_SYSTEM_FREEBL

1. Is this "USE_SYSTEM_FREEBL" a typo of "NSS_USE_SYSTEM_FREEBL"?

Note that "USE_SYSTEM_FREEBL" also occurs in the comment on line 207.

2. The variable name "NSS_USE_SYSTEM_FREEBL" implies freebl is a separate
package on Fedora, rather than a part of the softoken package. Is that
true?

Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
library, so the variable name is a little misleading.

@@ +41,5 @@
> +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> +else
> +# Use the system freebl and softoken libraries
> +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> +SOFTOKENLIB=

Nit: use spaces around '=' in makefiles. The no-space style is only necessary
in shell scripts.

::: cmd/shlibsign/Makefile
@@ +46,5 @@
>  
>  # sign any and all shared libraries that contain the word freebl
> +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1))
> +CHECKLIBS=
> +CHECKLOC=

Nit: same here: in makefiles, use spaces around '='.

::: lib/Makefile
@@ +65,5 @@
> +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> +# Not included when building nss without softoken
> +UTIL_SRCDIR=
> +FREEBL_SRCDIR=
> +SOFTOKEN_SRCDIR=

Nit: use spaces around '='.

::: lib/manifest.mn
@@ +15,5 @@
>  #  ssl
>  #  smime
>  #  ckfw (builtins module)
>  #  crmf jar (not dll's)
> +DIRS = $(UTIL_SRCDIR) $(FREEBL_SRCDIR) $(SQLITE_SRCDIR) $(DBM_SRCDIR) $(SOFTOKEN_SRCDIR) \

Fold this long line.

::: tests/cipher/cipher.sh
@@ +123,5 @@
>  
>  ################## main #################################################
>  
> +# When building without softoken, bltest isn't built. It was already
> +# built and the cipher suite run as part of an nss-softoken build. 

"cipher suite" => "cipher test suite"?

@@ +125,5 @@
>  
> +# When building without softoken, bltest isn't built. It was already
> +# built and the cipher suite run as part of an nss-softoken build. 
> +if [ ! -x ${DIST}/${OBJDIR}/bin/bltest${PROG_SUFFIX} ]; then
> +    echo "bltest not built, skipping this test." >> ${LOGFILE}

If you skip the cipher.sh tests in this case, why did you make the FREEBL_BINDIR
related changes to allow running the tests with the system installed bltest tool?

::: tests/fips/fips.sh
@@ +233,5 @@
>  
>    echo "mangling ${SOFTOKEN}"
>    echo "mangle -i ${SOFTOKEN} -o -8 -b 5"
> +  # If nss was built without softoken use the system installed one.
> +  # It's location must be specified by the package maintainer.

How does the package maintainer specify the location of the system installed
softoken?  By setting the SOFTOKEN_LIB_DIR environment variable?  The comment
should explain this more precisely.

Comment 25

5 years ago
Created attachment 744414 [details] [diff] [review]
Remove an extraneous ')' after an ifeq directive in nss/cmd/shlibsign/Makefile

This extraneous ')' causes a warning under GNU make:
    Makefile:48: Extraneous text after `ifeq' directive
but breaks the Mozilla build, whose pymake is stricter than
GNU make.

I also added the spaces around '=' that I suggested.

https://hg.mozilla.org/projects/nss/rev/c02120e4c1e7
Attachment #744414 - Flags: review?(emaldona)
Attachment #744414 - Flags: checked-in+
(Assignee)

Updated

5 years ago
Attachment #744414 - Flags: review?(emaldona) → review+
(Assignee)

Comment 26

4 years ago
(In reply to Wan-Teh Chang from comment #24)
> Comment on attachment 742479 [details] [diff] [review]
> Address Bob's review requests for portability and fix others flaws
> 
> Review of attachment 742479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Elio: I have some comments and questions.
> 
> The most important problem is the very first comment about whether
> the "USE_SYSTEM_FREEBL" in cmd/platlibs.mk is a typo.
> 
> ::: cmd/platlibs.mk
> @@ +35,5 @@
> >  ifdef USE_STATIC_LIBS
> >  
> >  DEFINES += -DNSS_USE_STATIC_LIBS
> >  # $(PROGRAM) has explicit dependencies on $(EXTRA_LIBS)
> > +ifndef USE_SYSTEM_FREEBL
> 
> 1. Is this "USE_SYSTEM_FREEBL" a typo of "NSS_USE_SYSTEM_FREEBL"?
It should be NSS_USE_SYSTEM_FREEBL. For some reason I exprted USE_SYSTEM_FREEBL=1 in the nss.spec file which is wher I set all the environment variables needed to control the type of build.

> 
> Note that "USE_SYSTEM_FREEBL" also occurs in the comment on line 207.
> 
> 2. The variable name "NSS_USE_SYSTEM_FREEBL" implies freebl is a separate
> package on Fedora, rather than a part of the softoken package. Is that
> true?
> 
> Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
> library, so the variable name is a little misleading.
> 
> @@ +41,5 @@
> > +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> > +else
> > +# Use the system freebl and softoken libraries
> > +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> > +SOFTOKENLIB=

Yes, NSS_USE_SYSTEM_FREEBL is a bit missleading or confusing. I'm actually trying to set both SOFTOKENLIB and CRYPTOLIB. I need to think a bit on how best to change it.
> 
> Nit: use spaces around '=' in makefiles. The no-space style is only necessary
> in shell scripts.
> 
> ::: cmd/shlibsign/Makefile
> @@ +46,5 @@
> >  
> >  # sign any and all shared libraries that contain the word freebl
> > +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1))
> > +CHECKLIBS=
> > +CHECKLOC=
> 
> Nit: same here: in makefiles, use spaces around '='.
> 
> ::: lib/Makefile
> @@ +65,5 @@
> > +ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> > +# Not included when building nss without softoken
> > +UTIL_SRCDIR=
> > +FREEBL_SRCDIR=
> > +SOFTOKEN_SRCDIR=
> 
> Nit: use spaces around '='.
> 
> ::: lib/manifest.mn

I'll add spaces for the Makefiles.

> @@ +15,5 @@
> >  #  ssl
> >  #  smime
> >  #  ckfw (builtins module)
> >  #  crmf jar (not dll's)
> > +DIRS = $(UTIL_SRCDIR) $(FREEBL_SRCDIR) $(SQLITE_SRCDIR) $(DBM_SRCDIR) $(SOFTOKEN_SRCDIR) \
> 
> Fold this long line.
Yes, I will change this.

> 
> ::: tests/cipher/cipher.sh
> @@ +123,5 @@
> >  
> >  ################## main #################################################
> >  
> > +# When building without softoken, bltest isn't built. It was already
> > +# built and the cipher suite run as part of an nss-softoken build. 
> 
> "cipher suite" => "cipher test suite"?
That a better name and in a parethethis perhaps say it's the blapi test.

> 
> @@ +125,5 @@
> >  
> > +# When building without softoken, bltest isn't built. It was already
> > +# built and the cipher suite run as part of an nss-softoken build. 
> > +if [ ! -x ${DIST}/${OBJDIR}/bin/bltest${PROG_SUFFIX} ]; then
> > +    echo "bltest not built, skipping this test." >> ${LOGFILE}
> 
> If you skip the cipher.sh tests in this case, why did you make the
> FREEBL_BINDIR
> related changes to allow running the tests with the system installed bltest
> tool?

I think at some point I was trying to rerun the suite but now I don't. I can remove FREEBL_BINDIR. I make need something like this but for experssing the location on the installed un-supported tools but that relates to the other bug for spliiting the rsa performance tool into high and level components whose patch isn't ready yet. Stay tuned.

> 
> ::: tests/fips/fips.sh
> @@ +233,5 @@
> >  
> >    echo "mangling ${SOFTOKEN}"
> >    echo "mangle -i ${SOFTOKEN} -o -8 -b 5"
> > +  # If nss was built without softoken use the system installed one.
> > +  # It's location must be specified by the package maintainer.
> 
> How does the package maintainer specify the location of the system installed
> softoken?  By setting the SOFTOKEN_LIB_DIR environment variable?  The comment
> should explain this more precisely.

I'll point to coreconf/location.mk where three years ago I added varaiables under the control of the package maintainer for finding the location of the sytem installed headers and libraries.  They were modelled on what had been done for NSPR. If the maintainer sets then we use them otherwise we rely on the variables based on the dist directory. I have some commenting to do.

I am considering writing a wiki page. In Fedora we rely on the 
{nss|nss-softokn|nss-util}.spec files as to control the whole building, testing, and packaging process. They could be though at meta Makefiles above the tree. There may be something analogous in other plaforms, say debian-based sytems. Until I do let's see how much clarity I can give with comments in the the Makefiels, manisfest.ms and shell scripts.
(Assignee)

Comment 27

4 years ago
> Note that NSS_USE_SYSTEM_FREEBL also causes NSS to use the system softoken
> library, so the variable name is a little misleading.
> 
> @@ +41,5 @@
> > +SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
> > +else
> > +# Use the system freebl and softoken libraries
> > +CRYPTOLIB=$(FREEBL_LIB_DIR)/$(LIB_PREFIX)freebl.$(LIB_SUFFIX)
> > +SOFTOKENLIB=

The reason that I choose NSS_USE_SYSTEM_FREEBL instead of NSS_USE_SYSTEM_SOFTOKEN is that this is in the use static libraries section. We set CRYPTOLIB to freebl library in the system whereas NSS_USE_SYSTEM_FREEBL is set to empty. A few tools still link statically and so does gblinc on RHEL and Fedora does. On fedora, as well as other Linux distributions, the use of static libraries is frowned upon. An exception was granted for glibc whose little hashing library needs to link statically with libfreebl.a so its gets FIPS 140 compliance via nss. We don't even install a softoken.a static library that's why SOFTOKENLIB is empty. I'll add comments explaining this is as brief a fashion as I can.
(Assignee)

Comment 28

4 years ago
On NSS_BUILD_WITHOUT_SOFTOKEN we shoudln't build shlibsign either.
(Assignee)

Comment 29

4 years ago
Created attachment 745736 [details] [diff] [review]
Remove unneeded changes, fix lack o fwhitespace, add better comments and ...

when building nss with softoken skip shlibsign as we do with bltest and fisptest.
(Assignee)

Updated

4 years ago
Attachment #745736 - Flags: review?(wtc)
(Assignee)

Updated

4 years ago
Attachment #745736 - Attachment description: Remove unneeded changes and addcomments per request from wtc and ... → Remove unneeded changes, fix lack o fwhitespace, add better comments and ...

Comment 30

4 years ago
Comment on attachment 745736 [details] [diff] [review]
Remove unneeded changes, fix lack o fwhitespace, add better comments and ...

Review of attachment 745736 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: cmd/Makefile
@@ +16,5 @@
>  
>  ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> +BLTEST_SRCDIR      =
> +FIPSTEST_SRCDIR    =
> +SHLIBSIGN_SRCDIR   =

Nit: in new code, I don't recommend aligning the equal signs like this.
They are a pain to maintain.

::: cmd/platlibs.mk
@@ +41,5 @@
>  SOFTOKENLIB=$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
>  else
> +# Use the system installed freebl static library and set softoken one to empty.
> +# Some tools need to link statically with freebl but none with softoken. Only
> +# the softoken shared library is installed in the system not the static one.

Nit: put "not the static one" after "shared library".

::: lib/Makefile
@@ +70,4 @@
>  else
>  # default is to include all
> +UTIL_SRCDIR     = util
> +FREEBL_SRCDIR   = freebl

Don't align the equal signs in new code.

All I am asking is that there is one space between a variable and '='.
Attachment #745736 - Flags: review?(wtc) → review+
(Assignee)

Comment 31

4 years ago
Patched committed to the tip for nss-3.15
https://hg.mozilla.org/projects/nss/rev/6fb9ff40d054
You need to log in before you can comment on or make changes to this bug.