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: pkam refactor and reconnection examples #281

Merged
merged 47 commits into from
Jun 6, 2024
Merged

Conversation

JeremyTubongbanua
Copy link
Member

@JeremyTubongbanua JeremyTubongbanua commented May 24, 2024

closes #275
closes #240

- What I did

  • Refactored at_talk demo with reconnection features
  • atclient_pkam_authenticate now takes atServer host and port (instead of original root_connection)
  • ttln in notifications are now a duration instead of a timestamp
  • atclient_connection_init demands a atclient_connection_type which can either be ATCLIENT_CONNECTION_TYPE_ATDIRECTORY or ATCLIENT_CONNECTION_TYPE_ATSERVER, which is used to determine if atclient_connection_is_connected either sends a \n (for ATDIRECTORY) or noop:0 (for ATSERVER)
  • void atclient_monitor_set_read_timeout(atclient *monitor_conn, const int timeout_ms) - atclient function which sets the blocking read timeout in milliseconds. (mbedtls_ssl_read will wait X milliseconds before giving up if no bytes are read)
  • New stringutils functions that I think can (or maybe should) replace the atsign with/without at symbol functins: atclient_stringutils_atsign_with_at_symbol and atclient_stringutils_atsign_without_at_symbol
  • atclient_connection_send will try to read the ACK for at most ATCLIENT_CONNECTION_MAX_READ_TRIES times (only applies when recv is not NULL)
  • refactored secondary_connection to atserver_connection and root_connection to atdirectory_connection
  • did test_atkey_to_string test4b

New atclient_utils.h

  • int atclient_utils_find_atserver_address(const char *atdirectory_host, const int atdirectory_port, const char *atsign, char **atserver_host, int *atserver_port) - translates input (atSign and atDirectory host+port) to (atServer host+port)
  • int atclient_utils_populate_atkeys_from_homedir(atclient_atkeys *atkeys, const char *atsign) - populates atKeys struct. expects atKeys file to be in default location (~/.atsign/keys/)

- How I did it

- How to verify it

  • new at_talk example (with reconnection)
  • new reconnection example
  • reconnection example builds in CI tests
  • current connection functional tests pass

- Description for the changelog
feat: pkam refactor and reconnection examples

@JeremyTubongbanua JeremyTubongbanua self-assigned this May 24, 2024
@JeremyTubongbanua JeremyTubongbanua marked this pull request as draft May 24, 2024 20:21
@JeremyTubongbanua
Copy link
Member Author

TODO: check for any non-freed pointers

@JeremyTubongbanua
Copy link
Member Author

JeremyTubongbanua commented May 28, 2024

We have hard coded atServer address in this PR

#define EXPECTED_HOST "228aafb0-94d3-5aa2-a3b3-e36af115480d.swarm0002.atsign.zone"
#define EXPECTED_PORT 6943

I plan to fix this (and hard code an address from VE) once I get to #211

@JeremyTubongbanua JeremyTubongbanua changed the title refactor: pkam authenticate feat: Jun 4, 2024
@JeremyTubongbanua JeremyTubongbanua changed the title feat: feat: pkam refactor and reconnection examples Jun 4, 2024
@JeremyTubongbanua JeremyTubongbanua marked this pull request as ready for review June 4, 2024 20:47
@JeremyTubongbanua JeremyTubongbanua requested a review from Xlin123 June 6, 2024 12:47
@XavierChanth
Copy link
Member

I will review later today @JeremyTubongbanua

@XavierChanth
Copy link
Member

What is the reason for handling a read of 0 bytes as an error?

@JeremyTubongbanua
Copy link
Member Author

JeremyTubongbanua commented Jun 6, 2024

What is the reason for handling a read of 0 bytes as an error?

The documentation says
0 if the read end of the underlying transport was closed without sending a CloseNotify beforehand, which might happen because of various reasons (internal error of an underlying stack, non-conformant peer not sending a CloseNotify and such) - in this case you must stop using the context (see below)."

It also says

Warning: If this function returns something other than a positive value, #MBEDTLS_ERR_SSL_WANT_READ, #MBEDTLS_ERR_SSL_WANT_WRITE, #MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS, #MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS or #MBEDTLS_ERR_SSL_CLIENT_RECONNECT, you must stop using the SSL context for reading or writing, and either free it or call mbedtls_ssl_session_reset() on it before re-using it for a new connection; the current connection must be closed.

Based on those two quotes above, I essentially treat ret <= 0 as an error.

This is the full mbedtls_ssl_read function documentation:

int mbedtls_ssl_read(mbedtls_ssl_context *ssl, unsigned char *buf, size_t len)
Read at most 'len' application data bytes

Parameters:
sslSSL context
bufbuffer that will hold the data
lenmaximum number of bytes to read

Returns:
The (positive) number of bytes read if successful. 0 if the read end of the underlying transport was closed without sending a CloseNotify beforehand, which might happen because of various reasons (internal error of an underlying stack, non-conformant peer not sending a CloseNotify and such) - in this case you must stop using the context (see below). #MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY if the underlying transport is still functional, but the peer has acknowledged to not send anything anymore. #MBEDTLS_ERR_SSL_WANT_READ or #MBEDTLS_ERR_SSL_WANT_WRITE if the handshake is incomplete and waiting for data to be available for reading from or writing to the underlying transport - in this case you must call this function again when the underlying transport is ready for the operation. #MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS if an asynchronous operation is in progress (see mbedtls_ssl_conf_async_private_cb()) - in this case you must call this function again when the operation is ready. #MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS if a cryptographic operation is in progress (see mbedtls_ecp_set_max_ops()) - in this case you must call this function again to complete the handshake when you're done attending other tasks. #MBEDTLS_ERR_SSL_CLIENT_RECONNECT if we're at the server side of a DTLS connection and the client is initiating a new connection using the same source port. See below. Another SSL error code - in this case you must stop using the context (see below).

Warning:
If this function returns something other than a positive value, #MBEDTLS_ERR_SSL_WANT_READ, #MBEDTLS_ERR_SSL_WANT_WRITE, #MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS, #MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS or #MBEDTLS_ERR_SSL_CLIENT_RECONNECT, you must stop using the SSL context for reading or writing, and either free it or call mbedtls_ssl_session_reset() on it before re-using it for a new connection; the current connection must be closed.

Note:
When this function returns #MBEDTLS_ERR_SSL_CLIENT_RECONNECT (which can only happen server-side), it means that a client is initiating a new connection using the same source port. You can either treat that as a connection close and wait for the client to resend a ClientHello, or directly continue with mbedtls_ssl_handshake() with the same context (as it has been reset internally). Either way, you must make sure this is seen by the application as a new connection: application state, if any, should be reset, and most importantly the identity of the client must be checked again. WARNING: not validating the identity of the client again, or not transmitting the new identity to the application layer, would allow authentication bypass!
Remarks regarding event-driven DTLS: - If the function returns #MBEDTLS_ERR_SSL_WANT_READ, no datagram from the underlying transport layer is currently being processed, and it is safe to idle until the timer or the underlying transport signal a new event. - This function may return MBEDTLS_ERR_SSL_WANT_READ even if data was initially available on the underlying transport, as this data may have been only e.g. duplicated messages or a renegotiation request. Therefore, you must be prepared to receive MBEDTLS_ERR_SSL_WANT_READ even when reacting to an incoming-data event from the underlying transport. - On success, the datagram of the underlying transport that is currently being processed may contain further DTLS records. You should call mbedtls_ssl_check_pending to check for remaining records.

@JeremyTubongbanua JeremyTubongbanua merged commit 06021ea into trunk Jun 6, 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
2 participants