Changing the crypto tests to do actual asserting #18

patrikcerbak · 2025-01-06T14:51:10Z

Hi!

I analyzed the results from my test project on Hydra and finally added some actual asserting to the crypto tests instead of just logging the results (however, I kept the logging there). CheckAlgorithms.java probably contains the most changes since I added the actual values of algorithms and providers I found when analyzing the results (from RHEL8 and RHEL9).

What do you think? What should I change?

If this is merged, I would first try and test it on my test project, the crypto tests will still remain disabled elsewhere.

judovana · 2025-01-06T15:24:51Z

containersQa/CheckAlgorithms.java


        boolean listItems = !shouldHonorFips.equals("silent-assert");
        boolean honorFipsHere = shouldHonorFips.equals("assert") || shouldHonorFips.equals("true") || shouldHonorFips.equals("silent-assert");

+        if (operatingSystem.equals("el9")) {
+            FIPS_PROVIDERS = EL9_FIPS_PROVIDERS;


this kind of IF is something I would like to avoid.

Maybe the java part can have argument -assertAlgorithmsAgainst -assertProvidersAgainst and the content of those sets in the assert files would be on the caller? (or maybe jsut one file with some ini-like sections for algorithms and providers. maybe with some kind of includes to avoid duplications)

Pre prepared assert files should be part of this repository of course.

The user - main script - will be then just calling this CheckAlgorithms.class program with eg:
-assertAgainst el9fips or -assertAgainst ubuntuGhActions ...

wdyt?

pls, remaind me - the CheckAlgorithms.java have dulicate somwhere?

Ok, it makes sense, I can edit it to function like this.

And yes, CheckAlgorithms.java is also here:
https://github.com/patrikcerbak/jtreg-buffer/blob/main/test/reproducers/checkAlgorithms/CheckAlgorithms.java

I have just realised it is a bit more compelx. It msut ahve two sets: -assertContains and -assertNotcontains . if it would be one -assertFile ,file> with some ini-like structure would be better. eg:

[algorithms-contains] ALG1 ALG2 .. ALGn [algorithms-not-contained] ALG1 ALG2 .. ALGn [providers-contains] prov1 .. provN [providers-not-contained] prov1 .. provN

Whether to spoil it with some includes, am not sure.... wdyt?

Hi, the example file you provided is similar to what I visualized. One config file, which contains all the necessary information (algorithms-contains, algs-not-contains, providers-contains, providers-not-contains).

I think that it will be a cleaner solution (although the config file will be more complex). I think that specifying multiple config files with arguments would be harder.

Superb. TY!

judovana · 2025-01-06T15:25:14Z

containersQa/CheckAlgorithms.java

+            "TLS_DHE_RSA_WITH_AES_256_CBC_SHA", "TLS_DHE_RSA_WITH_AES_128_CBC_SHA",
+            "TLS_RSA_WITH_AES_256_GCM_SHA384", "TLS_RSA_WITH_AES_128_GCM_SHA256",
+            "TLS_RSA_WITH_AES_256_CBC_SHA256", "TLS_RSA_WITH_AES_128_CBC_SHA256",
+            "TLS_RSA_WITH_AES_256_CBC_SHA", "TLS_RSA_WITH_AES_128_CBC_SHA");


This will much nicer (and easy) to put to external config files....

judovana · 2025-01-06T15:26:20Z

containersQa/testlib.bash

@@ -942,21 +942,24 @@ function setupAlgorithmTesting {
  cipherListCode=`cat $LIBCQA_SCRIPT_DIR/CipherList.java`
 }

-function logHostAndContainerCryptoPolicy() {
+function checkHostAndContainerCryptoPolicy() {


I would vote to log as one step, and keep logs, and asser as second step. they can both call same logic, jsut one with asserst on (and the os fs) and second without

I agree with @judovana here. Please log the information so we can audit the tests if we chose in the future.

judovana · 2025-01-06T15:27:00Z

containersQa/700_checkHostAndContainerCryptoPolicy.sh

@@ -19,4 +19,4 @@ parseArguments "$@"
 processArguments
 setup
 setupAlgorithmTesting
-logHostAndContainerCryptoPolicy 2>&1| tee $REPORT_FILE
+checkHostAndContainerCryptoPolicy 2>&1| tee $REPORT_FILE


Maybe kep 702 and 703 as list only, and add 704 and 705 which will do also asserts?

Sure, I can do that.

When I sae the mess in comparison, please rework indeed 700_checkHostAndContainerCryptoPolicy.sh , 702_assertCryptoAlgorithms.sh and 703_assertCryptoProviders.sh as asserting ones, and add 704+5+5 as loging/listing ones

sefroberg · 2025-01-07T21:20:52Z

I am converting this to a draft as I do not want to have this merged before the CPU. But I will take a look at this PR tomorrow for its content.

judovana · 2025-01-08T09:21:13Z

I am converting this to a draft as I do not want to have this merged before the CPU. But I will take a look at this PR tomorrow for its content.

Thanx. Agree. In addition there are huge changes requested anyway.

patrikcerbak · 2025-01-10T12:57:37Z

I am converting this to a draft as I do not want to have this merged before the CPU. But I will take a look at this PR tomorrow for its content.

Thank you, I will let you know when I finish the requested changes.

sefroberg · 2025-01-13T17:24:14Z

containersQa/testlib.bash

  skipIfJreExecution
  set +x
-  commandAlgorithms="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms list algorithms"
+
+  if [ "$OTOOL_OS" == "el.9" ] ; then


I think this would only run on the el9 and not el9z?
This run the wrong code for el9z test runs.
For now I suggest checking for the versions of Rhel.
el8z, el9, el9z, el10 (for the beta runs) else "SKIP"?

The check for rehl Z x Y is nearly impossible. So to pass this from otool_os is probably still encessary. There should be fallback if the variable is not declared. (It may be even exit with "unknown otool_os....".

However Scott is right. Currently I think there is no difference between el9z and el9 crypto setup, but very often there is! So the suite must count with the 9z and 9y having different algorithms provided.

Once you move to the config files, rthis should mget much more easy and celaner, and without duplicated commandAlgorithm setup perhaps?:

if [ "$OTOOL_OS" == "el.9" ] ; then config=el9 elif [ "$OTOOL_OS" == "el.9z" ] ; then config=el9z elif [ "$OTOOL_OS" == "el.8z" -o "$OTOOL_OS" == "el.8" ] ; then config=el8 elif echo "$OTOOL_OS" | grep "win"] ; then config=universalWin else echo "otool os not declard, or unknonw. otool_os os now $otool_os" exit 2 fi commandAlgorithms="echo '$config' > /tmp/$config && echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms assertFile /tmp/$config"

Hoefully the name of the config will not be lost. Otherwise you would need one more section in that confg. some [metadata] ....

sefroberg · 2025-01-13T17:24:46Z

containersQa/testlib.bash

  skipIfJreExecution
  set +x
-  commandProviders="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms list providers"
+
+  if [ "$OTOOL_OS" == "el.9" ] ; then


Same as above regarding the el9/el9z

judovana · 2025-01-14T09:04:46Z

containersQa/testlib.bash

  hostCryptoPolicy=`update-crypto-policies --show`
  containerCryptoPolicy=`runOnBaseDirBash "update-crypto-policies --show"`

  # host is RHEL in FIPS mode, the crypto policies on host and in the container should match
  if [ "$OTOOL_OS_NAME" == "el" ] && [ "$OTOOL_cryptosetup" == "fips" ] ; then
    if [ "$hostCryptoPolicy" == "$containerCryptoPolicy" ] ; then
      echo "Crypto policy on RHEL host is the same as in the container ($containerCryptoPolicy), which was expected."
+      exit


pls be explicit with exit 0 unless it hurts yours code feelings.

judovana · 2025-01-14T09:04:51Z

containersQa/testlib.bash

    fi

  # host is not RHEL in FIPS mode, the crypto policies may or may not match, just logging them
  else
    echo "Crypto policy on host is $hostCryptoPolicy, and in container $containerCryptoPolicy."
+    exit


pls be explicit with exit 0 unless it hurts yours code feelings.

judovana · 2025-01-14T09:04:56Z

containersQa/testlib.bash

@@ -970,37 +973,54 @@ function validateManualSettingFipsWithNoCrash() {
  if [ "$OTOOL_OS_NAME" == "el" ] && [ "$OTOOL_cryptosetup" == "fips" ] ; then
    if [ "$containerReturnCode" != 0 ] ; then
      echo "RHEL host is in FIPS, container returned $containerReturnCode, which was expected."
+      exit


pls be explicit with exit 0 unless it hurts yours code feelings.

judovana · 2025-01-14T09:05:01Z

containersQa/testlib.bash

    fi

  # host is RHEL not in FIPS mode, the command shouldn't fail
  elif [ "$OTOOL_OS_NAME" == "el" ] ; then
    if [ "$containerReturnCode" == 0 ] ; then
      echo "RHEL host is not in FIPS, container returned $containerReturnCode, which was expected."
+      exit


pls be explicit with exit 0 unless it hurts yours code feelings.

changing the crypto tests from just logging to asserting

816f75e

judovana reviewed Jan 6, 2025

View reviewed changes

sefroberg marked this pull request as draft January 7, 2025 21:20

sefroberg requested changes Jan 13, 2025

View reviewed changes

judovana reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changing the crypto tests to do actual asserting #18

Changing the crypto tests to do actual asserting #18

patrikcerbak commented Jan 6, 2025

judovana Jan 6, 2025

patrikcerbak Jan 6, 2025

judovana Jan 8, 2025

patrikcerbak Jan 10, 2025

judovana Jan 10, 2025

judovana Jan 6, 2025

judovana Jan 6, 2025

sefroberg Jan 13, 2025

judovana Jan 6, 2025

patrikcerbak Jan 6, 2025

judovana Jan 10, 2025

sefroberg commented Jan 7, 2025

judovana commented Jan 8, 2025

patrikcerbak commented Jan 10, 2025

sefroberg Jan 13, 2025

judovana Jan 14, 2025

sefroberg Jan 13, 2025

judovana Jan 14, 2025

judovana Jan 14, 2025

judovana Jan 14, 2025

judovana Jan 14, 2025

Changing the crypto tests to do actual asserting #18

Are you sure you want to change the base?

Changing the crypto tests to do actual asserting #18

Conversation

patrikcerbak commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sefroberg commented Jan 7, 2025

judovana commented Jan 8, 2025

patrikcerbak commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment