Allow TLS 1.2 servers to report client versions from the supported versions extension #4249
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goatgoose
force-pushed
the
supported-versions-fix-3
branch
2 times, most recently
from
ec4d8e2 to
2b3c9b2
Compare
…rsions extension Squashed commit of the following: commit 2b3c9b2 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Oct 17 16:01:14 2023 -0400 fixes commit 3e27ec8 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Oct 17 15:31:45 2023 -0400 update integ test to expect TLS 1.3 commit b66a361 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Oct 17 11:44:41 2023 -0400 fixes commit c3b120b Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Tue Oct 17 11:38:17 2023 -0400 TLS 1.2 fallback tests commit 4245e3e Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Oct 16 16:54:54 2023 -0400 add test for client getters commit 7e8e734 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Oct 16 14:35:05 2023 -0400 add test for server getters with no supported versions extension commit 9f63693 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Oct 16 14:09:27 2023 -0400 test for protocol getters on server with a received supported versions extension commit 08d3d78 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Mon Oct 16 10:29:51 2023 -0400 add supported versions RFC exception for legacy client hello version commit 4026318 Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Fri Oct 13 11:37:45 2023 -0400 parse supported versions extension in getter commit 4ee56eb Author: Sam Clark <3758302+goatgoose@users.noreply.github.com> Date: Fri Oct 13 10:17:13 2023 -0400 refactor supported versions process
goatgoose
force-pushed
the
supported-versions-fix-3
branch
from
2b3c9b2 to
78bd1a7
Compare
maddeleine
reviewed
Oct 18, 2023
maddeleine
approved these changes
Oct 19, 2023
lrstewart
reviewed
Oct 19, 2023
goatgoose
force-pushed
the
supported-versions-fix-3
branch
from
407470c to
949cd55
Compare
lrstewart
approved these changes
Oct 24, 2023
lrstewart
approved these changes
Oct 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved issues:
Resolves #4158
Description of changes:
Currently, TLS 1.2 servers ignore the supported versions extension if it exists in the client hello. This means that the client protocol version is always determined by the client hello version rather than a version in the supported versions extension. This can lead to incorrect reporting of the client protocol version from the
s2n_connection_get_client_protocol_version()API on TLS 1.2 servers. For example, a TLS 1.3 client that sends a supported versions extension containing TLS 1.3 would be reported as TLS 1.2 on TLS 1.2 servers, and TLS 1.3 on TLS 1.3 servers.This PR allows the supported versions extension to be processed on TLS 1.2 servers before returning a value from the client protocol version getter to ensure that the correct client protocol version is reported.
Call-outs:
An alternative to this PR would be to update the
client_protocol_versionfield on the connection based on the supported versions extension and use this value for protocol version selection, similar to TLS 1.3 servers. However, this could result in a behavior change, so it was decided to not make this change for now. This is discussed in #4240.Testing:
New unit tests to test the new behavior of
s2n_connection_get_client_protocol_version(). Also, there weren't any tests for the other protocol version getters so I added tests for those as well.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.