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

OpenSslEngine option to wrap/unwrap multiple packets per call #6365

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@Scottmitch
Member

Scottmitch commented Feb 13, 2017

Motivation:
The JDK SSLEngine documentation says that a call to wrap/unwrap "will attempt to consume one complete SSL/TLS network packet" [1]. This limitation can result in thrashing in the pipeline to decode and encode data that may be spread amongst multiple SSL/TLS network packets.
ReferenceCountedOpenSslEngine also does not correct account for the overhead introduced by each individual SSL_write call if there are multiple ByteBuffers passed to the wrap() method.

Modifications:

  • OpenSslEngine and SslHandler supports a mode to not comply with the limitation to only deal with a single SSL/TLS network packet per call
  • ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write
  • SslHandler shouldn't cache maxPacketBufferSize as aggressively because this value may change before/after the handshake.

Result:
OpenSslEngine and SslHanadler can handle multiple SSL/TLS network packet per call.

[1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html

@Scottmitch Scottmitch self-assigned this Feb 13, 2017

@Scottmitch Scottmitch requested review from normanmaurer and nmittler Feb 13, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Feb 13, 2017

Member
  • This depends upon netty/netty-tcnative#233
  • Once #6360 is pulled in I will update to account for multiple packets in the buffer.
  • Discovered #6348 while developing this. TODOs will be addressed as part of that issue.
Member

Scottmitch commented Feb 13, 2017

  • This depends upon netty/netty-tcnative#233
  • Once #6360 is pulled in I will update to account for multiple packets in the buffer.
  • Discovered #6348 while developing this. TODOs will be addressed as part of that issue.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer May 8, 2017

Member

@Scottmitch in general I would prefer if we could have two different PRs. One which fixes the "ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write" and the other which introduce the new "feature". Can you please split it up ?

Member

normanmaurer commented May 8, 2017

@Scottmitch in general I would prefer if we could have two different PRs. One which fixes the "ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write" and the other which introduce the new "feature". Can you please split it up ?

ByteBuffer dst = dsts[dstsOffset];
if (!dst.hasRemaining()) {
// No space left in the destination buffer, skip it.
if (++dstsOffset >= dstsEndOffset) {

This comment has been minimized.

@netkins

netkins May 8, 2017

MAJOR Refactor this code to not nest more than 5 if/for/while/switch/try statements. rule

@netkins

netkins May 8, 2017

MAJOR Refactor this code to not nest more than 5 if/for/while/switch/try statements. rule

clicked wrong button :D

@Scottmitch Scottmitch removed the defect label May 8, 2017

Show outdated Hide outdated pom.xml
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch May 8, 2017

Member

@normanmaurer - I have addressed all of your comments. PTAL

Member

Scottmitch commented May 8, 2017

@normanmaurer - I have addressed all of your comments. PTAL

@normanmaurer

Generally LGTM, I would like to delay pulling this in until we did another netty release tho. Ok ?

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch May 9, 2017

Member

I would like to delay pulling this in until we did another netty release

sgtm

Member

Scottmitch commented May 9, 2017

I would like to delay pulling this in until we did another netty release

sgtm

* via other synchronized blocks.
*/
final int calculateMaxLengthForWrap(int plaintextLength, int numComponents) {
return (int) min(maxWrapBufferSize, plaintextLength + (long) maxWrapOverhead * numComponents);

This comment has been minimized.

@netkins

netkins May 17, 2017

CRITICAL Inconsistent synchronization of io.netty.handler.ssl.ReferenceCountedOpenSslEngine.maxWrapOverhead; locked 83% of time rule

@netkins

netkins May 17, 2017

CRITICAL Inconsistent synchronization of io.netty.handler.ssl.ReferenceCountedOpenSslEngine.maxWrapOverhead; locked 83% of time rule

This comment has been minimized.

@normanmaurer

normanmaurer May 25, 2017

Member

@Scottmitch just wonder if this is something we need to investigate

@normanmaurer

normanmaurer May 25, 2017

Member

@Scottmitch just wonder if this is something we need to investigate

This comment has been minimized.

@Scottmitch

Scottmitch Jun 2, 2017

Member

let me know if you think there is a problem. The intention is not to necessarily to protect this variable in the synchronization block because it is mostly accessed inside the EventLoop (except maybe for OpenSslSession#getPacketBufferSize()). The value is also only modified in a synchronized block (or in the constructor via calculateMaxWrapOverhead()).

@Scottmitch

Scottmitch Jun 2, 2017

Member

let me know if you think there is a problem. The intention is not to necessarily to protect this variable in the synchronization block because it is mostly accessed inside the EventLoop (except maybe for OpenSslSession#getPacketBufferSize()). The value is also only modified in a synchronized block (or in the constructor via calculateMaxWrapOverhead()).

@Scottmitch Scottmitch referenced this pull request May 18, 2017

Closed

SSLHandler slow #6571

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch May 25, 2017

Member

@normanmaurer - any more comments? this currently depends upon tcnative 2.0.2.Final-SNAPSHOT ... do you want me to wait to merge until after a release?

Member

Scottmitch commented May 25, 2017

@normanmaurer - any more comments? this currently depends upon tcnative 2.0.2.Final-SNAPSHOT ... do you want me to wait to merge until after a release?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer May 25, 2017

Member

@Scottmitch no more comments... let me release tcnative first . I will do this over the next days

Member

normanmaurer commented May 25, 2017

@Scottmitch no more comments... let me release tcnative first . I will do this over the next days

OpenSslEngine option to wrap/unwrap multiple packets per call
Motivation:
The JDK SSLEngine documentation says that a call to wrap/unwrap "will attempt to consume one complete SSL/TLS network packet" [1]. This limitation can result in thrashing in the pipeline to decode and encode data that may be spread amongst multiple SSL/TLS network packets.
ReferenceCountedOpenSslEngine also does not correct account for the overhead introduced by each individual SSL_write call if there are multiple ByteBuffers passed to the wrap() method.

Modifications:
- OpenSslEngine and SslHandler supports a mode to not comply with the limitation to only deal with a single SSL/TLS network packet per call
- ReferenceCountedOpenSslEngine correctly accounts for the overhead of each call to SSL_write
- SslHandler shouldn't cache maxPacketBufferSize as aggressively because this value may change before/after the handshake.

Result:
OpenSslEngine and SslHanadler can handle multiple SSL/TLS network packet per call.

[1] https://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html
@blucas

This comment has been minimized.

Show comment
Hide comment
@blucas

blucas Jul 9, 2017

Contributor

@Scottmitch what is the latest on this PR?

Contributor

blucas commented Jul 9, 2017

@Scottmitch what is the latest on this PR?

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 10, 2017

Member

@blucas - We were just waiting for a release until we pulled this in. I will pull in shortly.

Member

Scottmitch commented Jul 10, 2017

@blucas - We were just waiting for a release until we pulled this in. I will pull in shortly.

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 10, 2017

Member

4.1 (f7b3cae) 4.0 (c68a6ad)

Member

Scottmitch commented Jul 10, 2017

4.1 (f7b3cae) 4.0 (c68a6ad)

@Scottmitch Scottmitch closed this Jul 10, 2017

@Scottmitch Scottmitch deleted the Scottmitch:openssl_multiple_packets branch Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment