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

Remade CheckAlgorithms.java test to be a little more advanced #31

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

patrikcerbak
Copy link
Contributor

Updated CheckAlgorithms.java, these are the main changes:

  • providers are now also printed, not just the algorithms
  • I added another option for the second switch, for now it was only algorithms or providers, I added the option for both which first lists/checks the algorithms and then the providers
  • apart from the true/false switches for checking FIPS compatibility, I added only-algorithms and only-providers for checking FIPS compatibility only on the specified category
  • I added a brief help prompt
  • I remade some logic for checking the FIPS compatibility and made the exceptions' texts more specific

Since there are still the same switches (true/false and algorithms/providers) in the same order available, the test should be backwards compatible and should not break anything.

// Check if the shouldHonorFips is "true" or "false"
if (!args[0].equalsIgnoreCase("true") && !args[0].equalsIgnoreCase("false")) {
// Check if the shouldHonorFips is valid value
List<String> possibleFirstArgs = Arrays.asList("true", "false", "only-algorithms", "only-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 is duplicated with help. Please make it static final, immutable, and in the hel, stream.join it. it od not need to be perfect, but bbetter then duplicate it.

if (!testCategory.equalsIgnoreCase("algorithms") && !testCategory.equalsIgnoreCase("providers")) {
System.err.println("Invalid test category: " + testCategory);
// Check if the test category is "algorithms", "providers" or "both"
List<String> possibleSecondArgs = Arrays.asList("algorithms", "providers", "both");
Copy link
Collaborator

@judovana judovana Nov 5, 2024

Choose a reason for hiding this comment

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

This is duplicated with help. Please make it static final, immutable, and in the help, stream.join it. it od not need to be perfect, but bbetter then duplicate it.

@judovana
Copy link
Collaborator

judovana commented Nov 5, 2024

At the end this is just vey minor, backward compatibel enhancment, right?

I still dont like the <true|false|only-algorithms|only-providers> esepcially with algorithms", "providers", "both"

it is twelve possible combinations, but only subset gave sense.
I would vote for true/false only, but with alias "assert" or "fail" or "test" for true, and "list" for false;
Second pareter is more correct, and should stay as is.
The possibility to "test in only-algorithms" and together haveing "providers" (to "test in only-providers" and together having "algorithms") seems quite hard to understand.

Thus the first parameter is whether we are testing or jsut listing, but second is what. algortighms, or providers, or both.
Slightly questionable is, what to assert of test mode is on, and "both" is on second place. Ideally both will be asserted, and correct exception of "invalid algorithm found" or "invalid provider found" or "both invalid provider and algorithm found". it would be also ince to print out what is asserted against what and how it went, becasue we may have much more asserts alter, even conditional ones, and we may have more asserts per subset.

Currently, the test is printing in both listing and testing cases, and that is correct. It may be good to think about oen more option, silent-test, which will skip the printing. (as in case of correclty written 'what is tested against what' the asserting output may be enough).

@patrikcerbak
Copy link
Contributor Author

Some new updates:

  • the true/false arguments now have aliases assert/list
  • the help prompt now uses same list as the argument checking
  • updated the output format of this test, to be more readable/consistent
  • changed the asserting logic
  • with that, I also added printing of the asserts (example output is lower)
  • added silent-assert argument, which skip the listing of the algorithms/providers and just does the asserts

Example output for arguments assert both (the ... are just here, so the comment doesn't take half the page :D ):

Test type: assert
What is tested: both
>>>CHECKING ALGORITHMS<<<
LIST OF ALGORITHMS:
  TLS_AES_256_GCM_SHA384
  ...
  TLS_RSA_WITH_AES_128_CBC_SHA
  TLS_EMPTY_RENEGOTIATION_INFO_SCSV
ASSERTING FIPS ALGORITHMS:
  asserting algorithms don't contain 'TLS_RSA_WITH_AES_128_CBC_SHA' - FAIL
>>>CHECKING PROVIDERS<<<
LIST OF PROVIDERS:
  SUN version 21
  SunRsaSign version 21
  ...
  SunPCSC version 21
  SunPKCS11 version 21
ASSERTING FIPS PROVIDERS:
  asserting providers contain 'SunPKCS11-NSS-FIPS' - FAIL
  asserting providers don't contain 'SunPCSC' - FAIL
Exception in thread "main" java.lang.Exception: Both algorithms and providers contain wrong or don't contain correct items.
	at CheckAlgorithms.main(CheckAlgorithms.java:56)

What do you think? Is this output format ok or should I change?

@judovana
Copy link
Collaborator

judovana commented Nov 6, 2024

Thanx, lets go with it as it is and tune it on the fly

@judovana judovana merged commit ea3c06d into rh-openjdk:main Nov 6, 2024
16 checks passed
@patrikcerbak patrikcerbak deleted the update-checkalgorithms branch November 12, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants