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

TLS layer: c->rtls to optimise recvd TLS data #2523

Merged
merged 1 commit into from
Dec 10, 2023
Merged

TLS layer: c->rtls to optimise recvd TLS data #2523

merged 1 commit into from
Dec 10, 2023

Conversation

cpq
Copy link
Member

@cpq cpq commented Dec 9, 2023

The reason for this PR is to minimise the amount of data copies for the TLS case. TLDR: raw data is now received in the c->rtls IO buffer, and can be accessed directly.

So, receive path:
stack receives new raw data -> c->rtls -> (TLS decrypt) -> c->recv

Note that the send path does not have the same. Apparently, TLS implementation can just encrypt data (mg_tls_send()) data on-the-fly.

Right now, the builtin receive path looks like this:

  1. Stack. Packet data gets copied to the s->raw IO buffer
  2. Stack. Call mg_tls_recv
  3. TLS. mg_tls_recv copies data in its own recv buffer
  4. TLS. mg_tls_recv decrypts the data and returns
  5. Stack gets decrypted data and copies into c->recv

This PR unifies receive path to all TLS implementations by creating c->rtls IO buffer for the raw, encrypted data.

This change required OpenSSL implementation be changed, because OpenSSL accessed an underlying socket descriptor directly. Instead, OpenSSL must use our own low-level send/recv primitives for the low level IO - just like mbedTLS does. So, OpenSSL code was refactored: a custom BIO is created that implements the IO rather that relying on the socket. Theoretically, it makes it possible to use OpenSSL with the builtin stack - which is unlikely anyways.

@cpq cpq requested a review from scaprile December 10, 2023 11:50
Copy link
Collaborator

@scaprile scaprile left a comment

Choose a reason for hiding this comment

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

AFAICU, LGTM

Comment on lines 64 to 67
if (tls != NULL) {
mg_iobuf_free(&tls->send);
mg_iobuf_free(&tls->recv);
// mg_iobuf_free(&tls->send);
// mg_iobuf_free(&tls->recv);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, just a leftover. tls_builtin.c will be totally replaced soon, so does not matter, really.

@scaprile scaprile merged commit 45d7f4c into master Dec 10, 2023
130 checks passed
@scaprile scaprile deleted the am branch December 10, 2023 17:54
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