Skip to content

Conversation

@jackctj117
Copy link
Contributor

@jackctj117 jackctj117 commented Apr 11, 2025

Added BC, wolfJCE and Sun signature algorithms benchmarks as well as refactored other test to better insert security providers to avoid provider conflicts.

@jackctj117 jackctj117 self-assigned this Apr 11, 2025
@jackctj117 jackctj117 requested a review from Copilot April 11, 2025 23:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

examples/provider/CryptoBenchmark.java:725

  • Reassigning providerName when using the SunJCE branch changes its identity for later logging; consider using a separate variable (e.g., actualProviderName) to retain the original provider identity for clarity.
providerName = "SunRsaSign";

examples/provider/CryptoBenchmark.java:737

  • [nitpick] The EC curve name 'secp256r1' is repeated; consider extracting it into a constant to improve maintainability.
keyGen.initialize(new ECGenParameterSpec("secp256r1"));

Copy link
Member

@cconlon cconlon left a comment

Choose a reason for hiding this comment

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

I don't understand why we can't include SUN/Oracle Signature benchmarks. As long as we are calling public Java Security API's, doing something like Signature.getInstance("SHA256withRSA", "SunRsaSign"); shouldn't cause any problems that I am aware of.

The list of Oracle security providers and the services they offer for Java 11 (for example) can be found here: https://docs.oracle.com/en/java/javase/11/security/oracle-providers.html.

@cconlon cconlon removed their assignment May 8, 2025
@jackctj117 jackctj117 force-pushed the wolfJCE_benchmark_signatures branch 2 times, most recently from 987963b to 7f18dc6 Compare June 2, 2025 23:56
@jackctj117 jackctj117 requested a review from Copilot June 3, 2025 18:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds signature performance benchmarks for BC, wolfJCE, and Sun providers and refactors provider setup to avoid conflicts across tests.

  • Introduces SIGNATURE_ALGORITHMS and SUN_SIGNATURE_ALGORITHMS arrays and a new runSignatureBenchmark method
  • Consolidates provider insertion/removal into setupProvidersForTest and adds setupDigestProvider
  • Updates main to invoke signature benchmarks and cleans up table formatting and exception handling

@cconlon cconlon removed their assignment Jun 3, 2025
@jackctj117 jackctj117 requested a review from Copilot June 3, 2025 18:45

This comment was marked as outdated.

@jackctj117 jackctj117 requested a review from Copilot June 3, 2025 19:33

This comment was marked as outdated.

@jackctj117 jackctj117 force-pushed the wolfJCE_benchmark_signatures branch from 9a212a3 to 4bc62c6 Compare June 3, 2025 19:35
@jackctj117 jackctj117 changed the title Added signature benchmarks excluding Sun providers Added signature benchmarks Jun 3, 2025
@jackctj117 jackctj117 force-pushed the wolfJCE_benchmark_signatures branch 2 times, most recently from ece8664 to f64f90b Compare June 3, 2025 21:35
@cconlon cconlon requested a review from Copilot June 3, 2025 21:56

This comment was marked as outdated.

Copy link
Member

@cconlon cconlon left a comment

Choose a reason for hiding this comment

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

See new Copilot suggestions

@cconlon cconlon removed their assignment Jun 3, 2025
@jackctj117 jackctj117 force-pushed the wolfJCE_benchmark_signatures branch 2 times, most recently from a47e802 to 028f172 Compare June 3, 2025 22:28
@cconlon
Copy link
Member

cconlon commented Jun 4, 2025

When I run the benchmark with Java 23, I get this error:

Running crypto benchmark with Bouncy Castle (version 1.80)
Exception in thread "main" java.lang.IllegalAccessError: class CryptoBenchmark (in unnamed module @0x64485a47) cannot access class com.sun.crypto.provider.SunJCE (in module java.base) because module java.base does not export com.sun.crypto.provider to unnamed module @0x64485a47
	at CryptoBenchmark.main(CryptoBenchmark.java:930)
../../../lib/bcprov-jdk18on-1.80.jar	../../../lib/bctls-jdk18on-1.80.jar

ChatGPT thinks the SunEC provider is already registered on all versions of Java, and we don't need to instantiate it ourselves (https://chatgpt.com/share/683f8cfa-5d80-8000-8648-a8fbaeca3cb3). Can you try to adjust so we can maintain compatibility with running on newer Java versions?

@cconlon cconlon removed their assignment Jun 4, 2025
@jackctj117 jackctj117 requested a review from Copilot June 4, 2025 17:06

This comment was marked as outdated.

@jackctj117 jackctj117 requested a review from Copilot June 4, 2025 17:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds new signature benchmarks for BC, wolfJCE, and Sun signature algorithms and refactors test setup to avoid provider conflicts. Key changes include:

  • Introduction of a runSignatureBenchmark method and supporting helper methods for signature algorithm detection and execution.
  • Refactoring of provider setup across different benchmark sections to ensure a clean environment.
  • Minor formatting updates in benchmark output displays.

@jackctj117 jackctj117 force-pushed the wolfJCE_benchmark_signatures branch from 92dbe19 to 7d285d9 Compare June 4, 2025 19:45
@cconlon cconlon merged commit a7b6f82 into wolfSSL:master Jun 4, 2025
41 checks passed
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