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: configure socket and feature detect kTLS support #3808

Closed
wants to merge 13 commits into from

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Feb 6, 2023

Description of changes:

This PR adds the interface for enabling kTLS on the socket and connection. It does proper error handling (some errors are recoverable) when trying to enable kTLS.

In this PR we enable the TLS_ULP on the socket. In following PRs we will enable setting kTLS keys on the socket, and configuring the connection IO.

Callout

Created issue to track down why feature probe and s2n-tls build envs are different: #3813

Testing:

Its difficult to test this code until the kTLS feature is finished so there is no testing at this point.

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?

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

@toidiu toidiu changed the title ktls: configure socket and connection ktls: configure socket Feb 6, 2023
@github-actions github-actions bot added the s2n-core team label Feb 6, 2023
@toidiu toidiu mentioned this pull request Feb 6, 2023
32 tasks
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch from ea93bca to 52c40b0 Compare February 7, 2023 07:36
@toidiu toidiu requested review from lrstewart and goatgoose February 7, 2023 18:51
@toidiu toidiu changed the title ktls: configure socket ktls: configure socket and feature detect kTLS support Feb 7, 2023
@toidiu toidiu marked this pull request as draft February 7, 2023 20:47
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch from 52c40b0 to ea9f0ba Compare February 8, 2023 03:08
@toidiu toidiu marked this pull request as ready for review February 8, 2023 04:02
s2n.mk Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
error/s2n_errno.c Outdated Show resolved Hide resolved
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch 2 times, most recently from 4d0dc65 to 5a3441c Compare February 9, 2023 00:01
@toidiu toidiu requested a review from goatgoose February 9, 2023 00:02
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch 5 times, most recently from 0fcad1d to 8614314 Compare February 9, 2023 20:26
@toidiu toidiu requested a review from goatgoose February 10, 2023 09:59
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
error/s2n_errno.c Outdated Show resolved Hide resolved
tests/features/ktls.c Outdated Show resolved Hide resolved
s2n.mk Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tests/features/ktls.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch 2 times, most recently from 85a2fca to 770da4f Compare February 14, 2023 18:44
tls/s2n_ktls.h Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Outdated Show resolved Hide resolved
tls/s2n_ktls.c Show resolved Hide resolved
error/s2n_errno.c Outdated Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
Comment on lines 62 to 63
/* kTLS support was first added to AL2 starting in 5.10.130. */
#if (LINUX_VERSION_CODE >= KERNEL_VERSION(5, 10, 130))
Copy link
Contributor

Choose a reason for hiding this comment

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

And our CI includes AL2 > 5.10.130? This check gets exercised? You've confirmed the CI fails if you check for S2N_PLATFORM_SUPPORTS_KTLS being set instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am checking this now to see if we are able to test ktls platform supported in our CI at the moment. We will need this eventually to deliver kTLS but it doent need to happen in this PR. Will report back

Copy link
Contributor Author

@toidiu toidiu Feb 14, 2023

Choose a reason for hiding this comment

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

We are able to test for kTLS support on ubuntu instances

s2nValgrindOpenSSL111Gcc9
kTLS SUPPORTED!!!! (s2n_ktls_feature_probe_test.c:24)
  • CI Image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
  • CI run
  • branch

Copy link
Contributor Author

@toidiu toidiu Feb 15, 2023

Choose a reason for hiding this comment

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

edit:

Looks like we are also running on Ubuntu 22 and are testing it there!

We should still try and detect AL2/update it so leaving the task in the issue.


To be clear here, its possible to check this but we dont currently since out AL2 image doesnt support kTLS. We dont because of the #if (LINUX_VERSION_CODE >= KERNEL_VERSION(5, 10, 130)) check.

Solution:

  • detect and ignore AL2
  • update AL2 image to >5.10.130

Adding a task to the tracking issue: #3711

tests/unit/s2n_ktls_test.c Show resolved Hide resolved
tests/unit/s2n_ktls_test.c Outdated Show resolved Hide resolved
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch 5 times, most recently from a33f786 to 7e6e4ad Compare February 14, 2023 23:35
@toidiu toidiu requested a review from lrstewart February 14, 2023 23:54
@toidiu toidiu force-pushed the ak-ktls3_config_socket branch from 4ad768c to 5bc503d Compare February 15, 2023 00:19
@toidiu toidiu mentioned this pull request Mar 8, 2023
@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@toidiu toidiu closed this May 26, 2023
@toidiu toidiu deleted the ak-ktls3_config_socket branch June 20, 2023 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants