botan icon indicating copy to clipboard operation
botan copied to clipboard

Consolidation and Enhancement of BSD Socket Layer

Open KaganCanSit opened this issue 11 months ago • 5 comments

Hello Botan Development Team,

In reference to Issue #2216, I have undertaken the consolidation and enhancement of the BSD socket layer. This endeavor involved refactoring redundant code within socket.cpp, socket_udp.cpp, and cli/socket_utils.h to foster a more streamlined and maintainable architecture.

Post-refactoring, the codebase compiles successfully, and the botan-test suite executes without errors. However, I am encountering challenges in testing the tls-server and tls-client applications. I would greatly appreciate guidance and assistance in effectively conducting these tests.

As a newcomer to the Botan project, I am impressed by its adherence to high C++ standards, utilization of CMake, and comprehensive documentation via Doxygen. I am eager to contribute to both my personal development and the project's advancement.

I recognize that further simplification could be applied to the files in question. Nonetheless, I am aware that extensive modifications can complicate analysis and review processes. Following this PR, I am open to implementing additional refinements based on your guidance.

I acknowledge that there may be errors in my contributions and am open to making necessary changes. I welcome any feedback and am willing to collaborate to ensure the highest quality of code.

Note: I checked the necessary files with clang-format to ensure that they comply with the file standards. However, despite updating to the latest version, I still received warnings on Debian 12 about "RequiresClausePositin and IndentRequiresClause", so I removed them.

Thank you for considering my contribution.

Sincerely

KaganCanSit avatar Feb 09 '25 17:02 KaganCanSit

Hi Kağan! Thanks a lot for looking into this. I didn't look into your code so far, just a quick reply to:

I am encountering challenges in testing the tls-server and tls-client applications. I would greatly appreciate guidance and assistance in effectively conducting these tests.

You can use those CLI-tools to quickly set up and test a TLS client and/or server. It makes sense to test those extensively, as they make use of the socket abstraction API, as you know.

The easiest way to test them is via the CLI-tests. For that, just build the library as usual and enable the CLI. Eg. like that:

 ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli --build-tool=ninja
ninja cli

The CLI-tests are implemented as a bunch of python functions in src/scripts/test_cli.py. To run the TLS-client/server specific one just go for:

python3 src/scripts/test_cli.py ./botan cli_tls_socket_tests

This runs several tests with a Botan tls_client connecting to a Botan tls_server with different protocol configurations. This should be enough to effectively test those CLI tools.

reneme avatar Feb 11 '25 07:02 reneme

Thanks, @reneme. Your guidance was extremely helpful.

I reviewed and updated my ninja and ccache tools:

sudo apt-get upgrade ninja-build sudo apt-get upgrade ccache

Then, after compiling the project, I ran the command you shared: python3 src/scripts/test_cli.py ./botan cli_tls_socket_tests

The result seems to be successful:

INFO: Ran cli_tls_socket_tests in 0.91 sec 5 tests were run in 0.90 seconds with 0 failures

I would like to state that I am open to doing additional testing and improvements. After reviewing the code content, we can refactor the code for readability if necessary. I am open to additional feedback.

Thanks again for your time and support.

Note: I also want to add. There are still repetitive structures in these sections. But I know that major edits will not be accepted. If this PR goes well and I embrace the scope a little more, we can make another simplification with your guidance.


I have found the time to look at this problem again. I see that 19 out of 38 tests failed, but the Nightly build completed successfully. I will look for errors and try to fix them. Then if you specify any changes you want to make to the parts we reviewed, I will try to make them.

First I look this -> InternalError: No header guard start found in ./src/lib/utils/socket/socket_platform.h

KaganCanSit avatar Feb 11 '25 16:02 KaganCanSit

Coverage Status

coverage: 90.648% (-0.004%) from 90.652% when pulling 1b04770609a873f39a26dc2cadc8db8da1642dad on KaganCanSit:feature into a2a0b43c3811f9f2a44d271fe590040c72119703 on randombit:master.

coveralls avatar Feb 18 '25 17:02 coveralls

Hello again @reneme,

I noticed my mistake at a few points and corrected it. Right now, all the compilations and analyses like valgrind seem to be successful. It seems that the branch can move from draft to review. As I said in my first comment, I think we can manage the process more cleanly and with fewer lines here. However, I am undecided because I am not familiar with the project.

If you want to make edits and improvements, I can do my best and spend the rest of my free time. I am open to learning.

Thanks for sharing your knowledge to me, I am just a comment away. Have a nice day.

KaganCanSit avatar Feb 20 '25 07:02 KaganCanSit

Thank you for sharing your knowledge with me and guiding me. I have gained new knowledge. @randombit can be merged after checking. I am just a message away if new edits are needed.

Good day and see you for new merge requests.

KaganCanSit avatar Mar 11 '25 15:03 KaganCanSit