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

Refine OpenSSL test names and allow more complete OpenSSL version coverage #3208

Open
wants to merge 1 commit into
base: 10.4
Choose a base branch
from

Conversation

DarrenZhang01
Copy link
Contributor

@DarrenZhang01 DarrenZhang01 commented Apr 15, 2024

Description

  • Remove "6975" from the OpenSSL test and result file names. "6975" refers to https://jira.mariadb.org/browse/MDEV-6975 about implementing TLS support in general, which is not a bug report. In this case, this is a generic OpenSSL test set, so update the test file names for better clarification.
  • Remove the upper bound of the OpenSSL version check to allow the tests to also run with latest OpenSSL versions given that this is a generic set of OpenSSL tests.
  • Update the tests and result files such that they are compatible with new error messages for OpenSSL 3.2.0 and later versions ("openssl/openssl@81b741f68984").
  • Update the configuration name from "tlsv10" to "sslv3" when SSL cipher used is "SSLv3".

How can this PR be tested?

by running the updated OpenSSL tests.

Basing the PR against the correct MariaDB version

  • This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

…erage

- Remove "6975" from the OpenSSL test and result file names. "6975" refers
  to https://jira.mariadb.org/browse/MDEV-6975 about implementing TLS
  support in general, which is not a bug report. In this case, this is a
  generic OpenSSL test set, so update the test file names for better
  clarification.
- Remove the upper bound of the OpenSSL version check to allow the tests
  to also run with latest OpenSSL versions given that this is a generic
  set of OpenSSL tests.
- Update the tests and result files such that they are compatible with new
  error messages for OpenSSL 3.2.0 and later versions
("openssl/openssl@81b741f68984").
- Update the configuration name from "tlsv10" to "sslv3" when SSL cipher
  used is "SSLv3".

Copyright:
All new code of the whole pull request, including one or several files that are
either new files or modified ones, are contributed under the BSD-new license. I
am contributing on behalf of my employer Amazon Web Services, Inc.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@LinuxJedi LinuxJedi left a comment

Choose a reason for hiding this comment

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

I have a few thoughts on this. Not necessarily related to your changes, but in some ways they are.

First of all, I'm not entirely sure what this test is supposed to do. SSLv3 ciphers: user is ok with any cipher; but then uses a TLS cipher?

Secondly, this test is known buggy anyway (MDEV-16902).

Finally I'm not sure I'm comfortable still supporting SSLv3 ciphers as an option.

As for the PR, it looks like the test was for 1.0.1d - 1.1.0, but now it is open for all current versions. Is that correct? I don't know the history of this test to know why 1.1.0 was the cut-off.

@ottok
Copy link
Contributor

ottok commented Apr 25, 2024

Secondly, this test is known buggy anyway (MDEV-16902).

Yes but nothing has been done for MDEV-16902 in since 2018. Fixing the test now is useful still, right?

Finally I'm not sure I'm comfortable still supporting SSLv3 ciphers as an option.

That can be dropped if you think that supporting older versions is not relevant anymore. The biggest fix here is however cleaning up a OpenSSL 1.1.0 -only test to something that has value and actually runs on any modern system.

As for the PR, it looks like the test was for 1.0.1d - 1.1.0, but now it is open for all current versions. Is that correct? I don't know the history of this test to know why 1.1.0 was the cut-off.

This was discovered while working on #3154. Commit messages and Jira descriptions from MDEV-6975 are thin and thus rewriting the test to something more sensible now is based on best guess. For sure it was stupid to have a test named openssl_6975 and MDEV-6975 is not a bug, but Jira to implement OpenSSL in general. Thus the test should probably test OpenSSL in general, right? If you think such a test is not needed, then the test should be deleted.

@ottok ottok requested a review from LinuxJedi April 25, 2024 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants