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 binding: add support for PSK client callback #8332

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mgrojo
Copy link

@mgrojo mgrojo commented Jan 4, 2025

Description

This adds support in the Ada binding for wolfSSL_set_psk_client_callback.

Testing

Tested with:
wolfSSL/wolfssl-examples/psk/server-psk.c
after changing DTLSv1_3_Client_Method to DTLSv1_2_Client_Method to comply with the server example.

Checklist

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

Tested with:
`wolfSSL/wolfssl-examples/psk/server-psk.c`
after changing `DTLSv1_3_Client_Method` to `DTLSv1_2_Client_Method` to comply with the server example.
@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@dgarske dgarske self-assigned this Jan 6, 2025
@dgarske
Copy link
Contributor

dgarske commented Jan 6, 2025

Hi @mgrojo ,

Thank you for your code contribution. In order to consider this we will need to have an agreement on file. Can you please email support at wolfssl dot com and reference this PR requesting a contributor agreement. Please include information about your location and current project.

Thanks,
David Garske, wolfSSL

@dgarske
Copy link
Contributor

dgarske commented Jan 7, 2025

Hi @mgrojo ,

Your contributor agreement has been approved. We will review these changes and provide feedback shortly.

Okay to test

Thanks,
David Garske, wolfSSL

@dgarske
Copy link
Contributor

dgarske commented Jan 7, 2025

Tagging some of our Ada contributors for awareness : @joakim-strandberg @dalybrown @Irvise

wrapper/Ada/tls_client.adb Show resolved Hide resolved
wrapper/Ada/tls_client.adb Show resolved Hide resolved
wrapper/Ada/tls_client.adb Show resolved Hide resolved
wrapper/Ada/tls_client.adb Outdated Show resolved Hide resolved
wrapper/Ada/wolfssl.ads Show resolved Hide resolved
- Add comments for the PSK value in the example.
- Add runtime argument for executing the PSK test.
- Warn user that their callback implementation can't be in the SPARK subset.
@dgarske dgarske self-requested a review January 7, 2025 22:42
@dgarske
Copy link
Contributor

dgarske commented Jan 8, 2025

Retest this please: "FIPSv2-regression" failure.

@mgrojo
Copy link
Author

mgrojo commented Jan 8, 2025

Retest this please: "FIPSv2-regression" failure.

Where can I see that failure? Nevertheless, it's probably unrelated, since these are only additions. Could it be the same case as I have found here: #5579 (comment)?

The "pending" on Options-A and FIPSv2 Regression are Jenkins->github integration glitches.

@dgarske
Copy link
Contributor

dgarske commented Jan 8, 2025

Retest this please: "FIPSv2-regression" failure.

Where can I see that failure? Nevertheless, it's probably unrelated, since these are only additions. Could it be the same case as I have found here: #5579 (comment)?

The "pending" on Options-A and FIPSv2 Regression are Jenkins->github integration glitches.

That's from our internal Jenkins and not related to your changes. So just ignore for now.
As far as I am concerned the state of this PR is good. I'm going to have another engineer review and run some tests on it before we merge. I would also appreciate some feedback from some of our third-party ADA contributors I tagged.

@mgrojo
Copy link
Author

mgrojo commented Jan 8, 2025

I've also added the server side PSK callbacks on top of this branch. May I add it to this pull request, or should a new one be opened once this one is merged?

@dgarske
Copy link
Contributor

dgarske commented Jan 8, 2025

I've also added the server side PSK callbacks on top of this branch. May I add it to this pull request, or should a new one be opened once this one is merged?

Yes please and add on top of this existing PR. Thank you!

Plus fix location of the certificate files in the examples.

Tested with both Ada examples:
```
obj/tls_server_main --psk
obj/tls_client_main 127.0.0.1 --psk
```
@mgrojo
Copy link
Author

mgrojo commented Jan 8, 2025

Done.

@Irvise
Copy link
Contributor

Irvise commented Jan 9, 2025

I am mostly an Ada connoisseur, but by just taking a look at the source code (without checking that the C signatures do indeed match those being imported), it looks fine :) I trust Manu, the compiler and the test to be good enough. Though I would recommend to also wait for at least someone with more experience to voice their point of view.

Best regards,
Fer

@dgarske
Copy link
Contributor

dgarske commented Jan 10, 2025

Retest this please. Build was aborted.

@mgrojo
Copy link
Author

mgrojo commented Jan 10, 2025

Retest this please. Build was aborted.

I don't understand. The pull request says: "All checks have passed. 151 successful checks".

@JacobBarthelmeh
Copy link
Contributor

@mgrojo you're good, the comment from David about restarting tests is directed at our internal testing system. Which it looks like it did re-run and now has the 151 successful checks.

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.

5 participants