Skip to content

Fixing test assertions for TLSv1.3 #937

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

Merged
merged 7 commits into from
Jan 28, 2025
Merged

Conversation

the-thing
Copy link
Contributor

@the-thing the-thing commented Jan 28, 2025

This will fix the SSL test suite for TLS v1.3.

Changes

  • switching cipher suite and TLS version for SSLCertificateTest (see TLSv1.3 test #892)
  • refactored some SSL util methods and extracted them to a separate class
  • modified assertNotAuthenticated method

The problem with authentication and TLS v1.3 seems to be with MacOS Java version 8 and 11 as it fails continuously with this combination (works fine with MacOS Java 17). I've seen it failing occasionally on Ubuntu.

The authentication actually works fine, but the assertions were not correct. Seeing Certificate was authenticated was misleading since the previous checks - assertSslExceptionThrown and assertNotLoggedOn were successful, which means that SSL session was actually NOT established with invalid certificates as expected.

Even when authentication is enabled and SSL handshake fails, the peer certificates (and peer principal) are set for javax.net.ssl.SSLSession#getPeerCertificates (there is a note in later versions of Java > 8 in JavaDoc for getPeerCertificates saying Note: The returned value may not be a valid certificate chain and should not be relied on for trust decisions.). This means that we can't rely on the value there.

It seems that peer certificates and peer princpial are set for all SSL sessions that require authentication regardless if the check was successful or not. Only when auth is disabled e.g by setting javax.net.ssl.SSLParameters#needClientAuth to false these certificate chains and peers can be expected to be empty.

For authentication we have to rely on MINA removing the org.apache.mina.filter.ssl.SslFilter from org.apache.mina.core.session.IoSession filter chain.

Putting a few second delay after initiator start solves the problem as it is enough for MINA to tear down the SSLFilter - that's what I did, but with busy spin loop. I also kept the old validation code for peers that don't require auth.

@chrjohn
Copy link
Member

chrjohn commented Jan 28, 2025

Thanks @the-thing , will have a look! 👍

@chrjohn chrjohn added this to the QFJ 3.0.0 milestone Jan 28, 2025
@the-thing
Copy link
Contributor Author

the-thing commented Jan 28, 2025

We could also add org.junit.runners.Parameterized runner and make this test suite run for both TLSv1.2 and TLSv1.3.

I forgot to mention that this will work for TLSv1.2 as well...

@chrjohn
Copy link
Member

chrjohn commented Jan 28, 2025

We could also add org.junit.runners.Parameterized runner and make this test suite run for both TLSv1.2 and TLSv1.3.

I'll leave that up to you. Thank you

@chrjohn
Copy link
Member

chrjohn commented Jan 28, 2025

Please let me know when this is ready to merge.

@chrjohn chrjohn marked this pull request as draft January 28, 2025 12:59
@the-thing
Copy link
Contributor Author

the-thing commented Jan 28, 2025

It is ready, but some other test have failed.

2025-01-28T13:33:55.1936450Z [ERROR] Run 933: AcceptanceTestSuite$AcceptanceTest.run:77 connection to server failed:

I see the acceptance test suite failing occasionally here and there.

@the-thing the-thing marked this pull request as ready for review January 28, 2025 14:06
@chrjohn
Copy link
Member

chrjohn commented Jan 28, 2025

I see the acceptance test suite failing occasionally here and there.

I'm sure it does not have to do with your changes. One of the acceptance tests fails from time to time, did not have time to follow up.

@chrjohn chrjohn mentioned this pull request Jan 28, 2025
@chrjohn chrjohn merged commit a967c4b into quickfix-j:master Jan 28, 2025
12 checks passed
@the-thing the-thing deleted the tls-13 branch January 28, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants