Skip to content

Add SSL server examples #9

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

Merged
merged 14 commits into from
Jul 12, 2023
Merged

Conversation

AnthonyGrondin
Copy link
Collaborator

@AnthonyGrondin AnthonyGrondin commented Jul 4, 2023

This PR add examples and bring changes needed to be able to use as a server.

TODO

Fix

Chores

  • Sync up with Update esp-wifi and esp-hal #8 when done
  • Add examples for esp32, esp32c3
  • Rename examples to be less confusing
  • Better document client_cert, client_key in the Certificates struct

- Update examples
- Fix server examples for esp32s3

BREAKING CHANGE: Session::new() now takes `&mut socket`, instead of `socket`
- Update examples
- Fix server examples for esp32s3

BREAKING CHANGE: Session::new() now takes `&mut socket`, instead of `socket`
@AnthonyGrondin
Copy link
Collaborator Author

After looking more into it, it looks like it would be up to the user to handle the self-signed error in the browser, as the connection is closed when it occurs.

There is also a bug with the async example, it seems to hang when the connection gets closed due to a self-signed certificate error. It doesn't return the correct res (-30592), it returns 0 so it goes into flushing data, but the connection is closed, so it hangs.

@bjoernQ
Copy link
Collaborator

bjoernQ commented Jul 6, 2023

Oh yes - probably the user just needs to choose to ignore the problem with the certificate or use command line wget/curl with the right options 👍

Regarding the async example: That definitely needs a fix then

@bjoernQ bjoernQ mentioned this pull request Jul 7, 2023
- Rename examples to be more comprehensive
- Add server examples for `esp32`, `esp32c3`
- Refactor and document Certificates struct for better comprehensiveness
@AnthonyGrondin
Copy link
Collaborator Author

AnthonyGrondin commented Jul 8, 2023

I've renamed and added the missing examples. That was a very lengthy process. I think we should merge them together, and use features for the different arch, considering most of the examples are code duplication. It becomes very long to maintain when changing the public facing API.

All objectives I wanted to fulfill in this issue are done, but there's still this async error that should get fixed.
I've documented extensively the Certificates struct, for usage in Client and Server mode.

This might need some squashing and rebasing before merging, as I've duplicate commits.

Also, for better maintainability, we should have a script that automatically update SSL certs, and put them in a folder, so that we don't have to update them when they expire.

@bjoernQ
Copy link
Collaborator

bjoernQ commented Jul 10, 2023

Good idea about the examples - maybe we should do something like we have in esp-wifi (while not everyone likes that approach)
Also, I like the idea of having a script to update the certificates.

@AnthonyGrondin
Copy link
Collaborator Author

Considering how most of the code is identical between the architectures, I wonder if we could get by, by only using the arch as a feature for the different versions. We would have to specify the build target when compiling, but this doesn't seem to be a hassle per say.

@AnthonyGrondin AnthonyGrondin marked this pull request as ready for review July 10, 2023 22:05
@bjoernQ
Copy link
Collaborator

bjoernQ commented Jul 11, 2023

Considering how most of the code is identical between the architectures, I wonder if we could get by, by only using the arch as a feature for the different versions. We would have to specify the build target when compiling, but this doesn't seem to be a hassle per say.

That's what we initially did in esp-wifi and what we still do in esp-storage.

There is a lot of conditional compilation for initialization but that is not too bad. The real ugly thing there was the problem that we cannot have dev-dependencies (like esp-println etc.) per target (which are just features) which was fine for the Xtensas which each had their own "architecture" but for the RISCV chips they are all riscv32-imc and the later ones riscv32-imac.

I totally agree that having multiple identical copies of the same example code is also not idea

Copy link
Collaborator

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM! Good job! Thanks for your contribution

@bjoernQ bjoernQ merged commit a524c39 into esp-rs:main Jul 12, 2023
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