Integrate OpenSSL AES and MD related code into the new framework - not ready to merge#1
Integrate OpenSSL AES and MD related code into the new framework - not ready to merge#1thu-ibm wants to merge 5 commits into
Conversation
| @@ -0,0 +1,259 @@ | |||
| /* | |||
There was a problem hiding this comment.
In general i dont anticipate too many openssl specific tests to be developed. Whatever is developed should be written for both "backends" which includes ockc and openssl and such tests parameterized for all of our desired test configurations ( such as a openssl test only provider )
This being said there could be some on off specific tests for features that are only available in one backend or another however something like what is outlined in this PR with various duplicate tests for algorithms we support we wouldnt want to add.
As part of running tests through mvn a jacoco code coverage report is generated. I highly recommend that we consult this coverage report to see gaps and changes in testing that we want to add for all library backends.
There was a problem hiding this comment.
Thanks for pointing this out. These are not intended to be permanent tests, they are just used for now to verify the Adapter java code and the native functions. Eventually the goal is to use the existing tests that we have when the new framework and tests are updated and ready.
| static OpenSSLContext* nonFipsContext = NULL; | ||
| static OpenSSLContext* fipsContext = NULL; |
There was a problem hiding this comment.
I think we should keep with what we already do for OCKC and maintain the different context pointers in the java adapters.
There was a problem hiding this comment.
Ok I will look into that. Thanks!
|
Another question is, why all these header files for each of the native files? |
To me this way looks cleaner. I asked Bob if this is better way, and this is what it says: :-) For this codebase: yes, generally one header per native implementation area is the better structure, and you already mostly follow that pattern (OpenSSLSignature.h/.c, OpenSSLDigest.h/.c, OpenSSLHMAC.h/.c, etc.). Why it is better:
|
All these are good reasons in general, but I don't think they fully apply to our project. I asked Bob explicitly, asking it to take the OpenJCEPlus project as context, and the answer was as follows: No, creating header files for each C file would NOT be useful in your case. Here's why: Your Current ArchitectureEach C file (like
Why Headers Per File Are Unnecessary
What You Already Have (and Should Keep)✅ Shared utility headers like Conclusion: Your current approach is correct. Only create headers for truly shared code (utilities, constants, structs), not for independent JNI implementation files. |
Thanks @KostasTsiounis for your further consideration on this! I also thought about it more and asked Bob to look at the project and also our discussions in this pull request. Following is what it said. Please see if you agree. The better approach for this project is: keep per-module headers, but only where they define a real shared interface; avoid headers that are just mirrors of a single implementation file. Why that conclusion fits this repo: The OpenSSL native layer is already organized by implementation area: OpenSSLSignature.h, OpenSSLSymmetricCipher.h, OpenSSLDigest.h, OpenSSLHMAC.h, etc. That matches the PR response and is a sensible structure for a JNI/native boundary. Some headers are clearly justified because they expose shared types or functions used across files. Example: OpenSSLSymmetricCipher.h defines CipherContext, and both OpenSSLCCM.c and OpenSSLGCM.c include it. Likewise, OpenSSLHelpers.h is a true common utility interface used broadly. But not every file-paired header is equally valuable. OpenSSLJNI.h is very thin and mainly included by its own implementation in OpenSSLJNI.c. If a header is not consumed by other translation units and contains no important shared declarations, it adds maintenance overhead without much payoff. So the best rule here is not “one header for every native file” and not “collapse everything into one big header.” It is: one header per native module/area when that module exports shared declarations, JNI entry points, structs, constants, or helper APIs; no separate header when the declarations are purely local to one .c file and can be kept static inside that file. Per-file/per-module headers: Pros: clearer boundaries, easier navigation, better dependency hygiene, less accidental coupling, closer to how this repo is already structured. Cons: can become boilerplate if every .c gets a mostly-empty companion header. My recommendation for this codebase: Keep the modular header layout as the default. Prune or avoid trivial headers like OpenSSLJNI.h when they don’t define meaningful cross-file contracts. Treat OpenSSLHelpers.h and data-structure headers like OpenSSLSymmetricCipher.h as the good pattern. Prefer internal static helpers over header declarations unless another file truly needs them. In short: Kostas’s concern is valid if the headers are merely 1:1 wrappers, but the repo as a whole benefits from module-scoped headers. The strongest design is a selective modular-header approach, not either extreme. |
Attempted to integrate the OpenSSL native code for the AES and MD algorithms with following two existing PRs. Since these PRs have not yet been merged into main, I created a separate repository to facilitate code sharing and review.
IBM/OpenJCEPlus#1167
IBM/OpenJCEPlus#851