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.

Sign up for GitHub

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

Jump to bottom

ssl-opt: improve PSK mode detection #9556

Merged

gilles-peskine-arm merged 14 commits into Mbed-TLS:development from gilles-peskine-arm:ssl-opt-psk-detection-development

Sep 13, 2024

Contributor

gilles-peskine-arm commented Sep 11, 2024 •

edited

Loading

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

Straight forward port of #9546.

PR checklist

changelog not required because: test only
development PR here
framework PR ssl-opt: improve PSK mode detection mbedtls-framework#44
3.6 PR [3.6] ssl-opt: improve PSK mode detection #9546
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 8c95999b3828f1b3e7a069e61c2b33b9f509e446.outcomes.csv.xz https://mbedtls.trustedfirmware.org/job/mbed-tls-nightly-tests/942/artifact/outcomes.csv.xz
wget -O 1.outcomes.csv.xz https://mbedtls.trustedfirmware.org/job/mbed-tls-pr-head/job/PR-9556-head/1/artifact/outcomes.csv.xz
comm -23 <(xzcat 8c95999b3828f1b3e7a069e61c2b33b9f509e446.outcomes.csv.xz | grep -v ';component_release_' | sort) <(xzcat 1.outcomes.csv.xz | sort) | grep -v ';SKIP;'

gilles-peskine-arm added component-tls needs-ci size-s labels

gilles-peskine-arm mentioned this pull request

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

Merged

5 tasks

gilles-peskine-arm added needs-review needs-backports and removed needs-ci labels

This was referenced Sep 11, 2024

Skipped test cases in ssl-opt.sh in TLS 1.3-only configurations #9560

Open

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

Merged

gilles-peskine-arm requested review from mpg and ronald-cron-arm

September 12, 2024 16:11

ronald-cron-arm previously approved these changes

View reviewed changes

Contributor

ronald-cron-arm left a comment

LGTM but it seems you need to rebase the framework PR.

Contributor Author

gilles-peskine-arm commented Sep 13, 2024

I'll need to rewrite the history to amend the submodule update commit, anyway, once the framework PR is merged.

How about I do that now, since the framework PR is approved and has passed the CI on both branches?

mpg previously approved these changes

View reviewed changes

Contributor

mpg left a comment

LGTM but there's a conflict in framework (also in the 3.6 PR now).

mpg removed the needs-review label

gilles-peskine-arm mentioned this pull request

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

Merged

gilles-peskine-arm dismissed stale reviews from mpg and ronald-cron-arm via

52584e2

September 13, 2024 07:47

gilles-peskine-arm force-pushed the ssl-opt-psk-detection-development branch from 27c7055 to 52584e2 Compare

September 13, 2024 07:47

gilles-peskine-arm added 11 commits

September 13, 2024 09:48


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

56ee69d

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Use CONFIGS_ENABLED instead of repeatedly calling query_compile_time_…

…config

It's faster and more readable.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          ssl-opt: Fix GnuTLS PSK injection

6f9952a

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Detect more cases where certificates are required

ae3dc17

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 <Gilles.Peskine@arm.com>


          Fix PSK-only mode doing less than it should

4f09864

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 <Gilles.Peskine@arm.com>


          Detect PSK-only mode in TLS 1.3 as well

24b4303

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Unify the two requires-key-exchange-with-certificate function

9d3b207

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 <Gilles.Peskine@arm.com>


          Also activate PSK-only mode when PSK-ephemeral key exchanges are avai…

db26406

…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 <Gilles.Peskine@arm.com>


          Fix weirdly quoted invocations of requires_any_configs_enabled

cd4fe70

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Remove unused auth_mode parameter on a PSK test case

78df617

It was causing the test case to be incorrectly skipped as needing
certificate authentication.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Improve some comments

7f453bf

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added 3 commits

September 13, 2024 09:48


          Fix detection of TLS 1.2 PSK-ephemeral key exchange modes

2dd4395

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          Documentation improvements

6e85e35

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>


          requires_certificate_authentication: prioritize TLS 1.3

e3eab32

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 <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the ssl-opt-psk-detection-development branch from 52584e2 to e3eab32 Compare

September 13, 2024 07:48

Contributor Author

gilles-peskine-arm commented Sep 13, 2024

I've rebased both this PR and the 3.6 one to get the latest framework updates.

mpg approved these changes

View reviewed changes

Contributor

mpg left a comment

The rebase looks good to me.

ronald-cron-arm self-requested a review

September 13, 2024 08:10

ronald-cron-arm approved these changes

View reviewed changes

Contributor

ronald-cron-arm left a comment

LGTM

gilles-peskine-arm added the approved label

gilles-peskine-arm added this pull request to the merge queue

Merged via the queue into Mbed-TLS:development with commit e16aecc

4 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved component-tls needs-backports size-s