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.

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

Add new security tests (Part 1: CryptoTest) #3829

Closed
smlambert opened this issue Jul 7, 2022 · 14 comments
Closed

Add new security tests (Part 1: CryptoTest) #3829

smlambert opened this issue Jul 7, 2022 · 14 comments

Comments

@smlambert
Copy link
Contributor

smlambert commented Jul 7, 2022

We will like to add some new security tests contributed from the Red Hat team. We have a couple of options for how to include them in the AQA test suite:

  1. Add them to the functional group
  2. Create a new test group called security and add them there.

The 2nd option implies expanding the test grid by adding a new column, and also as we have security tests scattered in most of the groups (system, openjdk, external, functional), we could consider moving them into the security group. Unfortunately, this is not necessarily a good idea, as it implies that the setup and compilation time will be high, as each of the existing tests has their own set of prereqs that would need to be downloaded and compiled. So the course of action if we take option 2 is to use the new group for new tests that do not have a large set of prereqs and requirements.

The new tests of concern for this issue can be found here:
https://github.com/rh-openjdk/CryptoTest - #4356
https://github.com/rh-openjdk/ssl-tests

We will plan to do this after branching in prep for the July release (so these new tests will not be part of the July release branch).

We will intend to add these tests as 2 unique new test targets into the functional group, at the dev level.

The work required is to clone the test material via the functional/security/build.xml file and add the 2 new targets in the functional/security/playlist.xml file.

@llxia
Copy link
Contributor

llxia commented Jul 7, 2022

I see pros and cons in both options. Personally, I think I prefer option 1 a little more because the existing security tests are scattered. Once we have the new security tests added, we can then evaluate and adjust them if needed.

@smlambert
Copy link
Contributor Author

Agree, I think that is a good approach too, as we do not delay adding the tests, and we can consider implications of Option 2 on execution time if we want a future re-org of test material.

@lumuchris256
Copy link
Contributor

hi @smlambert i would like to take a spin on this , more detail is welcome

@smlambert
Copy link
Contributor Author

Thanks @lumuchris256 - this is a much desired feature, and it will be great if you help to add these new security tests!

It will be helpful for you to look at the README of each of the 2 repositories of test material we wish to add:
https://github.com/rh-openjdk/CryptoTest/blob/master/README.md
tells how to build the test material and how to run tests.

To incorporate new test material, we typically put the information for how to download and build test material into the build.xml file which is an ant script, and information about how to run the test in the playlist.xml file. Then we would try to run the test locally or in Docker locally or by running them via Github actions.

I think that build.xml will need to contain targets that git clone the 2 repositories, plus a target that will build them, so in the case of CryptoTest (according to its README), it will run make classes in the cloned test material.

playlist.xml will then have 2 new entries, using the same xml schema as the other test target present, that would look something like:

<test>
		<testCaseName>rh-CryptoTest</testCaseName>
		<command>make SKIP_AGENT_TESTS=1 all \
		$(TEST_STATUS)</command>
		<levels>
			<level>dev</level>
		</levels>
		<groups>
			<group>functional</group>
		</groups>
	</test>
<test>
		<testCaseName>rh-ssl-tests</testCaseName>
		<command>make ssl-tests SSLTESTS_USE_OPENSSL_CLIENT=1 \
		$(TEST_STATUS)</command>
		<levels>
			<level>dev</level>
		</levels>
		<groups>
			<group>functional</group>
		</groups>
	</test>

We will most certainly need to test this well on many platforms. General instructions for running tests locally are at:
https://github.com/eclipse-openj9/openj9/wiki/Reproducing-Test-Failures-Locally#general-steps

@sophia-guo
Copy link
Contributor

sophia-guo commented Feb 8, 2023

In the PR #4319 we are running the CryptoTests by jar file and got the test failures.
CryptoTests.jar.zip

----------------------------------
Total checks: 13, failed: 0
Checked 13 providers
no bad provider appeared (from total of 1: [SunPKCS11-NSS])
all expected providers appeared (from total of 0: [])]nfailed: 0 providers
PASSED: cryptotest.tests.TestProviders
Total checks: 569, failed: 0
Checked 569 services
no bad curve appeared (from total of 2: [NIST P-192, 1.2.840.10045.3.1.1])
all expected curves appeared (from total of 0: [])
failed: 0 services
PASSED: cryptotest.tests.TestServices
Total checks: 1, failed: 1
Number of checked services changed during test run
FAILED: cryptotest.CryptoTest$ConstantServices
Total checks: 1, failed: 1
Some algorithms missed! Checked 0 from -2147483648
FAILED: cryptotest.CryptoTest$NoAlgorithmMissed
----------------------------------
PASSED: cryptotest.tests.TestProviders
PASSED: cryptotest.tests.TestServices
FAILED: cryptotest.CryptoTest$ConstantServices
FAILED: cryptotest.CryptoTest$NoAlgorithmMissed
Test runs: 4; failed :2
Exception in thread "main" java.lang.RuntimeException: Some tests failed: 2
	at cryptotest.CryptoTest.main(CryptoTest.java:109)
-----------------------------------
CryptoTests_0_FAILED

If running by classes no failures. The jar file has also jar the tests.jks file which is copied to classes/cryptotest/tests/

Feels like the failures are caused by running with the jar file ( replace the -cp with classes tests pass). Question: Can the tests run by jar file? If yes, what extra steps should we do?

@smlambert
Copy link
Contributor Author

Tagging @zzambers for input to the question above

@zzambers
Copy link
Contributor

zzambers commented Feb 8, 2023

Seems like there is something missing here. For full testsuite, there should be many testcases and much more output. I would expect to see something like:
https://github.com/rh-openjdk/CryptoTest/actions/runs/4008601900/jobs/6882968215#step:5:8

But to be honest I have never tried to run this as jar. I'll need to try this locally.

@judovana Is this familiar to you?

@sophia-guo
Copy link
Contributor

sophia-guo commented Feb 8, 2023

Yes, if I run by classes the output are same as https://github.com/rh-openjdk/CryptoTest/actions/runs/4008601900/jobs/6882968215#step:5:8 ( I'm using Mac locally). I have compared the content of jar file with the contents of classes and the contents are exactly same. I'm curious about the tests.jks file. Might be related with it?

@zzambers
Copy link
Contributor

zzambers commented Feb 8, 2023

@zzambers
Copy link
Contributor

zzambers commented Feb 8, 2023

This is bug in Cryptotest, I have created PR with fix:
rh-openjdk/CryptoTest#28

@sophia-guo
Copy link
Contributor

This is bug in Cryptotest, I have created PR with fix:
rh-openjdk/CryptoTest#28

Tried locally with @zzambers fix, works good! Thanks @zzambers

@sophia-guo
Copy link
Contributor

Running against latest branch with jtreg binding without jtreg, similar way of github runners, tests run good https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6608/console. Running with jtreg locally with the wrapper run.sh three tests failed. Attached the result jar file.
test.1675972215.tar.gz

@judovana idea?

@sophia-guo
Copy link
Contributor

NM. It turns out I missed Property -Dcryptotests.skipAgentTests=1

https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6619/

@smlambert smlambert changed the title Add new security tests Add new security tests (CryptoTest & ssl-tests) Feb 22, 2023
@smlambert smlambert changed the title Add new security tests (CryptoTest & ssl-tests) Add new security tests (Part 1: CryptoTest) Mar 8, 2023
@smlambert
Copy link
Contributor Author

I have split this issue into 2 parts, this one can be considered closed, the addition of ssl-tests is captured now in a separate issue #4403

@github-project-automation github-project-automation bot moved this from In Progress to Done in Adoptium 1Q 2023 Plan Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

5 participants