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

[Ada] Initial library support #7303

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

[Ada] Initial library support #7303

wants to merge 7 commits into from

Conversation

Irvise
Copy link

@Irvise Irvise commented Mar 6, 2024

Description

The current Ada wrapper and its associated build scripts create a client/server setup/example. However, this is not enough (from my understanding) in order to easily consume WolfSSL from an Ada project. WolfSSL should be built as a library using the files already present in the wrapper. This commit adds an initial implementation of the library build script that should be usable for any Ada project. It is based on the current server build script.

However, some issues/limitations have to be mentioned. I am unsure about how the required Linker flags should be added. For library project in GPRBuild, the Linker package has no effect, as that is for the final executable. So I suppose the consumer of WolfSSL's library will have to add the specified linker flags.

Secondly, and in my opinion, most importantly, is how to properly handle the different WolfSSL configuration options within the GPRBuild script... I know that there is user_settings.h and that is what currently configures the build process and all the details, but I would prefer if that was not fixed to the provided file... Of course GPRBuild supports user defined options, but I am unsure on how to properly handle the many options that WolfSSL has. I am also including in this aspect the correct handling of the different architectures! For example, I would like to try and use WolfSSL with my new ESP32-C6 (RISC-V) chip :)

I also created an alire.toml file for WolfSSL that builds the different project files. It can be found below. Alire, for those that don't know what it is, is equivalent to Rust's Cargo or C++ Conan, a build/project manager. Once the library build option is finished I will publish the Alire recipe to the main repository for everybody's ease of use.

name = "wolfssl_ada"
description = "WolfSSL encryption library and its Ada bindings"
version = "5.6.6-dev"

authors = ["Fernando Oleo Blanco"]
maintainers = ["Fernando Oleo Blanco <[email protected]>"]
maintainers-logins = ["Irvise"]
licenses = "GPL-2.0-only"
website = "https://www.wolfssl.com/"
project-files = ["wrapper/Ada/wolfssl.gpr", "wrapper/Ada/default.gpr", "wrapper/Ada/client.gpr"]
tags = ["ssl", "tls", "embedded"]

executables = ["tls_server_main"]

Documentation will be filled once the scope is fixed and the library works as expected.

I am opening this PR to start gathering feedback and comments.

I am pinging @dgarske as the committer of the bulk of the bindings. I cannot find Joakim's Github user (if there is any).

P.S: this pull requests comes after a conversation that I had with Martin (from the UK division).

Testing

The current GPR file builds the .a library, no further checks have been performed.

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@Irvise Irvise marked this pull request as draft March 6, 2024 20:14
@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@dgarske
Copy link
Contributor

dgarske commented Mar 6, 2024

Hi @Irvise ,

Thank for the pull request. I will look it over and I also pinged Joakim.

I don't see a contrubutor agreement on file. Would you mind submitting a ticket to our support at wolfssl dot com alias referencing this PR?

Thanks,
David Garske, wolfSSL

@embhorn
Copy link
Member

embhorn commented Mar 13, 2024

@Irvise has been approved as a wolfSSL contributor

@dgarske
Copy link
Contributor

dgarske commented Mar 13, 2024

@joakim-strandberg any feedback on this? Seems like a nice Ada feature having wolfSSL as a library.

@joakim-strandberg
Copy link

Hi all! I saw this in my e-mail just now. @Irvise , great of you to push improvements of the Ada binding forward. I saw the recorded video you organized @Irvise a couple of days ago and was very happy to see that the Ada binding to WolfSSL was the first topic of the video chat meetup. The progress I made 7 months ago with the intent of including it in Alire can be found here: https://github.com/joakim-strandberg/wolfssl/tree/ada_windows_support/wrapper/Ada , maybe something can be reused for the Alire support you are working on @Irvise ?

@dgarske
Copy link
Contributor

dgarske commented Mar 28, 2024

Hi @Irvise , what is the current state for this PR? Could you please rebase to latest master. I think that may help resolve some of the CI issues reported. Are you planning to add more to this PR?

@Irvise
Copy link
Author

Irvise commented Mar 31, 2024

Hi @dgarske, I still could not find time to properly finish it. Sorry for the delay. I will nonetheless rebase it and try to get it finished in the next couple of weeks. I may restructure the Ada bindings as in Joakim's branch too. But that shouldn't affect the use/consumption of the binding.

Btw @joakim-strandberg, did you receive my reply? Sometimes my personal email address gets marked as spam :/

@dalybrown
Copy link
Contributor

I totally didn't see this pull request. Great idea! I actually started doing the same thing ... whoops. I should probably look next time. I updated the Ada example to use DTLS here: #7397. I was going to make another pull request to make it an Alire project but you already started. I'm happy to help!

@dalybrown
Copy link
Contributor

Some initial thoughts:

  1. I think you should restructure the directory to better align the the alire project structure. (i.e., move the actual sources (wolfssl, spark_sockets to src/ folder.)
  2. Move the examples to an examples/ folder so that you don't need to exclude those files in your project file.
  3. Add an alire manifest to the root of this project where your project file will be.
  4. You can use Alire + actions to generate the user settings file to only include the features you can. You can use alire variables to control what to enable/disable and tie those in with actions (which would call configure...). I didn't try this out yet but it was what I was thinking to try out.
  5. You might have to break up the wolfssl bindings to separate files if you plan to generate the user settings file on demand. Otherwise, you'll have linker errors. You might want to simplify how much configuration you give clients as a starting point (e.g., TLS, DTLS can be separated, but not the individual versions).

@joakim-strandberg
Copy link

Thanks for your feedback Daly and your work on the support for DTLS. On Saturday April the 6:th of April 15:00 Central European Time (https://forum.ada-lang.io/t/ada-monthly-meeting/384/41) at the Ada monthly meeting the wolfSSL Ada binding will be discussed. It would be great if you would have possibility of joining too.

@dalybrown
Copy link
Contributor

Thanks for your feedback Daly and your work on the support for DTLS. On Saturday April the 6:th of April 15:00 Central European Time (https://forum.ada-lang.io/t/ada-monthly-meeting/384/41) at the Ada monthly meeting the wolfSSL Ada binding will be discussed. It would be great if you would have possibility of joining too.

Ahh I'm away this weekend so I can't attend unfortunately. Happy to help otherwise.

@dgarske dgarske self-requested a review April 5, 2024 17:01
@Irvise
Copy link
Author

Irvise commented Apr 5, 2024

Hi @dalybrown

  1. I think you should restructure the directory to better align the the alire project structure. (i.e., move the actual sources (wolfssl, spark_sockets to src/ folder.)
  2. Move the examples to an examples/ folder so that you don't need to exclude those files in your project file.

Will do :)

  1. You can use Alire + actions to generate the user settings file to only include the features you can. You can use alire variables to control what to enable/disable and tie those in with actions (which would call configure...). I didn't try this out yet but it was what I was thinking to try out.

I was also thinking about that, but I am not sure how ergonomic this will be. But I have to give it a try nonetheless!

  1. You might have to break up the wolfssl bindings to separate files if you plan to generate the user settings file on demand. Otherwise, you'll have linker errors. You might want to simplify how much configuration you give clients as a starting point (e.g., TLS, DTLS can be separated, but not the individual versions).

Yes... I am aware of the linker issues... I agree with your approach to limit the options to only the major features.

I will hack a bit on this and see what I can come up with :)

Cheers,
Fer

@Irvise
Copy link
Author

Irvise commented Apr 5, 2024

An update from my side. I managed to create a test submission of WolfSSL to the Alire index, see alire-project/alire-index#1023. This demonstrates that Alire does work correctly even with the current project structure.

I will wait for #7397 to be finished before continuing :)

@@ -0,0 +1,15 @@
name = "wolfssl_ada"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is being nitpicky, but, is the _ada really necessary here? By virtue of being an Alire crate, the ada part is implied.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I will take out the _ada surname :)

@dgarske
Copy link
Contributor

dgarske commented Apr 16, 2024

FYI: @Irvise , we have merged #7397. Looking forward to your continued work on this. Thank you, David Garske, wolfSSL

@dgarske dgarske removed their assignment Apr 16, 2024
@Irvise
Copy link
Author

Irvise commented Apr 17, 2024

I will get back to this PR then :) But I will not be available for the next week, so it will take a bit longer than I had wished for. I will also try and see if I can catch the Windows warning reported in #7397

@dalybrown
Copy link
Contributor

I will get back to this PR then :) But I will not be available for the next week, so it will take a bit longer than I had wished for. I will also try and see if I can catch the Windows warning reported in #7397

I'm happy to help. (Although, the next few weeks are also busy for me ... so i'm not sure how much time I can contribute right away.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants