Fix skipped tests in configurations without RSA #9067

gilles-peskine-arm · 2024-04-26T19:33:51Z

Tighten the matching when detecting which certificates are in use to determine algorithm requirements. This fixes a bug whereby all tests were skipped in configurations without RSA except for an Mbed TLS client against a GnuTLS or OpenSSL server, due to *server2* matching ssl_server2. Fixes #8366.

PR checklist

changelog yes for an incidental bugfix
3.6 backport Backport 3.6: Fix skipped tests in configurations without RSA #9136
2.28 backport (not for the main issue, but for some of the subsequent changes) Backport 2.28: Fix skipped tests in configurations without RSA #9137
tests check that test-ref-configs doesn't skip all ssl-opt tests anymore

Tighten the matching when detecting which certificates are in use to determine algorithm requirements. This fixes a bug whereby all tests were skipped in configurations without RSA except for an Mbed TLS client against a GnuTLS or OpenSSL server, due to *server2* matching ssl_server2. Fixes Mbed-TLS#8366. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

ronald-cron-arm

The changes look good to me but some CI components fail now.

…bled It isn't detected on the CI because we only test this with an ancient Clang that doesn't warn. Old GCC, modern GCC and modern Clang do warn (-Wunused-but-set-variable). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

Only s_server has a -nocert option, s_client doesn't. Fixes OpenSSL client test cases in PSK-only builds. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

When given a PSK key but no username, gnutls-cli prompts for a password. Prevent that by passing --pskusername with the same identity that ssl_server2 uses by default. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

ssl-opt.sh uses a 3-byte PSK in many test cases. Unfortunately GnuTLS >=3.4.0 rejects a PSK that is less than 4 bytes long: > Error setting the PSK credentials: The request is invalid. Use a longer PSK throughout ssl-opt. Only the test cases involving GnuTLS need to change, but it's easier to do a global search-and-replace, and it's easier to not have to worry about mismatches in constructed test cases later, so replace everything. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2024-04-29T15:52:01Z

@ronald-cron-arm Yes, now that a lot of test cases are no longer unduly skipped, some of them are failing. This will probably take a few rounds of CI.

gilles-peskine-arm · 2024-04-29T17:20:09Z

As of dd191fe the log of test-ref-configs shows that each run of ssl-opt.sh is executing (not skipping) at least one test case. They're still skipping a large number of test cases, but that's expected because the tested configurations are small whereas ssl-opt.sh is mostly about testing optional features of the TLS protocol.

test-ref-configs is executing zero compat.sh test cases, but that's a separate issue, out of scope here.

gilles-peskine-arm · 2024-04-30T12:26:46Z

I'm expecting the CI to pass now.

gilles-peskine-arm · 2024-04-30T14:07:50Z

Never mind, I missed some failures in outcome analysis.

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

Some OpenSSL or GnuTLS interoperability test cases fail if the other implementation is recent enough to support TLS 1.3. Force those test cases to use TLS 1.2 so that the script works with more recent $OPENSSL or $GNUTLS_CLI or $GNUTLS_SERV than our official CI versions. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

This allows many tests to pass with the system openssl and gnutls-*. As before, not all test cases will pass due to differences between versions and build options. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

paul-elliott-arm

LGTM

ronald-cron-arm

This looks almost good to me. I have just a few questions and suggestions.

tests/ssl-opt.sh

Replace more sample PSK by longer (GnuTLS-compatible) strings, taking care of keeping distinct PSK distinct for wrong-PSK tests. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

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

ronald-cron-arm

Thanks for addressing my comments, this looks good to me.

paul-elliott-arm

LGTM

gilles-peskine-arm added bug component-tls needs-ci Needs to pass CI tests priority-high High priority - will be reviewed soon size-xs Estimated task size: extra small (a few hours at most) labels Apr 26, 2024

ronald-cron-arm self-requested a review April 29, 2024 06:41

ronald-cron-arm reviewed Apr 29, 2024

View reviewed changes

gilles-peskine-arm added 5 commits April 29, 2024 12:38

Require RSA when using server1* key or certificate

d00b93b

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

Fix PSK invocation: OpenSSL client

2776240

Only s_server has a -nocert option, s_client doesn't. Fixes OpenSSL client test cases in PSK-only builds. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix PSK invocation: GnuTLS prompting

c158fe6

When given a PSK key but no username, gnutls-cli prompts for a password. Prevent that by passing --pskusername with the same identity that ssl_server2 uses by default. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added size-s Estimated task size: small (~2d) and removed size-xs Estimated task size: extra small (a few hours at most) labels Apr 29, 2024

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 labels Apr 30, 2024

gilles-peskine-arm added needs-work and removed 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 labels Apr 30, 2024

gilles-peskine-arm added 5 commits April 30, 2024 16:20

Add some missing dependencies on crypto features

3b81ea1

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

Default NEXT versions to be the base executables

dd782f4

This allows many tests to pass with the system openssl and gnutls-*. As before, not all test cases will pass due to differences between versions and build options. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add seme missing dependencies on renegotiation support

6191f4a

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

Driver-only FFDH is not good enough for DHE support in TLS 1.2

ff3b821

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

gilles-peskine-arm force-pushed the ssl-opt-server2-detection branch from b2031dc to ff3b821 Compare April 30, 2024 14:20

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-work needs-ci Needs to pass CI tests labels Apr 30, 2024

gilles-peskine-arm requested a review from ronald-cron-arm April 30, 2024 17:25

paul-elliott-arm self-requested a review May 10, 2024 08:23

paul-elliott-arm previously approved these changes May 10, 2024

View reviewed changes

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label May 12, 2024

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 13, 2024

ronald-cron-arm requested changes May 13, 2024

View reviewed changes

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

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 added 4 commits May 13, 2024 21:06

Fix PSK invocation: GnuTLS PSK length (more)

f9f3d21

Replace more sample PSK by longer (GnuTLS-compatible) strings, taking care of keeping distinct PSK distinct for wrong-PSK tests. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Explicitly use TLS 1.2 on <=1.2-specific keyUsage/extKeyusage tests

d9c7be7

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

Remove redundant RSA dependency

f5a30af

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

Add missing dependency that isn't autodetected

fc73aa0

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

gilles-peskine-arm dismissed paul-elliott-arm’s stale review via fc73aa0 May 13, 2024 19:18

ronald-cron-arm approved these changes May 14, 2024

View reviewed changes

gilles-peskine-arm mentioned this pull request May 14, 2024

Backport 3.6: Fix skipped tests in configurations without RSA #9136

Merged

3 tasks

gilles-peskine-arm requested a review from paul-elliott-arm May 14, 2024 10:09

gilles-peskine-arm mentioned this pull request May 14, 2024

Backport 2.28: Fix skipped tests in configurations without RSA #9137

Merged

3 tasks

paul-elliott-arm approved these changes May 14, 2024

View reviewed changes

gilles-peskine-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 May 15, 2024

gilles-peskine-arm added this pull request to the merge queue May 15, 2024

Merged via the queue into Mbed-TLS:development with commit bdce657 May 15, 2024
6 checks passed

LuDuda mentioned this pull request May 21, 2024

MbedTLS v3.6.0 - Compilation errors and warnings #9156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix skipped tests in configurations without RSA #9067

Fix skipped tests in configurations without RSA #9067

gilles-peskine-arm commented Apr 26, 2024 •

edited

Loading

ronald-cron-arm left a comment

gilles-peskine-arm commented Apr 29, 2024

gilles-peskine-arm commented Apr 29, 2024

gilles-peskine-arm commented Apr 30, 2024

gilles-peskine-arm commented Apr 30, 2024

paul-elliott-arm left a comment

ronald-cron-arm left a comment

ronald-cron-arm left a comment

paul-elliott-arm left a comment

Fix skipped tests in configurations without RSA #9067

Fix skipped tests in configurations without RSA #9067

Conversation

gilles-peskine-arm commented Apr 26, 2024 • edited Loading

PR checklist

ronald-cron-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 29, 2024

gilles-peskine-arm commented Apr 29, 2024

gilles-peskine-arm commented Apr 30, 2024

gilles-peskine-arm commented Apr 30, 2024

paul-elliott-arm 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

paul-elliott-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Apr 26, 2024 •

edited

Loading