[3.6] ssl-opt: improve PSK mode detection #9546

gilles-peskine-arm · 2024-09-06T13:48:43Z

Fix some issues in ssl-opt.sh around requirement detections, mostly related to PSK vs certificates. Prerequisite of #9541.

PR checklist

changelog not required because: test only
development PR ssl-opt: improve PSK mode detection #9556
framework PR ssl-opt: improve PSK mode detection mbedtls-framework#44
3.6 PR here
2.28 PR not required because: it's mostly about TLS 1.3 changes, and not worth backporting the few changes that aren't.

tests provided + check that we aren't executing fewer test cases, compared with the baseline outcomes.csv.xz (excluding ;component_release_ which are not run in the PR job). The following command lists the test cases that have changed, excluding those that used to be skipped; its output is empty.

wget -O d210bf73b2ff8adb237002446a520be16f8b3210.outcomes.csv.xz https://mbedtls.trustedfirmware.org/job/mbed-tls-nightly-tests/928/artifact/outcomes.csv.xz
wget -O 3.outcomes.xz https://mbedtls.trustedfirmware.org/job/mbed-tls-pr-head/job/PR-9546-head/3/artifact/outcomes.csv.xz
comm -23 <(xzcat d210bf73b2ff8adb237002446a520be16f8b3210.outcomes.csv.xz | grep -v ';component_release_' | sort) <(xzcat 3.outcomes.csv.xz | sort) | grep -v ';SKIP;'

mpg

Looks pretty good to me, thanks for the clean-up! Other than the XXX-PSK vs PSK-XXX thing, I only have questions / requests for adding comments.

tests/ssl-opt.sh

mpg

LGTM, thanks for addressing my comments.

ronald-cron-arm

This looks almost good to me. Minor comments and one thing I am not sure about.

tests/ssl-opt.sh

ronald-cron-arm

LGTM, thanks.

mpg

LGTM, thanks! 53907c7

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

…config It's faster and more readable. Signed-off-by: Gilles Peskine <[email protected]>

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

When requiring a cryptographic mechanism for the sake of certificate authentication, also require that certificate authentication is enabled. Setting auth_mode explicitly means that we're testing something related to how certificate-based authentication is handled, so require a key exchange with certificate-based authentication. Signed-off-by: Gilles Peskine <[email protected]>

Don't add a certificate requirement when PSK is enabled. Do command line requirement detection after the injection of PSK into the command line in PSK-only mode. Otherwise certificate requirements would be added even in PSK-only mode. Signed-off-by: Gilles Peskine <[email protected]>

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

requires_certificate_authentication was called in more places, but did not do fine-grained analysis of key exchanges and so gave the wrong results in some builds. requires_key_exchange_with_cert_in_tls12_or_tls13_enabled gave the correct result but was only used in some test cases, not in the automatic detection code. Remove all uses of requires_key_exchange_with_cert_in_tls12_or_tls13_enabled because they are in fact covered by automated detection that calls requires_certificate_authentication. Signed-off-by: Gilles Peskine <[email protected]>

…lable The point of PSK-only mode is to transform certificate-based command lines into PSK-based command lines, when the certificates are not relevant to what is being tested. So it makes sense to do that in with PSK-ephemeral key exchanges too. Signed-off-by: Gilles Peskine <[email protected]>

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

It was causing the test case to be incorrectly skipped as needing certificate authentication. Signed-off-by: Gilles Peskine <[email protected]>

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

When checking whether the build supports certificate authentication, check the key exchange modes enabled in the default protocol version. This is TLS 1.3 when it's enabled. Signed-off-by: Gilles Peskine <[email protected]>

mpg

The rebase looks good to me.

ronald-cron-arm

LGTM

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

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

ssl-opt: improve PSK mode detection Mbed-TLS/mbedtls-framework#44

Merged

gilles-peskine-arm force-pushed the ssl-opt-psk-detection-3.6 branch from 609b062 to b8ced9c Compare September 6, 2024 17:10

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Sep 8, 2024

mpg changed the title ~~ssl-opt: improve PSK mode detection~~ [3.6] ssl-opt: improve PSK mode detection Sep 9, 2024

mpg requested changes Sep 9, 2024

View reviewed changes

tests/ssl-opt.sh Outdated Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

tests/ssl-opt.sh Outdated Show resolved Hide resolved

gilles-peskine-arm requested a review from mpg September 9, 2024 09:24

mpg previously approved these changes Sep 9, 2024

View reviewed changes

ronald-cron-arm assigned ronald-cron-arm and unassigned ronald-cron-arm Sep 9, 2024

ronald-cron-arm self-requested a review September 9, 2024 12:14

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

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

Disabling MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MODE reduces interoperability #9551

Closed

ronald-cron-arm requested changes Sep 10, 2024

View reviewed changes

tests/ssl-opt.sh Show resolved Hide resolved

tests/ssl-opt.sh Outdated Show resolved Hide resolved

tests/ssl-opt.sh Show resolved Hide resolved

gilles-peskine-arm dismissed mpg’s stale review via 53907c7 September 10, 2024 10:25

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

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

Merged

7 tasks

ronald-cron-arm requested review from ronald-cron-arm and mpg September 10, 2024 12:07

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

View reviewed changes

mpg previously approved these changes Sep 11, 2024

View reviewed changes

mpg added approved Design and code approved - may be waiting for CI or backports needs-backports Backports are missing or are pending review and approval. and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 11, 2024

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

ssl-opt: improve PSK mode detection #9556

Merged

5 tasks

ronald-cron-arm mentioned this pull request Sep 11, 2024

Split mbedtls_config.h #9559

Closed

gilles-peskine-arm dismissed stale reviews from mpg and ronald-cron-arm via fbdc623 September 13, 2024 07:42

gilles-peskine-arm force-pushed the ssl-opt-psk-detection-3.6 branch from 53907c7 to fbdc623 Compare September 13, 2024 07:42

gilles-peskine-arm added 14 commits September 13, 2024 09:48

Fix "Renegotiation: openssl server, client-initiated" with OpenSSL 3

ed8cc46

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

Use CONFIGS_ENABLED instead of repeatedly calling query_compile_time_…

0bc5729

…config It's faster and more readable. Signed-off-by: Gilles Peskine <[email protected]>

ssl-opt: Fix GnuTLS PSK injection

9cd5848

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

Detect PSK-only mode in TLS 1.3 as well

bbdc1a3

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

Fix weirdly quoted invocations of requires_any_configs_enabled

07e24e9

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

Remove unused auth_mode parameter on a PSK test case

4c1347c

It was causing the test case to be incorrectly skipped as needing certificate authentication. Signed-off-by: Gilles Peskine <[email protected]>

Improve some comments

5838a64

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

Fix detection of TLS 1.2 PSK-ephemeral key exchange modes

19c60d2

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

Documentation improvements

d57212e

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

requires_certificate_authentication: prioritize TLS 1.3

cfbaffd

When checking whether the build supports certificate authentication, check the key exchange modes enabled in the default protocol version. This is TLS 1.3 when it's enabled. Signed-off-by: Gilles Peskine <[email protected]>

gilles-peskine-arm force-pushed the ssl-opt-psk-detection-3.6 branch from fbdc623 to cfbaffd Compare September 13, 2024 07:49

mpg approved these changes Sep 13, 2024

View reviewed changes

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

View reviewed changes

gilles-peskine-arm added this pull request to the merge queue Sep 13, 2024

Merged via the queue into Mbed-TLS:mbedtls-3.6 with commit 78b1362 Sep 13, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.6] ssl-opt: improve PSK mode detection #9546

[3.6] ssl-opt: improve PSK mode detection #9546

gilles-peskine-arm commented Sep 6, 2024 •

edited

Loading

mpg left a comment

mpg left a comment

ronald-cron-arm left a comment

ronald-cron-arm left a comment

mpg left a comment •

edited

Loading

mpg left a comment

ronald-cron-arm left a comment

[3.6] ssl-opt: improve PSK mode detection #9546

[3.6] ssl-opt: improve PSK mode detection #9546

Conversation

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

PR checklist

mpg left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

mpg left a comment • edited Loading

Choose a reason for hiding this comment

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 6, 2024 •

edited

Loading

mpg left a comment •

edited

Loading