-
Notifications
You must be signed in to change notification settings - Fork 923
Ada Bindings and CI Improvements #9617
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
base: master
Are you sure you want to change the base?
Conversation
julek-wolfssl
commented
Jan 6, 2026
- Add new crypto support (RSA, SHA, AES, RNG)
- CI and Testing Enhancements
- Added GNATprove static analysis and memory tracking to CI.
- Added Ada unit tests using AUnit.
- Integrated Ada AUnit tests and valgrind suppressions for leak checks.
- Updated Ada test harness, reduced boilerplate, and improved suite registration.
- Ada Bindings and Examples
- Refactored AES, RSA, RNG, and SHA256 objects to use dynamic allocation.
- Added and improved client-server and cryptographic examples (AES, RSA, SHA256).
- Moved examples to a dedicated sub-directory and updated related build files.
- Documentation and Configuration
- Updated README and added documentation for running tests with valgrind and using alr exec.
- Code Quality and Maintenance
- Fixed warnings, improved style, and added ownership/postcondition annotations for GNATprove.
- Ensured GNATprove is clean on examples.gpr.
7b03d32 to
7fe01ba
Compare
|
Retest this please AgentOffline |
There was a problem hiding this 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 pull request adds Ada bindings and cryptographic support (RSA, SHA, AES, RNG) while enhancing CI testing and code quality through GNATprove static analysis, memory tracking, and AUnit tests. The PR also reorganizes examples into a dedicated subdirectory and improves documentation.
Key changes:
- Added cryptographic bindings (RSA, SHA256, AES, RNG) with dynamic allocation
- Introduced AUnit test framework and valgrind memory checks
- Moved examples to
examples/subdirectory with dedicated build configuration - Added GNATprove annotations for ownership checking and memory safety
Reviewed changes
Copilot reviewed 40 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wrapper/Ada/wolfssl.gpr | Removed exclusion of TLS client/server sources (now in examples/) |
| wrapper/Ada/wolfssl.ads | Added crypto bindings (RSA, SHA256, AES, RNG) with ownership annotations |
| wrapper/Ada/wolfssl.adb | Implemented crypto bindings with dynamic allocation and exception handling |
| wrapper/Ada/wolfssl-full_runtime.ads | New package for full runtime features (GNAT.Sockets, PSK callbacks) |
| wrapper/Ada/wolfssl-full_runtime.adb | Implementation of full runtime features |
| wrapper/Ada/tls_server.adb | Updated for new API (Read/Write procedures, moved PSK to Full_Runtime) |
| wrapper/Ada/tls_client.ads | Fixed indentation consistency |
| wrapper/Ada/tls_client.adb | Updated for new API and moved examples |
| wrapper/Ada/tests/* | Added AUnit test infrastructure and test cases |
| wrapper/Ada/examples/* | Moved examples to subdirectory with new build configuration |
| wrapper/Ada/ada_binding.c | Added C wrapper functions for crypto operations |
| .github/workflows/ada.yml | Enhanced CI with alire, tests, valgrind, and GNATprove |
Comments suppressed due to low confidence (6)
wrapper/Ada/wolfssl.ads:1
- The
Terminator_Errorexception is declared but never documented. Add a comment explaining when this exception is raised and how it should be handled.
wrapper/Ada/wolfssl.ads:1 - The comment says 'RSA' but should say 'RNG' since this is the
Is_Validfunction forRNG_Key_Type.
wrapper/Ada/wolfssl.adb:1 - The constant name
nulcould be more descriptive. Consider renaming toNull_TerminatororNul_Byteto better indicate its purpose as a null terminator character.
wrapper/Ada/wolfssl.adb:1 - Magic number -121212 is used as a default error value in multiple places. Define this as a named constant (e.g.,
Default_Error_Value) to improve maintainability and clarify its purpose.
wrapper/Ada/wolfssl.adb:1 - Magic number -121212 is used as a default error value in multiple places. Define this as a named constant (e.g.,
Default_Error_Value) to improve maintainability and clarify its purpose.
wrapper/Ada/wolfssl.adb:1 - Magic number -121212 is used as a default error value in multiple places. Define this as a named constant (e.g.,
Default_Error_Value) to improve maintainability and clarify its purpose.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JacobBarthelmeh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment about using XMALLOC over malloc. In the future we may need something like the static memory feature so starting out with XMALLOC would help for if using a heap hint later.
Thank you for adding in the test cases. I compiled both the examples and tests and they ran okay with a newer version of gnat, not sure about old versions of gnat.
bash-3.2$ alr run
ⓘ Building tests=0.1.0-dev/tests.gpr...
gprbuild: "tests" up to date
✓ Build finished successfully in 0.87 seconds.
OK SHA256('asdf') produces expected hash
OK SHA256('') produces expected hash
OK RSA sign/verify and encrypt/decrypt (rsa_verify_main)
OK AES-CBC encrypt/decrypt roundtrip
OK AES_Free succeeds after Create_AES
Total Tests Run: 5
Successful Tests: 5
Failed Assertions: 0
Fixed. |
|
Retest this please Jenkins
|
|
very nice work. I didn't review tests |
5ee8987 to
1621908
Compare
1621908 to
af9086a
Compare
|
Retest this please StreamCorruptedException |
|
retest this please |
|
retest this please |
|
Jenkins retest this please. History lost |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 41 out of 47 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with Ada.Unchecked_Conversion; | ||
| pragma Warnings (Off, "* is an internal GNAT unit"); | ||
| with GNAT.Sockets.Thin_Common; | ||
| pragma Warnings (On, "* is an internal GNAT unit"); | ||
| with Interfaces.C.Extensions; | ||
| with Interfaces.C.Strings; | ||
| with System; | ||
| with WolfSSL; | ||
|
|
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapper/Ada/wolfssl.adb is the body of package WolfSSL, but it has a with WolfSSL; clause. A unit cannot with itself; this will cause a circular dependency / compilation error. Remove the self-with and instead with the actual dependencies needed by the body (or rely on the spec’s withs where appropriate).
| AUnit.Assertions.Assert | ||
| (Hash_Bytes = Expected_Hash, | ||
| "SHA256('asdf') hash mismatch"); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected_Hash returned by Hex_Bytes is indexed 0 .. 31, but Hash_Bytes is indexed 1 .. 32. For Interfaces.C.char_array, predefined equality includes bounds, so this comparison will fail even when the bytes match. Normalize bounds (e.g., make both arrays use the same range) before comparing.
| AUnit.Assertions.Assert | ||
| (Decoded = Plain, | ||
| "AES-CBC roundtrip mismatch"); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plain comes from Test_Support.Bytes (0-based Byte_Array), but Decoded is declared Byte_Array (1 .. Plain'Length), so Decoded = Plain will fail due to different bounds even if the data matches. Declare Cipher/Decoded with Plain'Range (or otherwise normalize ranges) so equality is meaningful.
| Last_N : constant Natural := (if S'Length = 0 then 0 else S'Length - 1); | ||
| B : WolfSSL.Byte_Array (0 .. WolfSSL.Byte_Index (Last_N)); | ||
| N : Natural := 0; |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For empty input, Last_N is set to 0 and B becomes Byte_Array (0 .. 0), which is a 1-byte array rather than an empty buffer. If callers intend Bytes("") to represent an empty C buffer, consider returning a null range (e.g. 1 .. 0) for the empty-string case.
| declare | ||
| N : constant Natural := Len / 2; | ||
| Last_N : constant Natural := (if N = 0 then 0 else N - 1); | ||
| B : WolfSSL.Byte_Array (0 .. WolfSSL.Byte_Index (Last_N)); | ||
| Hi : Natural; |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Hex is empty, this constructs Byte_Array (0 .. 0) (1 byte) instead of an empty buffer. Consider returning a null range for N = 0 so Hex_Bytes("") is truly empty, and to avoid bound-mismatch surprises in callers.
|
@julek-wolfssl please squash the 51 commits. Also review the latest copilot findings. Thanks |
af9086a to
f92d63e
Compare
Add Ada bindings for SHA-256, RSA sign/verify, and AES-CBC from wolfCrypt. Use XMALLOC/XFREE for dynamic allocation and add GNATprove ownership annotations to enable static leak detection. Refactor the Ada wrapper into a base package (wolfssl.ads) and a child package (wolfssl-full_runtime) to separate code that depends on Interfaces.C.Strings and GNAT.Sockets from zero-footprint-compatible code. Add standalone examples for SHA-256 hashing, RSA signature verification, and AES encryption under wrapper/Ada/examples/. Add AUnit test suites for SHA-256, RSA, and AES bindings under wrapper/Ada/tests/ with Valgrind suppressions and Alire integration. Move TLS client/server examples into wrapper/Ada/examples/src/ and update build files (default.gpr, examples.gpr, include.am) accordingly. Update CI (ada.yml) to build default.gpr, run AUnit tests, run the client-server examples, and run GNATprove. Co-authored-by: Joakim Strandberg <joakim@mequinox.se>
f92d63e to
40d3bef
Compare
|
Retest this please AgentOfflineException |