compat.sh: Skip static ECDH cases if unsupported in openssl #7137

lpy4105 · 2023-02-20T10:30:18Z

Description

This PR add the ability in compat.sh to check if the under testing openssl program supports static ECDH (i.e. ECDH_ECDSA) key exchange methods. The cases for TLS_ECDH_ECDSA_xxx ciphersuites will be skipped if openssl doesn't support so.

This PR would like to cover all out of box failures of ssl-opt.sh and compat.sh on modern systems (e.g Ubuntu 18.04 and newer).

Gatekeeper checklist

changelog provided, or not required
backport Backport 2.28: compat.sh: Skip static ECDH cases if unsupported in openssl #7153
tests provided, or not required

This commit add support to detect if openssl used for testing supports static ECDH key exchange. Skip the ciphersutes if openssl doesn't support them. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yanrayw

The code looks good to me, except for a minor improvement.

tests/compat.sh

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yanrayw

LGTM

gilles-peskine-arm

We're still not skipping enough.

tests/compat.sh

gilles-peskine-arm · 2023-02-21T22:41:56Z

tests/compat.sh

@@ -534,6 +534,15 @@ add_mbedtls_ciphersuites()
    esac
 }



I know it wasn't a requirement of #1785, but since compat.sh is now skipping static ECDH cipher suites automatically, I think we should remove the explicit exclusions in .travis.yml and all.sh.

No explicit exclusions found in all.sh, so I only removed that in .travis.yml.

I also found this doesn't work for a more recent OpenSSL (e.g 1.1.1f on Travis CI).

mbedtls/tests/compat.sh

Lines 823 to 824 in ffb92b0

if grep 'Cipher is (NONE)' $CLI_OUT >/dev/null; then

RESULT=1

Actually, the comment (which was added later by a different person) is wrong. Cipher is (NONE) indicates a null cipher, not a cipher that isn't supported, and that's still true at least in OpenSSL 1.1.1. So what this piece of code really does is to automatically mark null cipher suites as skipped if OpenSSL doesn't support them.

I don't think we need to change the code: it's working well enough. But before we forget, please fix the comment.

OK, I see. But Cipher is (NONE) is also printed if there is a invalid ciphersuite in old OpenSSL. So I misunderstood the code here. Let me update the comment.

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

The mechanism of detecting unsupported ciphersuites for OpenSSL client doesn't work on a modern OpenSSL. At least, it fails on Travis CI which is installed with OpenSSL 1.1.1f. So we need to skip ECDH cipher- suites for O->m. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yanrayw

Left a potential issue to consider.

.travis.yml

tests/compat.sh

gilles-peskine-arm

Looks good to me, except, please update the preexisting incorrect comment.

gilles-peskine-arm · 2023-02-22T21:42:42Z

tests/compat.sh

@@ -534,6 +534,15 @@ add_mbedtls_ciphersuites()
    esac
 }



Actually, the comment (which was added later by a different person) is wrong. Cipher is (NONE) indicates a null cipher, not a cipher that isn't supported, and that's still true at least in OpenSSL 1.1.1. So what this piece of code really does is to automatically mark null cipher suites as skipped if OpenSSL doesn't support them.

I don't think we need to change the code: it's working well enough. But before we forget, please fix the comment.

tests/compat.sh

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

yanrayw

LGTM

gilles-peskine-arm

LGTM

compat.sh: skip static ECDH cases if unsupported in openssl

1c0e4c0

This commit add support to detect if openssl used for testing supports static ECDH key exchange. Skip the ciphersutes if openssl doesn't support them. Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 requested a review from gilles-peskine-arm February 20, 2023 10:30

yanrayw reviewed Feb 21, 2023

View reviewed changes

tests/compat.sh Show resolved Hide resolved

Only use standard cipher name

5e780df

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 requested a review from yanrayw February 21, 2023 08:30

yanrayw previously approved these changes Feb 21, 2023

View reviewed changes

gilles-peskine-arm requested changes Feb 21, 2023

View reviewed changes

lpy4105 added 2 commits February 22, 2023 10:19

compat.sh: Skip all *ECDH_* ciphersuites

a64c277

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

Remove explicit ECDH exclusion for Travis CI

f01ac3a

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 dismissed yanrayw’s stale review via f01ac3a February 22, 2023 02:21

lpy4105 removed the needs-reviewer This PR needs someone to pick it up for review label Feb 22, 2023

lpy4105 requested review from gilles-peskine-arm and yanrayw February 22, 2023 05:17

yanrayw suggested changes Feb 22, 2023

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

tests/compat.sh Show resolved Hide resolved

gilles-peskine-arm requested changes Feb 22, 2023

View reviewed changes

gilles-peskine-arm added needs-work 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 Feb 22, 2023

Update some comments

9e7bb2a

Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>

lpy4105 requested review from gilles-peskine-arm and yanrayw February 23, 2023 08:05

lpy4105 mentioned this pull request Feb 23, 2023

Backport 2.28: compat.sh: Skip static ECDH cases if unsupported in openssl #7153

Merged

lpy4105 removed the needs-backports Backports are missing or are pending review and approval. label Feb 23, 2023

yanrayw approved these changes Feb 23, 2023

View reviewed changes

gilles-peskine-arm approved these changes Feb 23, 2023

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Feb 23, 2023

gilles-peskine-arm merged commit e461683 into Mbed-TLS:development Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

lpy4105 commented Feb 20, 2023 •

edited by paul-elliott-arm

Loading

yanrayw left a comment

yanrayw left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 21, 2023

lpy4105 Feb 22, 2023

lpy4105 Feb 22, 2023

gilles-peskine-arm Feb 22, 2023

lpy4105 Feb 23, 2023

yanrayw left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 22, 2023

yanrayw left a comment

gilles-peskine-arm left a comment

	if grep 'Cipher is (NONE)' $CLI_OUT >/dev/null; then
	RESULT=1

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

compat.sh: Skip static ECDH cases if unsupported in openssl #7137

Conversation

lpy4105 commented Feb 20, 2023 • edited by paul-elliott-arm Loading

Description

Gatekeeper checklist

yanrayw left a comment

Choose a reason for hiding this comment

yanrayw left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 21, 2023

Choose a reason for hiding this comment

lpy4105 Feb 22, 2023

Choose a reason for hiding this comment

lpy4105 Feb 22, 2023

Choose a reason for hiding this comment

gilles-peskine-arm Feb 22, 2023

Choose a reason for hiding this comment

lpy4105 Feb 23, 2023

Choose a reason for hiding this comment

yanrayw left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 22, 2023

Choose a reason for hiding this comment

yanrayw left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

lpy4105 commented Feb 20, 2023 •

edited by paul-elliott-arm

Loading