Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing the crypto tests to do actual asserting #18

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Changing the crypto tests to do actual asserting #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...Qa/700_logHostAndContainerCryptoPolicy.sh → .../700_checkHostAndContainerCryptoPolicy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ parseArguments "$@"
processArguments
setup
setupAlgorithmTesting
logHostAndContainerCryptoPolicy 2>&1| tee $REPORT_FILE
checkHostAndContainerCryptoPolicy 2>&1| tee $REPORT_FILE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

2 changes: 1 addition & 1 deletion containersQa/703_listCryptoProviders.sh → containersQa/702_assertCryptoAlgorithms.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ parseArguments "$@"
processArguments
setup
setupAlgorithmTesting
listCryptoProviders
assertCryptoAlgorithms
2 changes: 1 addition & 1 deletion containersQa/702_listCryptoAlgorithms.sh → containersQa/703_assertCryptoProviders.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@ parseArguments "$@"
processArguments
setup
setupAlgorithmTesting
listCryptoAlgorithms
assertCryptoProviders
65 changes: 57 additions & 8 deletions containersQa/CheckAlgorithms.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,35 @@
import java.util.List;

public class CheckAlgorithms {
// for future proofing, any time the algorithms change, just editing these hardcoded lists is enough:
public static final List<String> FIPS_PROVIDERS = Arrays.asList("SunPKCS11-NSS-FIPS");
public static final List<String> NONFIPS_PROVIDERS = Arrays.asList("SunPCSC");
public static final List<String> FIPS_ALGORITHMS = Arrays.asList();
public static final List<String> NONFIPS_ALGORITHMS = Arrays.asList("TLS_RSA_WITH_AES_128_CBC_SHA");

private static final List<String> possibleFirstArgs = Arrays.asList("assert", "true", "silent-assert", "list", "false");
private static final List<String> possibleSecondArgs = Arrays.asList("algorithms", "providers", "both");
private static final List<String> possibleThirdArgs = Arrays.asList("el8", "el9");

// general purpose variables used later
public static List<String> FIPS_PROVIDERS;
public static List<String> NONFIPS_PROVIDERS;
public static List<String> FIPS_ALGORITHMS;
public static List<String> NONFIPS_ALGORITHMS;

public static void main(String[] args) throws Exception {
if (args.length != 2 || args[0].equals("--help") || args[0].equals("-h")) {
if (args.length < 2 || args.length > 3 || args[0].equals("--help") || args[0].equals("-h")) {
System.err.println("Test for listing available algorithms and providers and checking their FIPS compatibility");
System.err.println("Usage: CheckAlgorithms " + possibleFirstArgs + " " + possibleSecondArgs);
System.err.println("Usage: CheckAlgorithms " + possibleFirstArgs + " " + possibleSecondArgs + " " + possibleThirdArgs);
System.err.println("First argument: specify whether to check FIPS compatibility (assert/true) or just list the items (list/false)");
System.err.println(" silent-asserts just asserts, not lists the items");
System.err.println("Second argument: specify what to check - algorithms, providers or both");
System.err.println("Third argument: specify the operating system for the checking, RHEL 8 or 9");
System.err.println(" optional, defaults to RHEL 8");
System.exit(1);
}

// Parse the arguments
String shouldHonorFips = args[0].toLowerCase();
String testCategory = args[1].toLowerCase();
String operatingSystem = "";
if (args.length == 3) {
operatingSystem = args[2].toLowerCase();
}

// Check if the shouldHonorFips is valid value
if (!possibleFirstArgs.contains(shouldHonorFips)) {
Expand All @@ -38,12 +45,31 @@ public static void main(String[] args) throws Exception {
System.exit(1);
}

// Check if the operatingSystem is valid value
if (!operatingSystem.isEmpty() && !possibleThirdArgs.contains(operatingSystem)) {
System.err.println("Invalid operating system: '" + args[2] + "', use --help for more info.");
System.exit(1);
}

System.out.println("Test type: " + shouldHonorFips);
System.out.println("What is tested: " + testCategory);
System.out.println("Operating system: " + operatingSystem);

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb. TY!

NONFIPS_PROVIDERS = EL9_NONFIPS_PROVIDERS;
FIPS_ALGORITHMS = EL9_FIPS_ALGORITHMS;
NONFIPS_ALGORITHMS = EL9_NONFIPS_ALGORITHMS;
} else {
FIPS_PROVIDERS = EL8_FIPS_PROVIDERS;
NONFIPS_PROVIDERS = EL8_NONFIPS_PROVIDERS;
FIPS_ALGORITHMS = EL8_FIPS_ALGORITHMS;
NONFIPS_ALGORITHMS = EL8_NONFIPS_ALGORITHMS;
}

boolean algorithmsOk = true;
boolean providersOk = true;
if (testCategory.equals("algorithms") || testCategory.equals("both")){
Expand Down Expand Up @@ -171,5 +197,28 @@ static boolean providerAssert() {

return allOk;
}

// the algorithm and providers fips and non-fips values:
public static final List<String> EL8_FIPS_PROVIDERS = Arrays.asList("SunPKCS11-NSS-FIPS");
public static final List<String> EL8_NONFIPS_PROVIDERS = Arrays.asList("SunJGSS", "SunSASL", "SunPCSC", "JdkLDAP", "JdkSASL", "SunPKCS11");
public static final List<String> EL8_FIPS_ALGORITHMS = Arrays.asList();
public static final List<String> EL8_NONFIPS_ALGORITHMS = Arrays.asList("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");

public static final List<String> EL9_FIPS_PROVIDERS = Arrays.asList("SunPKCS11-NSS-FIPS");
public static final List<String> EL9_NONFIPS_PROVIDERS = Arrays.asList("SunJGSS", "SunSASL", "SunPCSC", "JdkLDAP", "JdkSASL", "SunPKCS11");
public static final List<String> EL9_FIPS_ALGORITHMS = Arrays.asList();
public static final List<String> EL9_NONFIPS_ALGORITHMS = Arrays.asList("TLS_CHACHA20_POLY1305_SHA256", "TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256",
"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", "TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256",
"TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384", "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384",
"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256",
"TLS_DHE_RSA_WITH_AES_256_CBC_SHA256", "TLS_DHE_RSA_WITH_AES_128_CBC_SHA256",
"TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA", "TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",
"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA", "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
"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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

30 changes: 25 additions & 5 deletions containersQa/testlib.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,21 +942,24 @@ function setupAlgorithmTesting {
cipherListCode=`cat $LIBCQA_SCRIPT_DIR/CipherList.java`
}

function logHostAndContainerCryptoPolicy() {
function checkHostAndContainerCryptoPolicy() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
echo "Crypto policy on RHEL host ($hostCryptoPolicy) is not the same as in the container ($containerCryptoPolicy), which was unexpected."
exit 1
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fi
}

Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
echo "RHEL host is in FIPS, container returned $containerReturnCode, which was unexpected, expected not zero."
exit 1
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
echo "RHEL host is not in FIPS, container returned $containerReturnCode, which was unexpected, expected zero."
exit 1
fi

# other variants (for example Fedora), the behavior is just logged
else
echo "Non-RHEL host, undefined FIPS, container returned $containerReturnCode."
exit
fi
}

function listCryptoAlgorithms() {
function assertCryptoAlgorithms() {
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] ....

commandAlgorithms="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms assert algorithms el9"
else
commandAlgorithms="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms assert algorithms"
fi

echo "runOnBaseDirBash $commandAlgorithms"
runOnBaseDirBash "$commandAlgorithms" 2>&1| tee $REPORT_FILE
set -x
}

function listCryptoProviders() {
function assertCryptoProviders() {
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above regarding the el9/el9z

commandProviders="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms assert providers el9"
else
commandProviders="echo '$checkAlgorithmsCode' > /tmp/CheckAlgorithms.java && echo '$cipherListCode' > /tmp/CipherList.java && javac -d /tmp /tmp/CheckAlgorithms.java /tmp/CipherList.java && java -cp /tmp CheckAlgorithms assert providers"
fi

echo "runOnBaseDirBash $commandProviders"
runOnBaseDirBash "$commandProviders" 2>&1| tee $REPORT_FILE
set -x
Expand Down