1409516 - NSS Tests detect FIPS buildconfiguration using certutil --build-flags. gyp builds with --enable-fips enable init tests. Enable cert_rsa_exponent test. Add Linux64 FIPS gyp build to taskcluster/CI.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Kai Engert [:KaiE:]]

We have currently a very confusing state of our build systems with regards to FIPS enabled or disabled.

We have a mix of NSS_FORCE_FIPS, NSS_FIPS_DISABLED, NSS_TEST_ENABLE_FIPS.

We must clean this up to be consistent, or it will drive me insane.

See bug 1402410 and bug 1370667 for context.

Comment 1

[Kai Engert [:KaiE:]]

I want the default FIPS build mode to be the same, regardless what build system is used.

I want the same symbols to be used by both gyp and make.

What is the preferred way to parameterize gyp builds?
What syntax should be used to enable or disable FIPS in a gyp build?

Comment 2

[Kai Engert [:KaiE:]]

The syntax output of ./mach help prints:
    --enable-fips    don't disable FIPS checks.

This is confusing. There should be a single setting that controls both the building of the FIPS code and execution of FIPS tests.

Comment 3

[Kai Engert [:KaiE:]]

Bob, I'm worried that we make things even more confusing, if we completely revert all that we already have.

We had introduced the build control variable NSS_FORCE_FIPS.
And we have gyp --enable-fips.

This both suggests that using a default of "FIPS build disabled" will probably require a smaller amount of changes.

Could you accept that both make and gyp builds disable both FIPS code and FIPS tests by default?

If you can accept that, then I suggest that we keep the existing "gyp --enable-fips" syntax, but change everything underneath in the gyp build system to use the symbol NSS_FORCE_FIPS, and nothing else.

If you cannot accept that, we'd have to change existing "gyp --enable-fips" into a no-op, change NSS_FORCE_FIPS into a no-op, introduce a new "gyp --disable-fips", and introduce a new build control variable NSS_DISABLE_FIPS, and remove all other FIPS enable/disable symbols, and tell everyone building NSS today that all the old control variables are gone (in the next release notes).

Comment 4

[Robert Relyea]

It's not quite as simple as that. gyp doesn't actually run the tests. Once you have a .gyp build, you still run ./all.sh. That means there's no way for .gyp to affect the environment for the tests. Since .gyp can change options on the fly with the command line, it doesn't matter what the .gyp default is, it still has to somehow communicate that to ./all.sh.

I think that means we need a new way for .gyp to write what configuration it built somewhere ./all.sh can find it (rather than depending on the environment. It's not just fips, it's any of the .gyp options (debug/opt, 64bit/32bit, ecc flavor of the day, etc.)

My thought would be .gyp would write it's options into the build directory and ./all.sh would source them when it starts up. We could even update the make build system to do the same. That way you are always running a consistent set of tests with the build you made. The one downside to this is a couple of options determine the build directory (debug/opt/64bit/32bit/platform). I can think of several solutions to this, but I'm open for ideas. 

Anyway that needs to be solved before we can even make progress on the rest.

Comment 5

[Robert Relyea]

OK, I just thought of something we probably want long term:

We probably should have all build options have 2 flavors: default, on, or off. Currently we have default and environment to change the default. If we have default, on and off options, then we can set up environments that are impervious to default changes. We can do this by either looking at the environment variable value (1 or 0) or we can do this by using multiple environment variables.

That way if I set NSS_DISABLE_FIPS=0, both make and .gyp will build with FIPS. If I set NSS_DISABLE_FIPS=1 both will build with FIPS. (I'm going with NSS_DISABLE_FIPS, rather then NSS_FORCE_FIPS because I really believe the environment variable = 1 should match the -D line in the 'C' code). If you don't set it, you will get the default behavior of the build system, which could change from release to release.

That's just a suggestion and is independent of what we need to do as a minimum, but make help calm the arguments about what should be the default.

Comment 6

[Kai Engert [:KaiE:]]

(In reply to Robert Relyea from comment #4)
> It's not quite as simple as that. gyp doesn't actually run the tests. Once
> you have a .gyp build, you still run ./all.sh.

I believe that isn't the intention of the gyp build.

I see that Tim and Franziskus added the "mach" command, which provides a "mach test" command to run the tests, and which requires a mandatory parameter to select the group of tests to be executed:

usage: mach tests [-h]
                  {cipher,lowhash,chains,cert,dbtests,tools,fips,sdr,crmf,smime,ssl,ocsp,merge,pkits,ec,gtests,ssl_gtests}
mach tests: error: too few arguments

That commands sets the variables for all.sh like NSS_TESTS and NSS_CYCLES.

If the intention is to use "mach tests" for gyp builds, that command should probably be changed to set all the require parameters required for testing, in a way that matches the build configuration.

I notice that "mach tests" doesn't offer an "all" parameter to run "all tests". We should add that IMHO.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: 1409516-remove-reminder.patch

Once we've cleaned up, this code needs to be removed and replaced, see the comment.

Comment 8

[Martin Thomson [:mt:]]

Kai, Bob is right about the gyp build.  It affects what is built, not what is run.

The `mach tests` command is just a wrapper around `all.sh`.

My understanding is that NSS_FORCE_FIPS has two purposes:

1. In the make build (not the gyp build, which doesn't use this) it exists to force the running of initialization code, which on some platforms doesn't run automatically at the time the shared library is loaded.  It sets NSS_NO_INIT_SUPPORT.

2. In the tests it exists to cause FIPS tests to be run.

NSS_FIPS_DISABLED is set by default by the gyp build (but not by the make build).  Ideally those would agree, but it's not a huge problem.  That one seems to remove a few strategic pieces of FIPS-specific code from the build.

There is also NSS_FIPS, an environment variable that is consumed by some sysinit code in lowhash.  Maybe that isn't used any more.

Comment 9

[Kai Engert [:KaiE:]]

Ok, I'm taking back all my suggestions for defaults.
I accept that make and gyp use different defaults.

I have spent more time looking at the scripts and FIPS related variables, and I understand our status quo much better now. Thanks also to Bob and Martin for your explanations, which were helpful.

A comment for Bob, in your suggestion to use NSS_DISABLE_FIPS=0 as a build control variable to "enable FIPS", we'd have to use "don't define symbol NSS_DISABLE_FIPS" instead of defining it as zero, because the code uses #ifdef / #ifndef, and setting the macro to zero wouldn't make a difference for the #ifdef/#ifndef statements.

But your suggestion to use an environment variable to control the gyp build mode might not be aligned with the approach that the Mozilla team has used to implement the gyp build system, which uses parameters for a build.sh script. I believe they don't want to use environment variables.

Comment 10

[Kai Engert [:KaiE:]]

With that rsa exponent change, we have a new requirement:

In all.sh we want to execute different tests, based on the FIPS build
configuration.

But the gyp build system isn't based on environment variables, and
all.sh doesn't know what build configuration was used.


As of today, when used with a build produced by the make build system,
the all.sh test script can know the full build configuration by
looking at the environment variables that were used at build time.

I think it would be good if this continued to work.


Today, the make build system always builds most FIPS code.

Only the FIPS startup tests may be disabled in the make build system,
by not defining NSS_FORCE_FIPS.

As a consequence, when executing the all.sh script against a build produced
using the make build system, all FIPS tests should be executed,
unless it is told that it cannot do so.

For the RSA exponent test, we should be able to assume that we have been built
in the default FIPS build mode, unless we are told otherwise.


If the gyp build system wants to disable some fips code, then a gyp based
environment variable could be required to tell the the all.sh script that FIPS
code was disabled.

I think if a build was produced by defining C macro NSS_FIPS_DISABLED, then
environment variable NSS_FIPS_DISABLED=1 should be set when calling all.sh,
and the tests could be parameterized on that.


If you have trouble setting an environment variable prior to calling all.sh,
then we could use Bob's alternative suggestion: Write a text file to the
build directory, for example to the directory that contains the binaries.

The all.sh script could derive the directory from one of the utils,
e.g. dirname `which certutil`, and source the environment variables from that
text file.

Comment 11

[Kai Engert [:KaiE:]]

I couldn't figure out how to use an environment variable to tell all.sh that gyp set NSS_FIPS_DISABLED. The code that executes the tests doesn't seem to have direct access to the gyp build config, apparently.

Therefore I investigated the alternative approach. Writing build configuration to text file seems also tricky, because all.sh would have to discover from where it should load that text file.

It seems the easiest approach is to use a helper binary, nss-build-flags, which prints the build flags that are relevant for driving test execution.

I've implemented that approach, and nss-try looks good.

Comment 12

[Kai Engert [:KaiE:]]

Attachment: 1409516-v1.patch

This patch has the following components:

- it introduces a new utility nss-build-flags,
  which prints the set of NSS_FIPS_DISABLED / NSS_NO_INIT_SUPPORT flags
  that were defined at build time,
  and builds it with both the make and gyp builds.

- it changes all.sh to no longer look at NSS_FORCE_FIPS, but rather
  at the output of nss-build-flags

- it implements the pending TODO from bug 1402410,
  and changes cert.sh to test for small exponents success or failure
  depending on the use of NSS_FIPS_DISABLED at build time

- it introduces an additional flag for the gyp build system,
  which allows to enable the FIPS startup tests, too

- it extends the NSS taskcluster environment to run gyp FIPS builds
  on Linux

- the new tests uncovered that we also have a small exponent test
  in gtests, which also required a conditional test based 
  on NSS_FIPS_DISABLED

- it adds a section to the README file, that documents the flags
  we're using for FIPS build configuration.

- it removes the variables NSS_TEST_ENABLE_FIPS which is no longer
  necessary

A try run with this patch can be found here:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=ec38486731b466ae5da0826aa9a646f20b3a7dda

Comment 13

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8935020 [details] [diff] [review]
1409516-v1.patch

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

I'm fine with these changes in general. But I don't think we need that many new builds/tests and there are a couple nits.

::: automation/taskcluster/graph/src/extend.js
@@ +195,5 @@
>      image: LINUX_IMAGE,
>      features: ["allowPtrace"],
>    }, "--ubsan --asan");
>  
> +  await scheduleLinux("Linux 64 (FIPS debug)", {

If we actually want these builds, they should go into a FIPS group of the according platform (like it's done for the other FIPS builds).
I think it's enough when we have one FIPS gyp build with tests. This is more of a sanity check to make sure this build configuration works. If we do that, it would be ok to keep it as a platform and run all tests.

::: cmd/buildflags/buildflags.c
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I'm really unhappy with the cmd as is. This doesn't make it much worse so I guess that's fine. But it really needs a cleanup.

::: cmd/buildflags/manifest.mn
@@ +1,1 @@
> +# 

trailing whitespace

::: readme.md
@@ +156,5 @@
> +The legacy build system (make) by default disables these tests.
> +To enable these tests, set environment variable NSS_FORCE_FIPS=1 at build time.
> +
> +The gyp build system by default disables these tests.
> +To enable these tests, pass parameter --enable-fips-init to build.sh

missing .

@@ +167,5 @@
> +The legacy build system (make) never defines NSS_FIPS_DISABLED and always uses
> +the FIPS compliant code.
> +
> +The gyp build system by default defines NSS_FIPS_DISABLED.
> +To use the FIPS compliant code, pass parameter --enable-fips to build.sh

missing .

::: tests/all.sh
@@ -299,2 @@
>      RUN_FIPS="fips"
> -    export NSS_TEST_ENABLE_FIPS=1

If this is unused now, it should be removed from extend.js as well.

Comment 14

[Martin Thomson [:mt:]]

Kai,

Did you notice https://searchfox.org/nss/rev/20bdd18b84ba3f07b35733606087fcaaafc69b10/tests/common/init.sh#287

Comment 15

[Kai Engert [:KaiE:]]

Thanks for the review.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #13)
> 
> I don't think we need that many new builds/tests 

Ok, maybe it's sufficient to have just one 
  "--enable-fips --enable-fips-init --opt"
build.

> 
> ::: automation/taskcluster/graph/src/extend.js
> @@ +195,5 @@
> >      image: LINUX_IMAGE,
> >      features: ["allowPtrace"],
> >    }, "--ubsan --asan");
> >  
> > +  await scheduleLinux("Linux 64 (FIPS debug)", {
> 
> If we actually want these builds, they should go into a FIPS group of the
> according platform (like it's done for the other FIPS builds).
> I think it's enough when we have one FIPS gyp build with tests. This is more
> of a sanity check to make sure this build configuration works. If we do
> that, it would be ok to keep it as a platform and run all tests.

I guessed that you want something like this:

  await scheduleLinux("Linux 64 (FIPS opt)", {
    platform: "linux64",
    group: "FIPS",
    image: LINUX_IMAGE,
  }, "--enable-fips --enable-fips-init --opt");

Here's a try build:
  https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=bb92ca732b69a477d49aaeced653066cf0443646

But there are several surprises:

It has a modular build for FIPS, although that is supposed to be filtered out by collection. (I tried to explicitly specify collection: "opt", but I still got the modular build.)

It seems to duplicate the SSL tests, but they don't have different names and cannot be distinguished.

Do you have suggestions to improve this?

Comment 16

[Kai Engert [:KaiE:]]

(In reply to Martin Thomson [:mt:] from comment #14)
> https://searchfox.org/nss/rev/20bdd18b84ba3f07b35733606087fcaaafc69b10/tests/
> common/init.sh#287

No, thanks for the hint. I'll check if I can make use of this easily.

Comment 17

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie:) from comment #16)
> (In reply to Martin Thomson [:mt:] from comment #14)
> > https://searchfox.org/nss/rev/20bdd18b84ba3f07b35733606087fcaaafc69b10/tests/
> > common/init.sh#287
> 
> No, thanks for the hint. I'll check if I can make use of this easily.

Well, I guess I could use that to change all.sh to easily find a text file containing build information.

But I'd still have to implement two different mechanisms to write the build flags into a text file, one for the make build, one for the gyp build. The binary seems easier.

Comment 18

[Kai Engert [:KaiE:]]

Attachment: 1409516-v2.patch

Comment 19

[Kai Engert [:KaiE:]]

Patch v2 addresses the changes requests, but the question from comment 15 is still open

Comment 20

[Robert Relyea]

The binary is just another way of creating the text file. The only issue I have with it, is the the state of the FIPS flags aren't the only issue, it's any environment variable that can affect the build. at the very least, it should be designed to handle more than one environment/build variable even if the initial implementation only has one.

Comment 21

[Robert Relyea]

OK, I see how it's used. The build command is fine (I was thinking something like builds -isfips  would return something, but instead it just outputs all the variables and we search for the one we are interested in).

Comment 22

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8935358 [details] [diff] [review]
1409516-v2.patch

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

::: build.sh
@@ +97,5 @@
>          --with-nspr=?*) set_nspr_path "${1#*=}"; no_local_nspr=1 ;;
>          --system-nspr) set_nspr_path "/usr/include/nspr/:"; no_local_nspr=1 ;;
>          --enable-libpkix) gyp_params+=(-Ddisable_libpkix=0) ;;
>          --enable-fips) gyp_params+=(-Ddisable_fips=0) ;;
> +        --enable-fips-init) gyp_params+=(-Ddisable_fips_init=0) ;;

Do we really need this? I think I'd prefer if we'd set NSS_NO_INIT_SUPPORT on --enable-fips.

::: cmd/buildflags/buildflags.c
@@ +4,5 @@
> +
> +#include "prprf.h"
> +
> +int
> +main(int argc, char **argv)

I actually think we don't need this. You can read the config that was used to build from out/gyp_config. That's of course not available on make builds but that might be fine. It doesn't seem much more hassle and we would avoid introducing this new command.

Comment 23

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8935358 [details] [diff] [review]
1409516-v2.patch

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

::: automation/taskcluster/graph/src/extend.js
@@ +197,5 @@
>    }, "--ubsan --asan");
>  
> +  await scheduleLinux("Linux 64 (FIPS opt)", {
> +    platform: "linux64",
> +    group: "FIPS",

This doesn't need a group when it's a platform.

@@ +198,5 @@
>  
> +  await scheduleLinux("Linux 64 (FIPS opt)", {
> +    platform: "linux64",
> +    group: "FIPS",
> +    image: LINUX_IMAGE,

This should get a collection to not make it mix in another platform.
You can look at the ASAN build for example to see how to do this.
https://searchfox.org/nss/rev/ec2fece73a972ebf05133cb1633219032f82215e/automation/taskcluster/graph/src/extend.js#185

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #22)
> > +        --enable-fips-init) gyp_params+=(-Ddisable_fips_init=0) ;;
> 
> Do we really need this? I think I'd prefer if we'd set NSS_NO_INIT_SUPPORT
> on --enable-fips.

Sounds good.


> I actually think we don't need this. You can read the config that was used
> to build from out/gyp_config. That's of course not available on make builds
> but that might be fine. It doesn't seem much more hassle and we would avoid
> introducing this new command.


I'd prefer consistent detection of FIPS build parameter, regardless of the build system that was used.

If you want me to read from a file created by the gyp build, only, then the all.sh script must figure out from where to read it, and what to do if it doesn't exist.

If it doesn't exist in a make build, then we must fall back to classic detection of build mode - which would require to look at environment variables again, and we don't have consistency. I don't like this inconsistency and the additional complexity to figure out location of files.

If you don't like the introduction of a new separate utility, how about adding a parameterized mode to one of the existing tools, which can perform the equivalent? Hubert mentioned that this isn't uncommon, for example openssl uses this approach, too.

E.g. we could use
  certutil --print-build-flags

Comment 25

[Kai Engert [:KaiE:]]

Attachment: 1409516-v3.patch

I think I've addressed your suggestions.

However, instead of reading gyp_config, I've removed the separate nss-build-flags utility, and implemented "certutil --build-flags" which does the equivalent. My primary arguments are consistent behavior across build systems, and less complexity in all.sh discovering build modes from two different build systems.

I hope I have implemented the taskcluster collection in the way you wanted, following the existing asan approach. We get a new line "Linux x64 fips" which is treated as an optimized build.

full try build here:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=78c354b602f4881a74d342b705379c3e551c79df

I've implemented new try syntax, again following the linux64-asan example, which allows to individually select the gyp fips build.

try build limited to -p linux64-fips:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=0956b287286bdc7e89ac766105fd3631cf0cdb85

Comment 26

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8936775 [details] [diff] [review]
1409516-v3.patch

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

::: automation/taskcluster/graph/src/extend.js
@@ +195,5 @@
>      image: LINUX_IMAGE,
>      features: ["allowPtrace"],
>    }, "--ubsan --asan");
>  
> +  await scheduleLinux("Linux 64 (FIPS opt)", {

We should probably not run test builds in FIPS mode. They don't really make sense. (They work right now but I would not expect all things we add there to work.)
https://searchfox.org/nss/rev/a1d1d15a356dad547752bdc6bd8c1fe2a0e1d327/automation/taskcluster/graph/src/extend.js#86

::: automation/taskcluster/graph/src/try_syntax.js
@@ +21,5 @@
>      builds = ["d", "o"];
>    }
>  
>    // Parse platforms.
> +  let allPlatforms = ["linux", "linux64", "linux64-asan", "linux64-fips",

Can you add this to https://wiki.mozilla.org/NSS:TryServer?

Comment 27

[Kai Engert [:KaiE:]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #26)
> We should probably not run test builds in FIPS mode. They don't really make
> sense. (They work right now but I would not expect all things we add there
> to work.)
> https://searchfox.org/nss/rev/a1d1d15a356dad547752bdc6bd8c1fe2a0e1d327/
> automation/taskcluster/graph/src/extend.js#86


ok, I'll make the following change to exclude them:

--- a/automation/taskcluster/graph/src/extend.js
+++ b/automation/taskcluster/graph/src/extend.js
@@ -78,17 +78,17 @@ queue.filter(task => {
 
   // Only old make builds have -Ddisable_libpkix=0 and can run chain tests.
   if (task.tests == "chains" && task.collection != "make") {
     return false;
   }
 
   if (task.group == "Test") {
     // Don't run test builds on old make platforms
-    if (task.collection == "make") {
+    if (task.collection == "make" || task.collection == "fips") {
       return false;
     }
   }
 


> > +  let allPlatforms = ["linux", "linux64", "linux64-asan", "linux64-fips",
> 
> Can you add this to https://wiki.mozilla.org/NSS:TryServer?

Added linux64-fips, and updated remaining platforms items on the page to the current state.

Comment 28

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/481de926f1fa

Comment 29

[Kai Engert [:KaiE:]]

My original suggestion, to use a separate binary to print the build flags, could allow us to avoid the scenario I've described in bug 1434896.