1313122 - Remove bypass tests as latest NSS has removed PKCS#11 bypass support

Status: RESOLVED FIXED

(JSS Graveyard :: Tests, defect)

Description

[Elio Maldonado]

Attachment: Removes obsolete bypass tests and adds dbm: prefix to -d option in others

Fix for Bug 1263017 removed NSS PKCS#11 bypass support. The JSS bypass tests on jss/org/mozilla/jss/tests/all.pl should be removed.

Comment 1

[Elio Maldonado]

Attachment: Builds jss and runs tests

I used this to test. It builds nspr, nss, and jss and runs the tests. This tries to emulate what's done by https://hg.mozilla.org/projects/nss/file/tip/automation/buildbot-slave/build.sh.

Comment 2

[Elio Maldonado]

Attachment: results I had when the patch is not applied

Comment 3

[Elio Maldonado]

Attachment: results I had when the patch was applied

Comment 4

[Elio Maldonado]

Attachment: Build nspr, nss and jss and run the jss tests.

Slightly cleaner version. Should require less modifications on the part of the reviewers to run it on their environment.

Comment 5

[Elio Maldonado]

Christina pointed out copy errors and also we need to change the last command so it can be run by itself. As a minimum should make these two changes:

--- testAsAttachedToBug.sh	2016-10-29 08:33:44.534334570 -0700
+++ buildAndTest.sh	2016-10-29 08:43:07.109510305 -0700
@@ -27,10 +27,10 @@
 
 mkdir $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_glibc_PTH_64_DBG.OBJ/bin
 pushd $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_glibc_PTH_64_DBG.OBJ/lib
-cp $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib /lib64/*nssckbi* .
+cp $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/*nssckbi* .
 cp $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_cc_glibc_PTH_64_DBG.OBJ/bin/pk12util ../bin
 popd
 
-/usr/bin/perl $CHECKOUT/jss/org/mozilla/jss/tests/all.pl dist $CHECKOUT/dist/Linux${VERSION:-4.7}_x86_64_glibc_PTH_64_DBG.OBJ
+USE_64=1 JAVA_HOME=$(dirname $(dirname $(readlink -f $(which javac)))); /usr/bin/perl `pwd`/jss/org/mozilla/jss/tests/all.pl dist `pwd`/dist/Linux${VERSION:-version}_x86_64_glibc_PTH_64_DBG.OBJ

Currently in works fine on some systems but is failing on others.

Comment 6

[Elio Maldonado]

Attachment: Builds nspr, nss and jss and runs the jss tests

Adding cp ./jss/org/mozilla/jss/tests/passwords . took care of the problems I had. This one works on multiple systems, tested on f24, f25 beta, rawhide, and rhel-7.

Comment 7

[Elio Maldonado]

Attachment: Removes obsolete bypass tests

This version doesn't just removes the bypass test as requested, no more and no less, and doesn't add the dbm: prefix to -d option as the previous one did.

Comment 8

[Elio Maldonado]

Attachment: Builds nspr, nss and jss and runs the jss tests

Comment 9

[Elio Maldonado]

Attachment: The run.sh uses this file that sets the environment

I'll explain how I test in a subsequent comment.

Comment 10

[Elio Maldonado]

Attachment: Additional patch required by macOS

Below I quote from an email exchange I had with Matt.

>> On 11/10/2016 08:43 PM, Matthew Harmsen wrote:
>>>
>>> Elio,
>>>
>>> Sorry for the delay, but I was finally able to determine that the 'hg/dist' directory still exists when JSS is being built, but it is looking for the wrong OBJDIR_NAME.
>>>
>>> After debugging the 'run.sh' and associated 'hg/nss/automation/buildbot-slave/build.sh' scripts, I discovered the issue - basically, nss builds an OBJDIR_NAME in 'dist' of 'Linux4.7_x86_64_cc_glibc_PTH_64_DBG.OBJ' whereas JSS is looking for an OBJDIR_NAME in 'dist' of ' Linux4.7_x86_64_glibc_PTH_64_DBG.OBJ'.
>>>
>>> Basically, NSS computes a COMPILER_TAG of "_cc" for the OBJDIR_NAME, while JSS believes that the default compiler is being utilized and looks for an OBJDIR_NAME which contains no COMPILER_TAG as a part of its name.
>>>
>>> This appears to be due to a difference between the "nss/coreconf" and "jss/coreconf" directories.
>>>
>>> Prior to discovering this, I had created a new function for "hg/nss/automation/buildbot-slave/build.sh" called before "build_jss" which creates a JSS OBJDIR_NAME symlink to the NSS OBJDIR_NAME, and everything worked.
>> I'm curious about this. Could you share with us?
> I have attached the patch that I was using, but found the "core" problem before going back and trying to rid myself of certain "hard-codings" that are probably not acceptable (e. g. - OBJDIR_NAME_COMPILER).
>
> If I am following the logic correctly, if COMPILER_TAG is not defined (my suggested workaround), it sets it in nss/coreconf/Linux.mk as:
>
>     ifndef COMPILER_TAG
>     COMPILER_TAG := _$(CC_NAME)
>     endif
>
> where $(CC_NAME) is set through some wonkiness in nss/coreconf/Werror.mk.
>
> The coreconf inside jss does not appear to do this in the same manner.

It turns out we need this extra patch, not for fedora but for macOS. In fedora we can use the cleaner method or the patch or both but on macOS we need them both. Enclosed is a version that applied a bit cleaner using patch < -p1 instead of patch < -p2.

Comment 11

[Elio Maldonado]

How to test- based on advise we got from Kai:
1. Create a working directory and cd there: mkdir work4jss; cd work4jss
2. copy run.sh, bbenv.sh, all.pl, and mhl2.patch there
3. create an hg to clone from hg and go there: mkdir hg; cd hg
4. checkout nspr, nss and jss:
 4a. hg clone https:/hg.mozilla.org/projects/nspr
 4b. hg clone https:/hg.mozilla.org/projects/nss
 4c. hg clone https:/hg.mozilla.org/projects/jss
5. Apply the patches
 5a. cd nss; patch -p1 < ../../mlh2.patch; cd ..
 5b. cd jss; patch -p1 < ../../all.pl.patch; cd ../
6. Now you can build nspr, nss, and jss and run the jss tests
 ./run.sh 1> run.log 2>&1
when done examine run.log and if successful you should at the end
================= Test Results
JSSTEST_SUITE: 28 / 28
JSSTEST_RATE: 100 %
Test Status: SUCCESS

Comment 12

[Elio Maldonado]

I have tested successfully in the following:
i)  Fedora: f24, f25 release candidate, and Rawhide (on f24 did also 32-bit)
ii) RHEL-7: rhel-7.3 with system-wide FIPS enabled and disabled
ii) MacOS latest

Comment 13

[Matthew Harmsen]

Comment on attachment 8811416 [details] [diff] [review]
Removes obsolete bypass tests

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

This patch looks fine to me.

Comment 14

[Matthew Harmsen]

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

This patch looks fine to me.

Comment 15

[Matthew Harmsen]

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

Per discussions on IRC, place a comment above the JAVA_HOME_64 to look in bbenv.sh, and add a comment something like the following to bbenv.sh:

# For example, to check/select your Java version on Linux:
#
# # /usr/sbin/alternatives --config java
#
#  There is 1 program that provides 'java'.
#
#    Selection    Command
#  -----------------------------------------------
#  *+ 1           java-1.8.0-openjdk.x86_64 (/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.111-3.b16.fc24.x86_64/jre/bin/java)
#
#  Enter to keep the current selection[+], or type selection number:

Comment 16

[Matthew Harmsen]

Comment on attachment 8811435 [details] [diff] [review]
Additional patch required by macOS

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

As discussed on IRC, Windows may not accept the "ln -s ${SOURCE} ${TARGET} >/dev/null 2>&1" line.

Additional logic may need to be placed inside this routine to perform a "cp -rp ${SOURCE} ${TARGET} >/dev/null 2>&1" on Windows platforms.

If this is indeed necessary, the name of the routine should possibly be changed from "create_objdir_dist_link" to something like "create_objdir_dist_reference"

Comment 17

[Elio Maldonado]

(In reply to Matthew Harmsen from comment #16)
> Comment on attachment 8811435 [details] [diff] [review]
> Additional patch required by macOS
> 
> Review of attachment 8811435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As discussed on IRC, Windows may not accept the "ln -s ${SOURCE} ${TARGET}
> >/dev/null 2>&1" line.
> 
> Additional logic may need to be placed inside this routine to perform a "cp
> -rp ${SOURCE} ${TARGET} >/dev/null 2>&1" on Windows platforms.
> 
> If this is indeed necessary, the name of the routine should possibly be
> changed from "create_objdir_dist_link" to something like
> "create_objdir_dist_reference"

The additional logic may not be needed after all. Symbolic links have been available in NTFS starting with Windows Vista. see https://bugzilla.mozilla.org/show_bug.cgi?id=1313122#c16 and also https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).asp for the windows versions. The only place where we could not count on NTFS for them would be on environments such as USB sticks.

Comment 18

[Elio Maldonado]

(In reply to Elio Maldonado from comment #17)
Oops. the links I meant to provide are:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365680(v=vs.85).aspx and
https://msdn.microsoft.com/en-us/library/windows/desktop/ms724832(v=vs.85).aspx

Comment 19

[Elio Maldonado]

(In reply to Matthew Harmsen from comment #15)
> Review of attachment 8811434 [details]:
> -----------------------------------------------------------------
Yes, and it should actually be sudo /usr/sbin/alternatives --config java and that requires root privilege, the last line 'export NSS_NO_PKCS11_BYPASS=1' is one we can get rid off and
and MAKE=make is more general.

Comment 20

[Christina Fu]

Comment on attachment 8811416 [details] [diff] [review]
Removes obsolete bypass tests

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

Question: is "bypassoff" the default when you run startJssSelfServ.sh ?
My observation is the following: if startJssSelfServ.sh always requires either "bypassoff" or "bypass", wouldn't by removing ALL tests that contain "bypassoff" or "bypass" effectively remove EVERY test that run with startJssSelf.sh?

A quick search in you all.pl tells me that none of the startJssSelfServ tests are being called (tested) any more.
So, are you sure you are supposed to remove the "bypassoff" tests?  Because that seems to leave nothing left to be run with startJssSelfServ.sh
I could be wrong, but I think there is a likelihood that you should only remove the "bypass" tests.  However, for the "bypassoff" tests, you might want to look into replacing the NSS bypass calls with non-bypass calls? (That's the area I'm not familiar with).  Please investigate and let me know.  Thanks.

Comment 21

[Christina Fu]

One additional comment, which has to do with the sample script bbenv.sh

I had to make two changes to have the tests run successful on my FC24 (thanks to info provided by Matt:
1. Update java-1.8.0-openjdk from java-1.8.0-openjdk-1.8.0.111-1.b16.fc24.x86_64 to java-1.8.0-openjdk-1.8.0.111-3.b16.fc24.x86_64, and then do
/usr/sbin/alternatives --config java
2. change in bbenv.sh the line from 
JAVA_HOME_64=/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.111-3.b16.fc26.x86_64
to
JAVA_HOME_64=/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.111-3.b16.fc24.x86_64

Although you did put in a comment right above the JAVA_HOME_64 line: 
# change to suit your environent (it's misspelled, btw)
I think it might be more useful if there is some sort of a README section at the top of the file instead calling out the possible needed changes involving the two things above.

thanks.

Comment 22

[Elio Maldonado]

(In reply to Christina Fu from comment #20)
> Comment on attachment 8811416 [details] [diff] [review]
> Removes obsolete bypass tests
> 
> Review of attachment 8811416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Question: is "bypassoff" the default when you run startJssSelfServ.sh ?

Yes, bypassoff is the deafult.

> My observation is the following: if startJssSelfServ.sh always requires
> either "bypassoff" or "bypass", wouldn't by removing ALL tests that contain
> "bypassoff" or "bypass" effectively remove EVERY test that run with
> startJssSelf.sh?

Yes, it does.

> 
> A quick search in you all.pl tells me that none of the startJssSelfServ
> tests are being called (tested) any more.
> So, are you sure you are supposed to remove the "bypassoff" tests?  Because
> that seems to leave nothing left to be run with startJssSelfServ.sh
> I could be wrong, but I think there is a likelihood that you should only
> remove the "bypass" tests.  However, for the "bypassoff" tests, you might
> want to look into replacing the NSS bypass calls with non-bypass calls?
> (That's the area I'm not familiar with).  Please investigate and let me
> know.  Thanks.

I tried removing only the bypass on test and that doesn't help. 
With all tests
================= Test Results
JSSTEST_SUITE: 29 / 40
JSSTEST_RATE: 72 %
Test Status: FAILURE
With only byass off
================= Test Results
JSSTEST_SUITE: 29 / 32
JSSTEST_RATE: 91 %
Test Status: FAILURE

I also get a failure that we need understand
SSL Server is invoked using port 8575 
/usr/lib/jvm/java-1.8.0-openjdk-1.8.0.111-3.b16.fc25.x86_64/jre/bin/java -d64 -cp /home/emaldona/matt4/hg/dist/Linux4.8_x86_64_gcc_glibc_PTH_64_DBG.OBJ/../xpclass_dbg.jar org.mozilla.jss.tests.JSS_SelfServClient 2 -1 /home/emaldona/matt4/hg/tests_results/jss/dhcp-16-128.sjc.redhat.com.1 passwords localhost 8575 bypassOff JSSE 
Number of Threads to create: 2
Client connecting to server: localhost:8575
main: jss library loaded
***FilePasswordCallback returns m1oZilla
JSS does not support ciphersuite: 1301
JSS does not support ciphersuite: 1303
JSS does not support ciphersuite: 1302
JSS does not support ciphersuite: cca9
JSS does not support ciphersuite: cca8
JSS does not support ciphersuite: c02c
JSS does not support ciphersuite: c030
JSS does not support ciphersuite: c024
JSS does not support ciphersuite: c028
JSS does not support ciphersuite: ccaa
JSS does not support ciphersuite: a2
JSS does not support ciphersuite: 9f
JSS does not support ciphersuite: a3
JSS does not support ciphersuite: 40
JSS does not support ciphersuite: 6a
JSS does not support ciphersuite: 9d

ERROR: NSS has implemented ciphersuites that JSS does not support!

Need to investigate this.

Let me attach the run log files for both and also the patch to only skip bypass on tests

Comment 23

[Elio Maldonado]

Attachment: log with all tests

Comment 24

[Elio Maldonado]

Attachment: log with bypass_off test disabled

Comment 25

[Elio Maldonado]

Attachment: skips only the bypass on tests

For information purpose, this is what Christina suggested to investigate and doesn't work as you can see the log.

Comment 26

[Elio Maldonado]

(In reply to Elio Maldonado from comment #22)

Regarding this part
> JSS does not support ciphersuite: 1301
> JSS does not support ciphersuite: 1303
> JSS does not support ciphersuite: 1302
> JSS does not support ciphersuite: cca9
> JSS does not support ciphersuite: cca8
> JSS does not support ciphersuite: c02c
> JSS does not support ciphersuite: c030
> JSS does not support ciphersuite: c024
> JSS does not support ciphersuite: c028
> JSS does not support ciphersuite: ccaa
> JSS does not support ciphersuite: a2
> JSS does not support ciphersuite: 9f
> JSS does not support ciphersuite: a3
> JSS does not support ciphersuite: 40
> JSS does not support ciphersuite: 6a
> JSS does not support ciphersuite: 9d
> 
> ERROR: NSS has implemented ciphersuites that JSS does not support!
> 

This are the cipher suites taht JSS does not support
TLS_AES_128_GCM_SHA256                        0x1301
TLS_CHACHA20_POLY1305_SHA256                  0x1303
TLS_AES_256_GCM_SHA384                        0x1302
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 0xCCA9
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256   0xCCA8
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384       0xC02C
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384         0xC030
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384       0xC024
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384         0xC028
LS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256      0xCCAA
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256           0x00A2
TLS_DHE_RSA_WITH_AES_256_GCM_SHA384           0x009F
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384           0x00A3
TLS_DHE_DSS_WITH_AES_128_CBC_SHA256           0x0040
TLS_DHE_DSS_WITH_AES_256_CBC_SHA256           0x006A
TLS_RSA_WITH_AES_256_GCM_SHA384               0x009D

From http://mxr.mozilla.org/nss/source/lib/ssl/sslproto.h

Comment 27

[Elio Maldonado]

Which has moved to https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslproto.h

Comment 28

[Christina Fu]

ok, so looks like by uncommenting out the "bypassoff" in the test commands, the SSL tests are now really executed, and then you ran into cipher suite issues (which has nothing to do with bypass).
Also, I looked in JSS code, and I think all the code that reference bypassPKCS11 needs to be removed.

I am suggesting the following (which I already verbally discussed to Elio):
1. I will file a separate bug against the JSS code to remove the bypassPKCS11 (which I will provide the patch myself)
2. Elio will file a separate bug against SSL testing issue(s), one being the cipher suite issue he reported above (there may be more issues once he gets passed that issue though; we will deal when we get there);  I may have to take this bug myself, but we are not going to make this a blocker.  It will be fixed down the road at some point after the integration is completed
3. Elio will fix THIS bug by doing the following:
   a. Change all.pl so that the test script will test SSL but without the "bypassoff" parameter (leave the "bypass" tests removed as per his original patch)
   b. Clean up test code so that there is no more "bypass" or "bypassoff" related parameter option;  A quick search in jss/org/mozilla/jss/tests found startJssServ.sh startJssSelfServ.sh SSLClientAuth.java JSS_SelfServServer.java all containing the word "bypass"
   c. Test the above changed scripts and code (will hit the SSL cipher suite issue)
   d. comment out (not remove) the ssl related tests (the ones that HAD "bypassoff" before), and add a comment on top to indicate that they are to be uncommented at time when the bug filed in "2" above is fixed (this way, "2" will not become a blocker)

Comment 29

[Elio Maldonado]

Attachment: Builds nspr, nss and jss and runs the jss tests run.sh

I just added a pointer to the README at the top of bbenv.sh that Matt requested.

Comment 30

[Elio Maldonado]

Attachment: bbenv.sh

Add a short README at the top explaining how adpapt JAVA_HOME_64 for the user's own environment per Matt's and Cristina's request's on Comment 15 and Comment 21. Removed the last line with export NSS_NO_PKCS11_BYPASS=1 which is now obsolete.

Comment 31

[Elio Maldonado]

Attachment: removes all bypass references from the tests

Comment 32

[Christina Fu]

Comment on attachment 8816632 [details] [diff] [review]
removes all bypass references from the tests

For the record, I reviewed and tested various versions of this off line with Elio.
I'm sorry that I didn't catch this earlier, but when you remove an argument (bypass) from the usage, shouldn't you shift up all the arguments after?
for example, when you removed arg[6], shouldn't the next length be on 7 and args on 6, and so on?  I think there are 3 places like that.

Comment 33

[Elio Maldonado]

Attachment: removes all bypass references from the tests

corrected the missing stuff that Christina pointed out

Comment 34

[Christina Fu]

Comment on attachment 8816639 [details] [diff] [review]
removes all bypass references from the tests

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

yep. looks good now.

Comment 35

[Elio Maldonado]

pushed: https://hg.mozilla.org/projects/jss/rev/c84d7a7aba8e9afa38e188b26c29a7c20e6328ed

Comment 36

[Elio Maldonado]

Attachment: comments out bypass tests

Comments out, does not remove, the ssl related tests (the ones that HAD "bypassoff" before) and adds a comment on top of each to indicate that they are to be restored when Bug 1321350, to remove all JSS bypassPKCS11 functions, is fixed per request in Comment 28 - item d.

Comment 37

[Christina Fu]

Comment on attachment 8816794 [details] [diff] [review]
comments out bypass tests

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

looks good.  All the original "bypass" tests removed, and the "bypassoff" ssl tests commented out.

Comment 38

[Elio Maldonado]

pushed: https://hg.mozilla.org/projects/jss/rev/7706f6ba5b02367ee58aa60d93530d863f494990

Comment 39

[Elio Maldonado]

pushed: https://hg.mozilla.org/projects/nss/rev/2ed8aef0b3601884752be5768f1626a00d665b66

Comment 40

[Kai Engert [:KaiE:]]

The script you fixed was an automation script.

It should be possible to build JSS directly, and knowledge to build it shouldn't be hidden in an external automation script.

Everything that JSS requires for building, should be contained within JSS' own build scripts or makefiles.

It should be possible to build JSS simply by running "make" inside the jss directory. Currently, it doesn't, it fails to find plarena.h