-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
compat.sh: Skip static ECDH cases if unsupported in openssl #7137
compat.sh: Skip static ECDH cases if unsupported in openssl #7137
Conversation
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me, except for a minor improvement.
Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still not skipping enough.
@@ -534,6 +534,15 @@ add_mbedtls_ciphersuites() | |||
esac | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No explicit exclusions found in all.sh
, so I only removed that in .travis.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also found this doesn't work for a more recent OpenSSL (e.g 1.1.1f on Travis CI).
Lines 823 to 824 in ffb92b0
if grep 'Cipher is (NONE)' $CLI_OUT >/dev/null; then | |
RESULT=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a potential issue to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, except, please update the preexisting incorrect comment.
@@ -534,6 +534,15 @@ add_mbedtls_ciphersuites() | |||
esac | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Signed-off-by: Pengyu Lv <pengyu.lv@arm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Fix: #1785
This PR add the ability in
compat.sh
to check if the under testingopenssl
program supports static ECDH (i.e.ECDH_ECDSA
) key exchange methods. The cases forTLS_ECDH_ECDSA_xxx
ciphersuites will be skipped ifopenssl
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