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
[WFCORE-4842] Add support for TLS 1.3 using the OpenSSL TLS provider with Elytron #288
Conversation
actually available will depend on this provider. | ||
|
||
One-way and two-way SSL tests will be added to the WildFly OpenSSL testsuite and to the WildFly | ||
Core testsuite that make use of the TLS 1.3 protocol and TLS 1.3 cipher suites. The `java.specification.version` |
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.
Just checking whether this part is still true since we agreed that we'll cover this feature in one of our internal testsuites?
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.
Overall I don't believe we should ever take a decision to not include tests in the open source test suites if we have the option to perform the tests here regardless of any internal testing.
Ideally test cases should be as close to the engineer making changes as is practically possible so that any regressions can be picked up early - test cases within WildFly Core give us an opportunity to detect regressions definitely before WildFly Core is tagged and potentially before WildFly Open SSL is tagged.
Tests within the internal testsuite on the other hand only detect regressions late in the process potentially meaning any fix could require WildFly OpenSSL, WildFly Core, and WildFly all to be tagged again for something we had the potential to detect earlier.
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.
@jstourac Tests will still be added in the WildFly OpenSSL testsuite (I am currently working through these). Ideally, it would be good to have some test cases in the WildFly Core testsuite as well but I'll need to check with Ken first to see if we can perform the required setup on the Linux and Windows CI machines used for these runs.
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.
@darranl I completely agree. This case is a little bit more complicated with the setup necessary on platforms where OpenSSL libraries are not available at the moment in WildFly CI. That is why we talked with Farah that there will be tests for this in WildFly OpenSSL upstream TS
but we're gonna use internal TS instead of WildFly upstream TS
for integraiton tests. I wasn't quite sure what was the decision regarding to WildFly Core upstream TS
.
@fjuma okay then, let me know in case you need anything. Thanks!
One-way and two-way SSL tests will be added to the WildFly OpenSSL testsuite and to the WildFly | ||
Core testsuite that make use of the TLS 1.3 protocol and TLS 1.3 cipher suites. The `java.specification.version` | ||
system property will be used to make sure that these new tests only get run when JDK 11 or higher | ||
is in use since TLS 1.3 was only introduced in JDK 11. |
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.
Is new system property necessary? Cannot we use combination of org.junit.Assume
and System.getProperty('java.version')
?
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.
Mentioned System.getProperty('java.specification.version')
here just to be consistent with what is currently used in the existing TLS 1.3 tests in the Elytron and WildFly Core testsuites, e.g., see below:
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.
Oh, sorry, of course. :)
https://issues.redhat.com/browse/WFCORE-4842