-
Notifications
You must be signed in to change notification settings - Fork 39
4.x: Introduce integration test for TLS session tickets #593
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
4.x: Introduce integration test for TLS session tickets #593
Conversation
|
Tested with Java 8, 11 and 17. |
dkropachev
left a 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.
Way to test counters is prone to failure, if connection is re-estalished during the test (I have seen it happening), counters will drift and you going to get false-positive.
To test that TLS tickets work properly you need to establish connection, then break it and then check if ticket was reused, don't see it is happening here.
integration-tests/src/test/java/com/datastax/oss/driver/core/ssl/SessionTicketsIT.java
Outdated
Show resolved
Hide resolved
I don't think the false positive (test passing when it should not) is possible here, but false negative is.
There are 5 resumptions and 6 handshakes happening in this test. I can make the checks more explicit by tracking the ticket data from the logs and match it against what is reported to be received through NewSessionTicket messages. |
https://en.wikipedia.org/wiki/False_positives_and_false_negatives :
I’m totally fine with it, unless there’s a better way—which there isn’t.
Given the fact that |
3e29572 to
2527f1e
Compare
|
Set protocol version to V4, removed hardcoded expected number of initiated handshakes, removed some counters and made some checks more lenient. I've also ran some tests with more nodes and higher smp and in general the feature is supported but probably does not work like we would want it to. The server seems to consistently send two session tickets per connection. Setting higher smp did not seem to impact that. As was mentioned above when channel pool is created it first makes one connection to a node and then tries to add all remaining channels at once. Having a few surplus tickets is not going to help with initialization of all those connections. I have also tried to see what happens if i stop the node and start it again using ccm. Once the node is stopped all channels try to connect again and so far it seems that only one channel tries to use session ticket as if only one ticket was being kept in cache. Other connection attempts report that there is no PSK identity to use for them. In theory there should be around as many unused tickets as there are shards, since each connection receives two tickets. I did not find yet where does this result come from. So since it does not provide any extra value I did not set higher smp or added more nodes to the test for now. |
|
@Bouncheck , please update on what is going on. |
7ca6356 to
4fd48e0
Compare
|
In regard to the test: In regard to the whole issue: |
4fd48e0 to
9effa23
Compare
|
Moved some parts to separate methods to avoid repetition. The CI failure does not look to be related. This test class does not even target Cassandra and the failure is caused by the cluster version reported by the node. It contains |
| // We want all reconnections to use session tickets for resumptions, but current | ||
| // implementation does not provide that. |
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.
Please put all available information into the issue and reference it here.
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.
Added the link and short explanation.
9effa23 to
5f071b6
Compare
Adds isolated SessionTicketsIT which checks if session tickets mechanism behaves as expected. It does that by watching Java's SSL debug logs and ensuring that specific substrings appear in them. Since the server supports tickets with TLSv1.3, only that version is tested. In that version the session resumption without server-side state is done through pre-shared keys. The details are described in rfc8446 (see section 2.2).
5f071b6 to
1c84300
Compare
dkropachev
left a 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.
CICD failure has nothing to do with the PR.
Looks good.
Adds isolated SessionTicketsIT which checks if session tickets mechanism behaves as expected. It does that by watching Java's SSL debug logs and ensuring that specific substrings appear in them.
Since the server supports tickets with TLSv1.3, only that version is tested. In that version the session resumption without server-side state is done through pre-shared keys. The details are described in rfc8446 (see section 2.2).