Skip to content
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.

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

Fix skipped tests in configurations without RSA #9067

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Apr 26, 2024

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

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>
@gilles-peskine-arm 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
@gilles-peskine-arm gilles-peskine-arm added this to To Do in Roadmap Board for Mbed TLS via automation Apr 26, 2024
@ronald-cron-arm ronald-cron-arm self-requested a review April 29, 2024 06:41
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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 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
Copy link
Contributor Author

I'm expecting the CI to pass now.

@gilles-peskine-arm
Copy link
Contributor Author

Never mind, I missed some failures in outcome analysis.

@gilles-peskine-arm 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
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>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm 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
@paul-elliott-arm paul-elliott-arm self-requested a review May 10, 2024 08:23
Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label May 12, 2024
@ronald-cron-arm ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label May 13, 2024
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Roadmap Board for Mbed TLS automation moved this from To Do to In Development May 13, 2024
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>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@paul-elliott-arm paul-elliott-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Roadmap Board for Mbed TLS automation moved this from In Development to Has Approval May 14, 2024
@gilles-peskine-arm 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 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
Roadmap Board for Mbed TLS automation moved this from Has Approval to Done May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-tls needs-backports Backports are missing or are pending review and approval. priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)
Development

Successfully merging this pull request may close these issues.

Spurious dependency on RSA in ssl-opt.sh
3 participants