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

feat: move mbedtls sockets into its own layer under connection #475

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

XavierChanth
Copy link
Member

@XavierChanth XavierChanth commented Dec 13, 2024

- What I did

  • Moved TLS sockets into a generic interface

    • Connection responsible for managing connections to atDirectory/atServer and parsing/stripping prompts from those connections
    • TLS sockets responsible for establishing TLS connections, and reading/writing raw bytes to and from those sockets
  • TLS socket implementation can be swapped out without the need to reimplement all of connection which also contains a bunch of parsing logic

- How I did it

- How to verify it

- Description for the changelog
feat: move mbedtls sockets into its own layer under connection

*
* @param socket The socket structure to free resources from
*/
void atclient_raw_socket_free(struct atclient_raw_socket *socket);
Copy link
Member Author

Choose a reason for hiding this comment

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

Raw sockets only have init and free for now. I plan to add more functionality later, but all we need is init and free right now, as the ssl context takes ownership of the object.

#ifndef ATCLIENT_CONNECTION_H
#define ATCLIENT_CONNECTION_H
#ifdef __cplusplus
extern "C" {
#endif

#include "atchops/mbedtls.h"
#include "atclient/connection_hooks.h"
#include <atchops/platform.h> // IWYU pragma: keep
Copy link
Member Author

Choose a reason for hiding this comment

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

platform needs to be first, as socket depends on a value in it.

Comment on lines -21 to +39

uint16_t port; // example: 64
bool _is_host_initialized : 1;
char *host; // example: "root.atsign.org"

bool _is_port_initialized : 1;
uint16_t port; // example: 64
bool _is_connection_enabled : 1;
bool _is_hooks_enabled : 1;

char *host; // example: "root.atsign.org"
Copy link
Member Author

Choose a reason for hiding this comment

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

Grouped the booleans in this struct to marginally improve the alignment. Since the alignment is quite high, should help reduce the chances of cache misses

@cpswan cpswan marked this pull request as draft December 17, 2024 10:01
@XavierChanth XavierChanth requested a review from cpswan December 17, 2024 14:08
@XavierChanth
Copy link
Member Author

@cpswan it's worth discussing how we want to test this. It is tested through functional tests with notify, monitor, connection, pkam_authenticate & get_atkeys. But we now have the ability to test the pure tls socket client, we would need to create a test harness though.

cpswan
cpswan previously approved these changes Dec 17, 2024
@cpswan cpswan marked this pull request as ready for review December 17, 2024 14:49
@cpswan
Copy link
Member

cpswan commented Dec 17, 2024

@XavierChanth agree that more tests would be good, though maybe open a fresh issue for those.

@XavierChanth XavierChanth force-pushed the feat-socket-layer branch 3 times, most recently from 9599ed8 to c14e6d0 Compare December 18, 2024 20:55
@XavierChanth XavierChanth merged commit 247ac4b into trunk Dec 19, 2024
8 checks passed
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.

3 participants