Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9563

gilles-peskine-arm · 2024-09-13T14:19:07Z

When MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled, if an Mbed TLS client or server attempted a handshake with a peer that uses compatibility mode, it would abort the handshake. Fix that: now, even with MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE disabled, Mbed TLS can communicate with e.g. a default OpenSSL or GnuTLS or Mbed TLS, as long as there is no middlebox in the way. Fixes #9551.

Prerequisite of #9541.

Content of this PR:

The very small actual fix.
Some massaging of ssl-opt test cases so that they are more suitable for automated processing.
Remove requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE except when testing middlebox compatibility mode.
Register tests/opt-testcases/tls13-compat.sh as a generated file.

Note to reviewers: the diff is large, but most of that is automated rewriting of some requires lines in ssl-opt. See the commit messages for Perl command lines.

PR checklist

changelog provided
development PR Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9628
framework PR not required
3.6 PR here
2.28 PR not required because: feature not in 2.28
tests provided

The *.sh files in opt-testcases cannot be executed directly: they can only be sourced by ssl-opt.sh. So don't make them executable and don't give them a shebang line. Also make sure that the first paragraph of each file is a short description. Signed-off-by: Gilles Peskine <[email protected]>

mpg · 2024-09-17T08:23:24Z

This looks like it might be fixing #9551 - is that the case?

gilles-peskine-arm · 2024-09-17T08:54:46Z

Yes, and there's a commit that would auto-close the issue if this pull request was targeting the repository's default branch. But I forgot to mention it in the issue description, done.

mpg

LGTM, thanks!

tests/opt-testcases/tls13-compat.sh

ronald-cron-arm

Looks almost good to me but the generation in the CMake case does not seem to work.

ronald-cron-arm · 2024-09-19T10:13:31Z

tests/scripts/generate_tls13_compat_tests.py

@@ -288,9 +285,6 @@ def update_priority_string_list(items, map_table):
        priority_string = ':+'.join(priority_string_list)
        priority_string += ':%NO_TICKETS'

-        if not self._compat_mode:


We can probably also remove requires_gnutls_next_disable_tls13_compat line 252 as well.

ronald-cron-arm · 2024-09-19T10:41:12Z

CMakeLists.txt

@@ -362,6 +362,20 @@ if(ENABLE_TESTING OR ENABLE_PROGRAMS)
        )
        add_custom_target(test_certs_header DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/tests/src/test_certs.h)
        add_dependencies(mbedtls_test test_keys_header test_certs_header)
+


I would put that rather in tests/CMakeLists.txt. Otherwise I may miss something but tls13-compat.sh does not seem to be generated in the CMake case.

I would put that rather in tests/CMakeLists.txt.

I wasn't sure, but the root file seemed to be a correct place since it's where we have the commands for test_keys.h and test_certs.h. Why is one location preferred? My understanding is that the normal thing in CMake is to have one CMakeLists.txt per directory that handles everything “in” that directory, but sometimes a build rule involves files outside that directory as well (e.g. files from the framework) and sometimes that means the rule has to be in a parent directory of all the files involved. What goes into that last sometimes?

tls13-compat.sh does not seem to be generated in the CMake case.

Indeed you have to run make tls13-compat.sh explicitly, which is a problem. What's the solution? make generated_files doesn't work.

set_target_properties(tls13-compat.sh PROPERTIES EXCLUDE_FROM_ALL NO)

seems to work. Please don't ask me why.

My understanding is that the normal thing in CMake is to have one CMakeLists.txt per directory that handles everything “in” that directory,

Maybe rather: to have one CMakeLists.txt per directory that handles what is built in that directory?

On that note the way things are working (not to be addressed by this PR, I'll try to keep that in mind and improve it) in an out of tree build (with the changes I've proposed) with tls13-compat.sh is quite interesting. There is no CMakeLists.txt in tests/opt-testcases thus CMake does not create a tests/opt-testcases in the build tree (Would we need set_target_properties(tls13-compat.sh PROPERTIES EXCLUDE_FROM_ALL NO) in that case?). But because of link_to_source(opt-testcases) in tests/CMakeLists.txt, CMake creates in the build tree the symbolic link tests/opt-testcases to the source tests/opt-testcases. That way there is actually a tests/opt-testcases in the build directory and ${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh is generated in the build tree, and in the source tree because it is a symbolic link.

ronald-cron-arm · 2024-09-19T10:52:09Z

tests/Makefile

@@ -55,6 +55,10 @@ GENERATED_DATA_FILES += $(GENERATED_PSA_DATA_FILES)
 GENERATED_FILES = $(GENERATED_DATA_FILES)
 GENERATED_FILES += src/test_keys.h src/test_certs.h

+opt-testcases/tls13-compat.sh: scripts/generate_tls13_compat_tests.py
+	$(PYTHON) scripts/generate_tls13_compat_tests.py -o $@


Suggested change

$(PYTHON) scripts/generate_tls13_compat_tests.py -o $@

echo " Gen ..."

$(PYTHON) scripts/generate_tls13_compat_tests.py -o $@

to have a log when running make generated_files as for the other generated files.

gilles-peskine-arm · 2024-09-19T17:31:27Z

tests/CMakeLists.txt

+        DEPENDS
+            ${CMAKE_CURRENT_SOURCE_DIR}/../framework/scripts/generate_test_keys.py
+    )
+    add_custom_target(tls13-compat.sh


ssl-opt which is added by #9565 should also generate tls13-compat.sh. This can only be done once both parts of the code are in — reminder to make a follow-up PR.

For better searchability and readability, call requires_config_enabled or requires_config_disabled for each option, instead of calling requires_all_configs_enabled or requires_all_configs_disabled with a long list of options. ``` perl -0777 -i -pe ' # With -0777, we act on the whole file. # s[REGEXP][CODE]egm replaces every occurrence of REGEXP by the result # of running CODE. # The regexp matches "requires_all_configs_enabled" or # "requires_all_configs_disabled" followed by a list of words ending # with a line break. The words can be separated by a sequence of # spaces and optionally a backslash-newline. s[^requires_all_configs_(enabled|disabled) *((?:(?: \w+) *(?:\\\n)? *)+)\n][ $state = $1; # Extract all the words from the list of words (/(\w+)/g). For each word, # For each word, construct a line "requires_config_XXXabled WORD". # The replacement text is the concatenation of these lines. join("", map {"requires_config_$state $_\n"} $2 =~ /(\w+)/g) ]egm' tests/ssl-opt.sh tests/opt-testcases/*.sh ``` Signed-off-by: Gilles Peskine <[email protected]>

The compile-time option MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE gates both support for interoperability with a peer that uses middlebox compatibility mode, and support for activating that mode ourselves. Change code that is only needed for interoperability to be guarded by MBEDTLS_SSL_TLS1_3_ACCEPT_COMPATIBILITY_MODE. As of this commit, MBEDTLS_SSL_TLS1_3_ACCEPT_COMPATIBILITY_MODE is always enabled: there is no way to disable it, and there are no tests with it disabled. Signed-off-by: Gilles Peskine <[email protected]>

Adapt the test cases for TLS 1.3 middlebox compatibility mode, now that we always interoperate with peers that support it, regardless of whether MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is enabled. Signed-off-by: Gilles Peskine <[email protected]>

Signed-off-by: Gilles Peskine <[email protected]>

MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is no longer required, except in test cases that are specifically about it. This commit removes the requirement on all test cases except those whose description contains "middlebox". Exclude tls13-compat.sh which is automatically generated and will be handled in a separate commit. ``` perl -0777 -i -pe ' # With -0777, we act on the whole file. # s[REGEXP][EXPR]gm replaces every occurrence of REGEXP by EXPR. # The regexp matches "requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE", # followed by zero or more non-empty lines, followed by a line starting # with "run_test" and not containing "middlebox". # The replacement is everything matched except the first line. s[^requires_config_enabled MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE\n((?:.+\n)*run_test (?!.*middlebox))] [$1]gm' tests/ssl-opt.sh tests/opt-testcases/tls13-kex-modes.sh tests/opt-testcases/tls13-misc.sh ``` Signed-off-by: Gilles Peskine <[email protected]>

MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is no longer required, except in test cases that are specifically about it. This commit removes the requirement in tls13-compat.sh (which does not have test cases that actually depend on the feature). Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2024-09-20T05:46:42Z

I've force-pushed a new version with only changes to commit messages. I added some explanations to the two commit messages with Perl code.

mpg

LGTM, thanks!

ronald-cron-arm

Some CMake things I'd prefer to address, otherwise this looks to me.

ronald-cron-arm · 2024-09-24T13:25:08Z

tests/CMakeLists.txt

+            "${MBEDTLS_PYTHON_EXECUTABLE}"
+            "${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_tls13_compat_tests.py"
+        DEPENDS
+            ${CMAKE_CURRENT_SOURCE_DIR}/../framework/scripts/generate_test_keys.py


Suggested change

${CMAKE_CURRENT_SOURCE_DIR}/../framework/scripts/generate_test_keys.py

${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_tls13_compat_tests.py

ronald-cron-arm · 2024-09-24T13:31:15Z

tests/CMakeLists.txt

@@ -163,6 +163,21 @@ if(GEN_FILES)
            ${CMAKE_CURRENT_SOURCE_DIR}/../include/psa/crypto_extra.h
    )

+    add_custom_command(
+        OUTPUT
+            ${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh


Suggested change

${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh

${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh

in case of out of tree build, the generated files are generated in the build tree.

As you could notice by the copypasta, I copied what we were doing for test_keys.h and test_certs.h. Why should tls13-compat.sh be different?

In fact I don't think we have a choice for tls13-compat.sh, not without doing some additional redesign. In the build tree, tests/opt-testcases is a symbolic link to the source tree.

I've fixed the copypasta. Changing the tree used by the generated files that aren't .data files feels out of scope here, and best done by a CMake expert. Would you be ok to file an issue for that, and leave it alone here?

I am not sure to understand why things are the way they are for test_keys.h and test_certs.h. It seems to be related to the fact that mbedtls_test is declared in the root CMakeLists.txt. I'd say that tls13-compat.sh does not have that "constraint" thus things could be done rather the way test suites are generated, I'd say.

Would you be ok to file an issue for that, and leave it alone here?

Yes that's completely fine by me.

I will create the issue tomorrow morning to unblock this PR.

I've created the issue: #9627 . Please edit it and classify it as you see fit.

Great, thanks.

ronald-cron-arm · 2024-09-24T13:31:30Z

tests/CMakeLists.txt

+            ${CMAKE_CURRENT_SOURCE_DIR}/../framework/scripts/generate_test_keys.py
+    )
+    add_custom_target(tls13-compat.sh
+        DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh)


Suggested change

DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh)

DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh)

ronald-cron-arm · 2024-09-24T13:51:56Z

CMakeLists.txt

@@ -362,6 +362,20 @@ if(ENABLE_TESTING OR ENABLE_PROGRAMS)
        )
        add_custom_target(test_certs_header DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/tests/src/test_certs.h)
        add_dependencies(mbedtls_test test_keys_header test_certs_header)
+


My understanding is that the normal thing in CMake is to have one CMakeLists.txt per directory that handles everything “in” that directory,

Maybe rather: to have one CMakeLists.txt per directory that handles what is built in that directory?

On that note the way things are working (not to be addressed by this PR, I'll try to keep that in mind and improve it) in an out of tree build (with the changes I've proposed) with tls13-compat.sh is quite interesting. There is no CMakeLists.txt in tests/opt-testcases thus CMake does not create a tests/opt-testcases in the build tree (Would we need set_target_properties(tls13-compat.sh PROPERTIES EXCLUDE_FROM_ALL NO) in that case?). But because of link_to_source(opt-testcases) in tests/CMakeLists.txt, CMake creates in the build tree the symbolic link tests/opt-testcases to the source tests/opt-testcases. That way there is actually a tests/opt-testcases in the build directory and ${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh is generated in the build tree, and in the source tree because it is a symbolic link.

Signed-off-by: Gilles Peskine <[email protected]>

ronald-cron-arm

LGTM

mpg

LGTM.

I'll note that since #9565 has been merged,#9563 (comment) could be addressed here. Or in a follow-up as you prefer.

mpg · 2024-09-25T07:49:16Z

I'll note that since #9565 has been merged,#9563 (comment) could be addressed here. Or in a follow-up as you prefer.

Well since the development PR has an additional commit addressing this, I think it should be addressed here as well for consistency.

…t-disabled-3.6

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm · 2024-09-25T07:53:57Z

I think it should be addressed here as well for consistency.

Ok, done. I added a commit to merge 3.6 (necessary, otherwise the build would break) and then I cherry-picked the new commit.

mpg · 2024-09-25T07:57:50Z

Ah, I didn't notice that the development PR had been rebased but not this one, which explained why you had done the last commit there but not here. Have you seen #9628 (comment) though? I'm happy to approve once it's addressed (possibly by telling me what I'm missing that means we don't need to do anything).

This is necessary if you haven't run `make generated_files` first. Signed-off-by: Gilles Peskine <[email protected]>

mpg

LGTM, thanks!

ronald-cron-arm

LGTM

gilles-peskine-arm added needs-ci Needs to pass CI tests component-tls13 size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Sep 13, 2024

This was referenced Sep 13, 2024

Backport 3.6: test sample programs in ssl-opt.sh #9541

Merged

Disabling MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE reduces interoperability #9551

Closed

gilles-peskine-arm force-pushed the tls13-middlebox-compat-disabled-3.6 branch 3 times, most recently from 717f20d to 1ef9071 Compare September 13, 2024 16:30

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-backports Backports are missing or are pending review and approval. needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Sep 13, 2024

gilles-peskine-arm requested a review from ronald-cron-arm September 13, 2024 18:39

mpg self-requested a review September 17, 2024 08:12

mpg previously approved these changes Sep 17, 2024

View reviewed changes

tests/opt-testcases/tls13-compat.sh Outdated Show resolved Hide resolved

ronald-cron-arm requested changes Sep 19, 2024

View reviewed changes

gilles-peskine-arm dismissed mpg’s stale review via 81b0a50 September 19, 2024 17:28

gilles-peskine-arm mentioned this pull request Sep 19, 2024

Switch from test-ref-configs.pl to separate components #9565

Merged

5 tasks

gilles-peskine-arm commented Sep 19, 2024

View reviewed changes

gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Sep 19, 2024

gilles-peskine-arm added 6 commits September 20, 2024 07:31

Remove mid-stanza blank lines

669728a

Signed-off-by: Gilles Peskine <[email protected]>

mpg previously approved these changes Sep 20, 2024

View reviewed changes

gilles-peskine-arm requested a review from ronald-cron-arm September 23, 2024 11:30

ronald-cron-arm requested changes Sep 24, 2024

View reviewed changes

Fix copypasta

c67a0ad

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm dismissed mpg’s stale review via c67a0ad September 24, 2024 15:33

gilles-peskine-arm mentioned this pull request Sep 24, 2024

CMake: some generated files are written to the source tree #9627

Open

gilles-peskine-arm requested review from ronald-cron-arm and mpg September 24, 2024 16:54

gilles-peskine-arm changed the title ~~Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled~~ Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled Sep 24, 2024

ronald-cron-arm previously approved these changes Sep 24, 2024

View reviewed changes

gilles-peskine-arm mentioned this pull request Sep 24, 2024

Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9628

Merged

5 tasks

mpg previously approved these changes Sep 25, 2024

View reviewed changes

gilles-peskine-arm added 2 commits September 25, 2024 09:52

Merge remote-tracking branch 'mbedtls-3.6' into tls13-middlebox-compa…

02f0f9f

…t-disabled-3.6

Have make ssl-opt generate tls13-compat.sh

278b417

Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm dismissed stale reviews from mpg and ronald-cron-arm via 278b417 September 25, 2024 07:52

gilles-peskine-arm requested review from mpg and ronald-cron-arm September 25, 2024 07:54

Have make ssl-opt generate tls13-compat.sh (make edition)

ae7e1c7

This is necessary if you haven't run `make generated_files` first. Signed-off-by: Gilles Peskine <[email protected]>

mpg approved these changes Sep 25, 2024

View reviewed changes

ronald-cron-arm approved these changes Sep 25, 2024

View reviewed changes

ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 25, 2024

ronald-cron-arm added this pull request to the merge queue Sep 25, 2024

Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 1dbfb4b Sep 25, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9563

Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9563

gilles-peskine-arm commented Sep 13, 2024 •

edited

Loading

mpg commented Sep 17, 2024

gilles-peskine-arm commented Sep 17, 2024 •

edited

Loading

mpg left a comment

ronald-cron-arm left a comment

ronald-cron-arm Sep 19, 2024

ronald-cron-arm Sep 19, 2024

gilles-peskine-arm Sep 19, 2024

gilles-peskine-arm Sep 19, 2024

ronald-cron-arm Sep 24, 2024 •

edited

Loading

ronald-cron-arm Sep 19, 2024

gilles-peskine-arm Sep 19, 2024

gilles-peskine-arm commented Sep 20, 2024

mpg left a comment

ronald-cron-arm left a comment

ronald-cron-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024

gilles-peskine-arm Sep 24, 2024

gilles-peskine-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024 •

edited

Loading

ronald-cron-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024

gilles-peskine-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024

ronald-cron-arm Sep 24, 2024 •

edited

Loading

ronald-cron-arm left a comment

mpg left a comment

mpg commented Sep 25, 2024

gilles-peskine-arm commented Sep 25, 2024

mpg commented Sep 25, 2024

mpg left a comment

ronald-cron-arm left a comment

	$(PYTHON) scripts/generate_tls13_compat_tests.py -o $@
	echo " Gen ..."
	$(PYTHON) scripts/generate_tls13_compat_tests.py -o $@

	${CMAKE_CURRENT_SOURCE_DIR}/../framework/scripts/generate_test_keys.py
	${CMAKE_CURRENT_SOURCE_DIR}/scripts/generate_tls13_compat_tests.py

	${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh
	${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh

	DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/opt-testcases/tls13-compat.sh)
	DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/opt-testcases/tls13-compat.sh)

Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9563

Backport 3.6: Fix interoperability when MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE is disabled #9563

Conversation

gilles-peskine-arm commented Sep 13, 2024 • edited Loading

PR checklist

mpg commented Sep 17, 2024

gilles-peskine-arm commented Sep 17, 2024 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Sep 20, 2024

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

mpg commented Sep 25, 2024

gilles-peskine-arm commented Sep 25, 2024

mpg commented Sep 25, 2024

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Sep 13, 2024 •

edited

Loading

gilles-peskine-arm commented Sep 17, 2024 •

edited

Loading

ronald-cron-arm Sep 24, 2024 •

edited

Loading

ronald-cron-arm Sep 24, 2024 •

edited

Loading

ronald-cron-arm Sep 24, 2024 •

edited

Loading