Bug 1123435 - Flaws in test script and conditional compile for disabling SSL2 and export suites support
Summary: Flaws in test script and conditional compile for disabling SSL2 and export su...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: nss
Version: 7.1
Hardware: Unspecified
OS: Unspecified
high
low
Target Milestone: rc
: ---
Assignee: Elio Maldonado Batiz
QA Contact: Hubert Kario
URL:
Whiteboard:
: 1204404 (view as bug list)
Depends On:
Blocks: 1245627 1263005
TreeView+ depends on / blocked
 
Reported: 2014-07-25 15:48 UTC by Elio Maldonado Batiz
Modified: 2015-11-19 12:24 UTC (History)
6 users (show)

Fixed In Version: nss-3.19.1-9.el7
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 1263005 (view as bug list)
Environment:
Last Closed: 2015-11-19 12:24:48 UTC


Attachments (Terms of Use)
All changes needed to fix the flaws (11.08 KB, patch)
2014-07-25 23:14 UTC, Elio Maldonado Batiz
rrelyea: review+
Details | Diff
fix disabling of ssl2/export support - conditional compilation part (3.28 KB, patch)
2014-07-25 23:19 UTC, Elio Maldonado Batiz
rrelyea: review+
Details | Diff
fix skipping of ssl2/export tests that arent supported - testscripts part (4.76 KB, patch)
2014-07-25 23:21 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
removal from to sslcov.txt of SSL2 and export tests cases (4.27 KB, patch)
2014-07-29 20:35 UTC, Elio Maldonado Batiz
rrelyea: review+
Details | Diff
removal from sslstress.txt of SSL2 and export tests cases (3.36 KB, patch)
2014-07-29 20:36 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
Fix skipping of ssl2 & export test which aren't supprted - test scripts part (14.87 KB, patch)
2014-07-29 20:42 UTC, Elio Maldonado Batiz
rrelyea: review-
Details | Diff
changes to nss.spc file in patch format (1.16 KB, patch)
2014-07-29 20:43 UTC, Elio Maldonado Batiz
rrelyea: review+
Details | Diff
Fix skipping of ssl2 & export test which aren't supported - test scripts part - V3 (12.58 KB, patch)
2014-07-30 16:12 UTC, Elio Maldonado Batiz
no flags Details | Diff
Fix skip of unsupprted ssl2/export tests - test scripts part - V4 (14.67 KB, patch)
2014-07-30 18:43 UTC, Elio Maldonado Batiz
no flags Details | Diff
Fix disabling: test scripts part - V5 - just remove ths ssl2 tests (14.66 KB, patch)
2014-07-30 20:36 UTC, Elio Maldonado Batiz
no flags Details | Diff
fix skipping of ssl2/export tests that aren't supported - testscripts part V6 (14.66 KB, patch)
2014-07-31 15:20 UTC, Elio Maldonado Batiz
rrelyea: review+
Details | Diff
fixes most of the shell syntax errors (5.53 KB, patch)
2015-02-01 22:16 UTC, Elio Maldonado Batiz
no flags Details | Diff
fixes all reported shell syntax errors (5.56 KB, patch)
2015-02-06 17:28 UTC, Elio Maldonado Batiz
no flags Details | Diff
fixes all reported shell syntax errors and more (7.41 KB, patch)
2015-02-08 01:22 UTC, Elio Maldonado Batiz
emaldona: review-
Details | Diff
Fix checks for SSL2 and EXPORT ciphers suite tests (2.55 KB, patch)
2015-08-02 16:26 UTC, Elio Maldonado Batiz
no flags Details | Diff
Fix checks for SSL2 and EXPORT cipher suites tests (1.49 KB, patch)
2015-08-05 16:44 UTC, Elio Maldonado Batiz
kengert: review-
Details | Diff
Fix checks for SSL2 and EXPORT cipher suites tests (1.52 KB, patch)
2015-08-10 22:14 UTC, Elio Maldonado Batiz
no flags Details | Diff
Fix checks for SSL2 and EXPORT cipher suites tests - not for checkin (3.21 KB, patch)
2015-08-10 22:20 UTC, Elio Maldonado Batiz
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2015:2121 normal SHIPPED_LIVE nss bug fix and enhancement update 2015-11-19 11:01:16 UTC
Mozilla Foundation 1128367 None None None 2019-05-16 08:39:09 UTC

Description Elio Maldonado Batiz 2014-07-25 15:48:50 UTC
Description of problem: A syntax error in the test to skip tests for SSL2 and export suites which are no longer supported. 

Version-Release number of selected component (if applicable): nss-3.15.4-7.el7


How reproducible: Always


Steps to Reproduce:
1. Checkout the sources and run rhpkg prep
2. cd  ./nss-3.15.4/nss/tests/ssl and run
3. $ sh -e ssl.sh

Actual results: ssl.sh: line 356: syntax error near unexpected token `done'
Expected results: No ouput

Additional info: The faulty line is found via
$ grep "NSS_NO_SSL2" ssl.sh
      if [ "${NSS_NO_SSL2}" = 1 ] && [ -n ${EXP} -o -n ${SSL2} ] && continue

This should be
      [ "${NSS_NO_SSL2}" = 1 ] && [ -n ${EXP} -o -n ${SSL2} ] && continue


In Bug-1001841-disable-sslv2-tests.patch this are the faulty lines
+
+      #  skip export and ssl2 tests when build has disabled SSL2
+      if [ "${NSS_NO_SSL2}" = 1 ] && [ -n ${EXP} -o -n ${SSL2} ] && continue

The problem remains latest build for rhel-7.1.

Comment 1 Elio Maldonado Batiz 2014-07-25 16:42:37 UTC
When I fix this and rebuild build as several of the test fails. 

This is not surprising because the patch is skipping tests for unsupported features and we must take into account the the servers are not going to start in some cases. 

I fix the Bug-1001841-disable-sslv2-tests.patch by adding these lines:
diff --git a/tests/chains/chains.sh b/tests/chains/chains.sh
--- a/tests/chains/chains.sh
+++ b/tests/chains/chains.sh
@@ -35,17 +35,21 @@ is_httpserv_alive()
   if [ "${OS_ARCH}" = "WINNT" ] && \
      [ "$OS_NAME" = "CYGWIN_NT" -o "$OS_NAME" = "MINGW32_NT" ]; then
       PID=${SHELL_HTTPPID}
   else
       PID=`cat ${HTTPPID}`
   fi
 
   echo "kill -0 ${PID} >/dev/null 2>/dev/null" 
+  if [ "${NSS_NO_SSL2}" = "1" ]; then
+  echo "skipping kill because NSS_NO_SSL=${NSS_NO_SSL2}"
+  else
   kill -0 ${PID} >/dev/null 2>/dev/null || Exit 10 "Fatal - httpserv process not detectable"
+  fi
 
   echo "httpserv with PID ${PID} found at `date`"
 }
 
 ########################### wait_for_httpserv ##########################
 # local shell function to wait until httpserver is running and initialized
 ########################################################################
 wait_for_httpserv()
@@ -54,17 +58,21 @@ wait_for_httpserv()
   echo "tstclnt -p ${NSS_AIA_PORT} -h ${HOSTADDR} -q -v"
   ${BINDIR}/tstclnt -p ${NSS_AIA_PORT} -h ${HOSTADDR} -q -v
   if [ $? -ne 0 ]; then
       sleep 5
       echo "retrying to connect to httpserv at `date`"
       echo "tstclnt -p ${NSS_AIA_PORT} -h ${HOSTADDR} -q -v"
       ${BINDIR}/tstclnt -p ${NSS_AIA_PORT} -h ${HOSTADDR} -q -v
       if [ $? -ne 0 ]; then
-          html_failed "Waiting for Server"
+          if [ "${NSS_NO_SSL2}" = 1 ]; then
+              html_passed "Waiting for Server is supposed to fail"
+          else
+              html_failed "Waiting for Server"
+          fi
       fi
   fi
   is_httpserv_alive
 }
 
 ########################### kill_httpserv ##############################
 # local shell function to kill the httpserver after the tests are done
 ########################################################################
@@ -1174,17 +1182,21 @@ parse_config()
             ;;
         "break")
             break
             ;;
         "check_ocsp")
             TESTNAME="Test that OCSP server is reachable"
             check_ocsp ${VALUE}
             if [ $? -ne 0 ]; then
+                if [ "${NSS_NO_SSL2}" = "1" ]; then
+                html_passed "$TESTNAME"
+                else
                 html_failed "$TESTNAME"
+                fi
                 break;
             else
                 html_passed "$TESTNAME"
             fi
             ;;
         "ku")
             EXT_KU="${VALUE}"
             ;;
diff --git a/tests/ssl/ssl.sh b/tests/ssl/ssl.sh
--- a/tests/ssl/ssl.sh
+++ b/tests/ssl/ssl.sh
@@ -115,17 +115,21 @@ is_selfserv_alive()
   if [ "${OS_ARCH}" = "WINNT" ] && \
      [ "$OS_NAME" = "CYGWIN_NT" -o "$OS_NAME" = "MINGW32_NT" ]; then
       PID=${SHELL_SERVERPID}
   else
       PID=`cat ${SERVERPID}`
   fi
 
   echo "kill -0 ${PID} >/dev/null 2>/dev/null" 
+  if [ "${NSS_NO_SSL2}" = "1" ]; then
+  ;
+  else
   kill -0 ${PID} >/dev/null 2>/dev/null || Exit 10 "Fatal - selfserv process not detectable"
+  fi
 
   echo "selfserv with PID ${PID} found at `date`"
 }
 
 ########################### wait_for_selfserv ##########################
 # local shell function to wait until selfserver is running and initialized
 ########################################################################
 wait_for_selfserv()
@@ -138,17 +142,21 @@ wait_for_selfserv()
   if [ $? -ne 0 ]; then
       sleep 5
       echo "retrying to connect to selfserv at `date`"
       echo "tstclnt -p ${PORT} -h ${HOSTADDR} ${CLIENT_OPTIONS} -q \\"
       echo "        -d ${P_R_CLIENTDIR} -v < ${REQUEST_FILE}"
       ${BINDIR}/tstclnt -p ${PORT} -h ${HOSTADDR} ${CLIENT_OPTIONS} -q \
               -d ${P_R_CLIENTDIR} -v < ${REQUEST_FILE}
       if [ $? -ne 0 ]; then
+          if [ "${NSS_NO_SSL2}" = "1" ]; then
+              html_passed "Server never started"
+          else
           html_failed "Waiting for Server"
+          fi
       fi
   fi
   is_selfserv_alive
 }
 
and try again. An the test still fail. 

Let's look at Bug-1001841-disable-sslv2-libssl.patch patch but the done with diff -up ./nss/lib/ssl/sslsock.c.disableSSL2 ./nss/lib/ssl/sslsock.c doesn't provide enough context. 
Better do diff -C 8 sslsock.c.disableSSL2 sslsock.c which shows:

        case SSL_ENABLE_SSL2:
+ #ifdef NSS_NO_SSL2
+ if (on) {
+             PORT_SetError(SSL_ERROR_SSL2_DISABLED);
+             rv = SECFailure; /* not allowed */
+         }
+         break;
+         ss->opt.enableSSL2      = on;
+ #else
          if (IS_DTLS(ss)) {
              if (on) {
                  PORT_SetError(SEC_ERROR_INVALID_ARGS);
                  rv = SECFailure; /* not allowed */
              }
              break;
          }
          ss->opt.enableSSL2       = on;
          if (on) {
              ss->opt.v2CompatibleHello = on;
          }
+ #endif
          ss->preferredCipher     = NULL;
          if (ss->cipherSpecs) {
              PORT_Free(ss->cipherSpecs);
              ss->cipherSpecs     = NULL;
              ss->sizeCipherSpecs = 0;
          }
          break;
  
This doesn't like quite right. I think we should instead have something like
....

          break;
  
        case SSL_ENABLE_SSL2:
+ #ifdef NSS_NO_SSL2
+         if (on) {
+             PORT_SetError(SSL_ERROR_SSL2_DISABLED);
+             rv = SECFailure; /* not allowed */
+         }
+ #else
          if (IS_DTLS(ss)) {
              if (on) {
                  PORT_SetError(SEC_ERROR_INVALID_ARGS);
                  rv = SECFailure; /* not allowed */
              }
              break;
          }
          ss->opt.enableSSL2       = on;
          if (on) {
              ss->opt.v2CompatibleHello = on;
          }
          ss->preferredCipher     = NULL;
          if (ss->cipherSpecs) {
              PORT_Free(ss->cipherSpecs);
              ss->cipherSpecs     = NULL;
              ss->sizeCipherSpecs = 0;
          }
+ #endif /* NSS_NO_SSL2 */
          break;

Comment 2 Elio Maldonado Batiz 2014-07-25 23:14:38 UTC
Created attachment 921077 [details]
All changes needed to fix the flaws

This is a bit hard on the eyes as it has differences between version of patches so let me attach the individual patches next.

Comment 3 Elio Maldonado Batiz 2014-07-25 23:19:06 UTC
Created attachment 921080 [details]
fix disabling of ssl2/export support - conditional compilation part

Comment 4 Elio Maldonado Batiz 2014-07-25 23:21:28 UTC
Created attachment 921082 [details]
fix skipping of ssl2/export tests that arent supported - testscripts part

Comment 5 Elio Maldonado Batiz 2014-07-25 23:28:39 UTC
If anyone is interested, a scratch build with these changes applied can be found at https://brewweb.devel.redhat.com/taskinfo?taskID=7738232
Being scratch it gets garbage collected in a week or so.

Comment 6 Bob Relyea 2014-07-26 00:13:31 UTC
Comment on attachment 921077 [details]
All changes needed to fix the flaws

r+ for this set. NOTE: I think it's confusing having 2 patches with the same .suffices, I would prefer if you distiguished them.

NOTE2: While what I can see in this patch is fine, the disable tests code is NOT fine. I don't know if it's an existing problem, but none of those show up in this patch here even though it sall all changes.

Comment 7 Bob Relyea 2014-07-26 00:18:41 UTC
Comment on attachment 921082 [details]
fix skipping of ssl2/export tests that arent supported - testscripts part

r- This doesn't turn off SSL2 ciphers, it ignores all problems in all ciphers if SSL2 is turned off.

What you want is to create new versions of test/ssl/sslcov.txt and test/ssl/sslstress.txt which do not have the SSL2 tests, and then switch to those either unconditionally or in the script.

bob

Comment 8 Bob Relyea 2014-07-26 00:19:32 UTC
Comment on attachment 921080 [details]
fix disabling of ssl2/export support - conditional compilation part

r+ rrelyea

Comment 10 Elio Maldonado Batiz 2014-07-29 20:35:12 UTC
Created attachment 922338 [details]
removal from to sslcov.txt of SSL2 and export tests cases

Comment 11 Elio Maldonado Batiz 2014-07-29 20:36:38 UTC
Created attachment 922339 [details]
removal from sslstress.txt of SSL2 and export tests cases

Comment 12 Elio Maldonado Batiz 2014-07-29 20:42:06 UTC
Created attachment 922341 [details]
Fix skipping of ssl2 & export test which aren't supprted - test scripts part

Comment 13 Elio Maldonado Batiz 2014-07-29 20:43:59 UTC
Created attachment 922344 [details]
changes to nss.spc file in patch format

With different suffixes for the libssl and test parts as requested.

Comment 14 Elio Maldonado Batiz 2014-07-29 20:51:28 UTC
Comment on attachment 922341 [details]
Fix skipping of ssl2 & export test which aren't supprted - test scripts part

(In reply to Bob Relyea from comment #7)
> Comment on attachment 921082 [details]

> What you want is to create new versions of test/ssl/sslcov.txt and
> test/ssl/sslstress.txt which do not have the SSL2 tests, and then switch to
> those either unconditionally or in the script.
> 
I opted to add new versions of test/ssl/sslcov.txt and test/ssl/sslstress.txt 
and in the script checks NSS_NO_SSL2 to switch to the new ones, otherwise the originals are used.

Comment 15 Bob Relyea 2014-07-30 00:22:27 UTC
Comment on attachment 922338 [details]
removal from to sslcov.txt of SSL2 and export tests cases

r+

Comment 16 Bob Relyea 2014-07-30 00:24:04 UTC
Comment on attachment 922341 [details]
Fix skipping of ssl2 & export test which aren't supprted - test scripts part

r- You'll need to explain to me why these are needed. The still look like disabling generic ssl tests.

Comment 17 Bob Relyea 2014-07-30 00:24:51 UTC
Comment on attachment 922344 [details]
changes to nss.spc file in patch format

r+ rrelyea

Comment 18 Bob Relyea 2014-07-30 00:27:42 UTC
Comment on attachment 922339 [details]
removal from sslstress.txt of SSL2 and export tests cases

r- You are diabling to many tests here. There is only 1 SSL2 test and no export tests in this suite. If you are turning off MD5, please change them to SHA1 then.

bob

Comment 19 Elio Maldonado Batiz 2014-07-30 16:06:50 UTC
(In reply to Bob Relyea from comment #16)
> Comment on attachment 922341 [details]
> Fix skipping of ssl2 & export test which aren't supprted - test scripts part
> 
> r- You'll need to explain to me why these are needed. The still look like
> disabling generic ssl tests.

Are you suggesting 
-   noECC     0      _         -c_1000_-C_A                  Stress SSL2 RC4 128 with MD5

with
-   noECC     0      _         -c_1000_-C_A                  Stress SSL2 RC4 128 with MD5
+   noECC     0      _         -c_1000_-C_A                  Stress SSL2 RC4 128 with SHA1

and so on? I dout it.

(In reply to Bob Relyea from comment #18)
> Comment on attachment 922339 [details]
> removal from sslstress.txt of SSL2 and export tests cases
> 
> r- You are diabling to many tests here. There is only 1 SSL2 test and no
> export tests in this suite. If you are turning off MD5, please change them
> to SHA1 then.
> 
> bob

Not sure if I fully unedestand this part but I think you are right. 
1) The changes to tests/chains/chains.sh, aren't needed at all. 
is_httpserv_alive(), wait_for_httpserv(), and kill server or not need to go away. I removed them and it builds just fine.  

2) I do need this part 
diff --git a/tests/ssl/ssl.sh b/tests/ssl/ssl.sh
--- a/tests/ssl/ssl.sh
+++ b/tests/ssl/ssl.sh
@@ -57,18 +57,23 @@ ssl_init()
   fi
 
   PORT=${PORT-8443}
   NSS_SSL_TESTS=${NSS_SSL_TESTS:-normal_normal}
   nss_ssl_run="stapling cov auth stress"
   NSS_SSL_RUN=${NSS_SSL_RUN:-$nss_ssl_run}
 
   # Test case files
-  SSLCOV=${QADIR}/ssl/sslcov.txt
+  SSLCOV=[ "${NSS_NO_SSL2}" = "1" ] \
+    && ${QADIR}/ssl/sslcov.txt.disableSSL2test \
+    || ${QADIR}/ssl/sslcov.txt
   SSLAUTH=${QADIR}/ssl/sslauth.txt
+  SSLSTRESS=[ "${NSS_NO_SSL2}" = "1" ] \
+    && ${QADIR}/ssl/sslstress.txt.disableSSL2test \
+    || ${QADIR}/ssl/sslstress.txt
   SSLSTRESS=${QADIR}/ssl/sslstress.txt
   REQUEST_FILE=${QADIR}/ssl/sslreq.dat
 
2) On /tests/ssl/ssl.sh b/tests/ssl/ssl.sh we need that part in
 ###################### wait_for_selfserv #######################
and  
 ########################### kill_selfserv #################

but the tests the script does should be changed from
+  if [ "${NSS_NO_SSL2}" = "1" ]; then
...
to 
+  if [ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ]; then
...
just like further down in loop where skip
+      #  skip export and ssl2 tests when build has disabled SSL2
+      [ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ] && continue
+
I hope I have understood you correctly. I new patch is comming.

Comment 20 Elio Maldonado Batiz 2014-07-30 16:12:17 UTC
Created attachment 922640 [details]
Fix skipping of ssl2 & export test which aren't supported - test scripts part - V3

Comment 21 Bob Relyea 2014-07-30 16:51:39 UTC
RE: the SSL stress test patch.

*** sslstress.txt	2014-06-24 13:45:27.000000000 -0700
--- sslstress.txt.disableSSL2test	2014-07-29 13:15:57.463569838 -0700
***************
*** 3,41 ****
  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
  #
  # This file defines the stress tests for SSL/TLS.
  #
  #        expected
  # Enable  return  server     client                         Test Case name
  #  ECC    value   params     params
  # ------- ------  ------     ------                         ---------------
-   noECC     0      _         -c_1000_-C_A                  Stress SSL2 RC4 128 with MD5
-   noECC     0      _         -c_1000_-C_c_-V_:ssl3               Stress SSL3 RC4 128 with MD5
-   noECC     0      _         -c_1000_-C_c                  Stress TLS  RC4 128 with MD5
-   noECC     0      _         -c_1000_-C_c_-g               Stress TLS  RC4 128 with MD5 (false start)
-   noECC     0      -u        -V_ssl3:_-c_1000_-C_c_-u            Stress TLS  RC4 128 with MD5 (session ticket)
-   noECC     0      -z        -V_ssl3:_-c_1000_-C_c_-z            Stress TLS  RC4 128 with MD5 (compression)
-   noECC     0      -u_-z     -V_ssl3:_-c_1000_-C_c_-u_-z         Stress TLS  RC4 128 with MD5 (session ticket, compression)
-   noECC     0      -u_-z     -V_ssl3:_-c_1000_-C_c_-u_-z_-g      Stress TLS  RC4 128 with MD5 (session ticket, compression, false start)
-   SNI       0      -u_-a_Host-sni.Dom -V_tls1.0:_-c_1000_-C_c_-u Stress TLS RC4 128 with MD5 (session ticket, SNI)
  
  #
  # add client auth versions here...
  #
-   noECC     0      -r_-r     -c_100_-C_A_-N_-n_TestUser    Stress SSL2 RC4 128 with MD5 (no reuse, client auth)
-   noECC     0      -r_-r     -c_100_-C_c_-V_:ssl3_-N_-n_TestUser Stress SSL3 RC4 128 with MD5 (no reuse, client auth)
-   noECC     0      -r_-r     -c_100_-C_c_-N_-n_TestUser    Stress TLS RC4 128 with MD5 (no reuse, client auth)
-   noECC     0      -r_-r_-u  -V_ssl3:_-c_100_-C_c_-n_TestUser_-u Stress TLS RC4 128 with MD5 (session ticket, client auth)
-   noECC     0      -r_-r_-z  -V_ssl3:_-c_100_-C_c_-n_TestUser_-z Stress TLS RC4 128 with MD5 (compression, client auth)
-   noECC     0      -r_-r_-z  -V_ssl3:_-c_100_-C_c_-n_TestUser_-z_-g Stress TLS RC4 128 with MD5 (compression, client auth, false start)
-   noECC     0   -r_-r_-u_-z  -V_ssl3:_-c_100_-C_c_-n_TestUser_-u_-z Stress TLS RC4 128 with MD5 (session ticket, compression, client auth)
-   noECC     0   -r_-r_-u_-z  -V_ssl3:_-c_100_-C_c_-n_TestUser_-u_-z_-g Stress TLS RC4 128 with MD5 (session ticket, compression, client auth, false start)
-   SNI       0   -r_-r_-u_-a_Host-sni.Dom -V_tls1.0:_-c_1000_-C_c_-u Stress TLS RC4 128 with MD5 (session ticket, SNI, client auth, default virt host)
-   SNI       0   -r_-r_-u_-a_Host-sni.Dom_-k_Host-sni.Dom -V_tls1.0:_-c_1000_-C_c_-u_-a_Host-sni.Dom Stress TLS RC4 128 with MD5 (session ticket, SNI, client auth, change virt host)
  

Only one of those tests above is SSL2.  I don't understand way you are removing SSL3 and TLS ciphers, and my only guess was you were trying to remove MD5. Removing the single SSL2 test above is fine, removing the large number on unrelated tests is not.

Comment 22 Bob Relyea 2014-07-30 16:57:13 UTC
> 2) I do need this part 

Agreed, this part is fine. The other part of the chain.sh test looked like it was silencing all test errors.

> 2) On /tests/ssl/ssl.sh b/tests/ssl/ssl.sh we need that part in
>  ###################### wait_for_selfserv #######################
> and  
>  ########################### kill_selfserv #################

These changes are acceptable, thought I'd like to understand why they are needed now that you've removed the ssl2 tests from the source file (at least you aren't silencing non-ssl2 tests anymore).

bob

Comment 23 Elio Maldonado Batiz 2014-07-30 18:37:26 UTC
(In reply to Bob Relyea from comment #21)
.....
> Only one of those tests above is SSL2.  I don't understand way you are
> removing SSL3 and TLS ciphers, and my only guess was you were trying to
> remove MD5. Removing the single SSL2 test above is fine, removing the large
> number on unrelated tests is not.

Yes, I was trying to remove MD5. I did a build removing only SSL2 and it works just fine. I did another build where I changed MD5 to SHA1 in the stress test, as I think you suggested earlier, and that works fine. If I'm going to stress testing I better do it with SHA1 instead of with MD5. Version 4 coming next.

Comment 24 Elio Maldonado Batiz 2014-07-30 18:43:21 UTC
Created attachment 922673 [details]
Fix skip of unsupprted ssl2/export tests - test scripts part - V4

Comment 25 Bob Relyea 2014-07-30 20:05:27 UTC
> Yes, I was trying to remove MD5. I did a build removing only SSL2 and it
> works just fine.

For this bug why don't we just remove ths ssl2 tests. We haven't turned off all uses of MD5, only MD5 used in signatures, so doing so isn't related to this bug.

Comment 26 Elio Maldonado Batiz 2014-07-30 20:36:29 UTC
Created attachment 922728 [details]
Fix disabling: test scripts part - V5 - just remove ths ssl2 tests

Comment 27 Elio Maldonado Batiz 2014-07-31 15:20:17 UTC
Created attachment 922970 [details]
fix skipping of ssl2/export tests that aren't supported - testscripts part V6

Comment 28 Bob Relyea 2014-08-01 18:21:03 UTC
Comment on attachment 922970 [details]
fix skipping of ssl2/export tests that aren't supported - testscripts part V6

r+ rrelyea

Comment 34 Elio Maldonado Batiz 2015-01-29 16:44:05 UTC
Created attachment 985681 [details]
fix shell syntax errors ssl2/export test skipping patch and dbtest patch

Comment 41 Elio Maldonado Batiz 2015-02-01 22:16:54 UTC
Created attachment 986851 [details]
fixes most of the shell syntax errors

A got a scratch build to complete with this patch but I had to disable the fix for cipher.sh. It's a work in progress. I just wanted to save the work done so far so there is no need to review it. By the time we start working on rhel-7.2 a lot will have changed.

Comment 42 Elio Maldonado Batiz 2015-02-06 17:28:13 UTC
Created attachment 988982 [details]
fixes all reported shell syntax errors

Comment 43 Elio Maldonado Batiz 2015-02-08 01:22:22 UTC
Created attachment 989305 [details]
fixes all reported shell syntax errors and more

Comment 49 Elio Maldonado Batiz 2015-08-02 16:13:47 UTC
Comment on attachment 989305 [details]
fixes all reported shell syntax errors and more

One flaw (among others)
+  if [[ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ]]; then
First mistake is the double brackets "[[" and "]]" but even with single brakes
+  if [ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ]; then
is still is wrong. -n is a string comparison. -n is true if it's nonzero length, the integer is always a nonzero string which means, the expression is always true
Thanks to Kai that pointed it out in the context of another bug investigation and another part of the ssl.sh script. More analysis and fix proposals to come.

Comment 50 Elio Maldonado Batiz 2015-08-02 16:26:24 UTC
Created attachment 1058571 [details]
Fix checks for SSL2 and EXPORT ciphers suite tests

Patch provided by Kaie adds a lot of of tracing and fixes some serious errors
in the way I check in the ssl test suite script for SSL2 and Export cipher suites. Test cases when SSL2/EXPORT is disabled as has been the case since rhel-7.0.

Recently a regression was reported, that the NULL cipher suites are broken on RHEL 7. Kai thoroughly analyzed the problem and traced the error to bad commits I made. I'll copy from what he told me that's right on spot.
 
My first commit on 2014-07-10, which introduced the error
 +      #  skip export and ssl2 tests when build has disabled SSL2
-+      if [ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ]; then <-1
-+          echo "SKIPPED"
-+          continue
-+      fi
++      if [ "${NSS_NO_SSL2}" = 1 ] && [ -n ${EXP} -o -n ${SSL2} ] && continue
 +
        if [ "${SSL2}" -eq 0 ] ; then  <- 2

The first check is in error, see below for why.
------------------------------------------------
Another commit, on 2014-08-01, changed style but kept the error in the check.
 +      #  skip export and ssl2 tests when build has disabled SSL2
-+      if [ "${NSS_NO_SSL2}" = 1 ] && [ -n ${EXP} -o -n ${SSL2} ] && continue
++      [ "${NSS_NO_SSL2}" = "1" ] && [ -n ${EXP} -o -n ${SSL2} ] && continue
 +
        if [ "${SSL2}" -eq 0 ] ; then
            # We cannot use asynchronous cert verification with SSL2
            SSL2_FLAGS=-O
+           VMIN="ssl2"

Kai states: as you can see below the line that you had added, the correct way to check the value of those variables is with the arithmetic comparison operators there is an existing line:
         if [ "${SSL2}" -eq 0 ] ; then
 this one does it right
 But exactly one line above it, you added your new test with -n
 -n is a string comparison. -n is true if it's nonzero length. The integer is always a nonzero string. That means, the expression is always true, and all ciphers go to continue  and are skipped
 I think you should have checked the logs after you added this new code to ensure that these tests are still being executed for the remaining ciphers.
 Today we got a bug report, a regression, that the NULL ciphersuites are broken on RHEL 7"

The reference is to Bug 1245627 (TLS_RSA_WITH_NULL_SHA256 nor working in RHEL7)
I'm currently working on a patch for that bug which calls for enabling some additional cipher suites by default and fixing this one is required in order to properly verify the fix for that other one.

Kai's patch as it is fixes the errors. It may need some minor changes.

Comment 51 Elio Maldonado Batiz 2015-08-05 16:39:32 UTC
Received very good advise on an email discussion I had with Kai and Hubert. some snippets from Hubert's email.
.....
 if [ "${NSS_NO_SSL2}" = "1" ] && [ "${EXP}" -eq 1 -o "${SSL2}" -eq 1 ]; then
  continue
 fi

Though we will have problems if the variables are empty:
  EXP=
  [ "${EXP}" -eq 1 ]
bash: [: : integer expression expected

For that
 if [ "${NSS_NO_SSL2}" = "1" ] && [[ ${EXP} -eq 1 || ${SSL2} -eq 1 ]]; then
  continue
 fi

would be better.

That is because [] uses the POSIX test(1) syntax, while [[]] is the bash
extension (see bash(1), look for '\[\[ expression \]\]' or 'CONDITIONAL
EXPRESSIONS'), where the shell knows that it is comparing variables, not
parameters.

I tested this in various ways and with additional logs and it solves the outstanding issues. I'll submit a new patch next, the copious, yet useful, tracing has been reduced to a bare minimum.

Comment 52 Elio Maldonado Batiz 2015-08-05 16:44:37 UTC
Created attachment 1059558 [details]
Fix checks for SSL2 and EXPORT cipher suites tests

Comment 53 Kai Engert (:kaie) (inactive account) 2015-08-05 17:15:59 UTC
Elio, I used the "yyy" output markers only for debugging purposes. I think they shouldn't be contained in the production patch (if you want debug output, use something better than yyy).

You've asked for a review. I have to say that it's easy to confuse these statements. I wouldn't trust my own review without having actually tested it. My suggestion is that you test it and inspect the output to ensure that it works as expected.

Comment 54 Kai Engert (:kaie) (inactive account) 2015-08-05 17:42:48 UTC
I think you should test the patch prior to asking for review.

I would like to suggest that you use the identical expression in all three places you're changing.

Please remove the "yyy" from the test output, you can keep the rest.

Looking at the code, you have changed my "-eq 0" to "-eq 1", which is wrong.

You want to skip (continue) if at least one value was found.

grep returns 0 for "found".

Comment 55 Elio Maldonado Batiz 2015-08-10 22:14:52 UTC
Created attachment 1061267 [details]
Fix checks for SSL2 and EXPORT cipher suites tests

This not for review yet. The three checks are consistent now are compare against 0. One tracing echo line remains as I'm not done yet with the testing. 

If should mention that along with this patch I'm also testing using one that implements your recommendation from 
https://bugzilla.redhat.com/show_bug.cgi?id=1245627#c15 and takes care of all many failures (the ..._RSA_WITH_NULL_ ... ones), there are others. More details coming later to that other bug where the topic properly belongs.

Comment 56 Elio Maldonado Batiz 2015-08-10 22:20:09 UTC
Created attachment 1061281 [details]
Fix checks for SSL2 and EXPORT cipher suites tests - not for checkin

This is not ready for review is merly for information. One tracing line remains as I am still testing and the testing includes a fix for the bug this one blocks. I'll provide details in that other bug were the topic properly belongs

Comment 57 Nathan Kinder 2015-09-02 19:26:14 UTC
*** Bug 1204404 has been marked as a duplicate of this bug. ***

Comment 59 errata-xmlrpc 2015-11-19 12:24:48 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2015-2121.html


Note You need to log in before you can comment on or make changes to this bug.