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

ktls: ktls interface and building blocks #3707

Closed
wants to merge 1 commit into from

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Dec 14, 2022

#3711

Description of changes:

This PR introduces the interface for the kTLS feature. The interface is currently not public but will be release as an unstable feature (see s2n_ktls.h for the public APIs).

There are two concepts which capture the kTLS work flow:

  • requesting kTLS on a config
  • marking kTLS as enabled on the connection

The application requests that kTLS be enabled (on config), but this is not always possible since it requires kernel support and enabling the kTLS kernel module. The library can then attempt to enable kTLS but might not succeed. If successful then we mark kTLS as enabled on the connection and use that to send/recv.

Since, kTLS can be enabled for the Send, Recv or Duplex (both sockets), or Disabled, we introduce s2n_ktls_mode, which captures the combination of these states.

Callouts:

  • s2n_config_enable: the application is actually requesting kTLS not enabling it. But being on the config I think this is a fine distinction and enable matches GnuTLS apis also.
  • We currently use binary arithmetic on s2n_ktls_mode values. We generally dont want our public APIs to have binary values. Since we only have 4 states (0,1,2,3) this is not apparent, making it a 2-way door. We also dont expect any additional states for kTLS anytime soon so I prefer this option for the moment.

Testing:

Test functionality around comparing and toggling s2n_ktls_mode on the config and connection.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Dec 14, 2022
@toidiu toidiu requested a review from lrstewart December 14, 2022 00:43
@toidiu toidiu marked this pull request as ready for review December 14, 2022 06:47
@toidiu toidiu changed the title ktls: introduce ktls interface ktls: ktls interface and api Dec 14, 2022
@toidiu toidiu changed the title ktls: ktls interface and api ktls: ktls interface and building blocks Dec 14, 2022
@toidiu toidiu mentioned this pull request Dec 14, 2022
32 tasks
@toidiu toidiu force-pushed the ak-ktlsSkeleton branch 2 times, most recently from 9ee130d to eca0281 Compare December 16, 2022 08:20
*/
S2N_RESULT s2n_ktls_enable(struct s2n_connection *conn, s2n_ktls_mode mode)
{
/* TODO perform managed_send_io and managed_recv_io checks */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file at this point in the project? Seems like there's nothing in here that we need yet. What purpose is this function serving?


#include "utils/s2n_result.h"

/* --- unstable API ---
Copy link
Contributor

Choose a reason for hiding this comment

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

Who is this comment for? No customer can use these functions, they're not in s2n.h.

S2N_KTLS_MODE_SEND = 1,
/* Enable kTLS for the recv socket. */
S2N_KTLS_MODE_RECV = 2,
/* Enable kTLS for both rx and tx socket. */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an annoying nit, but be consistent with whatever you want to call these sockets.

Suggested change
/* Enable kTLS for both rx and tx socket. */
/* Enable kTLS for both send and recv socket. */

S2N_KTLS_MODE_DUPLEX = 3,
} s2n_ktls_mode;

int s2n_config_ktls_enable(struct s2n_config *config, s2n_ktls_mode ktls_mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int s2n_config_ktls_enable(struct s2n_config *config, s2n_ktls_mode ktls_mode);
int s2n_config_set_ktls_mode(struct s2n_config *config, s2n_ktls_mode ktls_mode);

I feel like this is more in line with how s2n-tls names functions in this file.

if (ktls_mode == S2N_KTLS_MODE_DISABLED) {
return config->ktls_mode_requested == S2N_KTLS_MODE_DISABLED;
}
return config->ktls_mode_requested & ktls_mode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay maybe I don't fully understand the purpose of this function, but if it's to tell the user which mode is set, why not just:

int s2n_config_get_ktls_mode(struct s2n_config *config, s2n_ktls_mode *ktls_mode)
{
    POSIX_ENSURE_REF(config);
    *ktls_mode = config->ktls_mode_requested;
    return S2N_SUCCESS;
}

@@ -1067,3 +1070,26 @@ int s2n_config_set_recv_multi_record(struct s2n_config *config, bool enabled)

return S2N_SUCCESS;
}

/* Indicates if the connection should attempt to enable kTLS. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really sure this comment is accurate. The function enables ktls, it doesn't indicate anything.

*
* Note: currently, kTLS cannot be disabled once enabled.
*/
S2N_RESULT s2n_connection_mark_ktls_enabled(struct s2n_connection *conn, s2n_ktls_mode ktls_mode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused. Are both of these functions that you've added APIs? Why is only one declared in connection.h?

@toidiu toidiu closed this Jan 31, 2023
@toidiu toidiu deleted the ak-ktlsSkeleton branch May 25, 2023 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants