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

More precise calculate the maximum record size when using SslProvider… #6276

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Jan 26, 2017

….OPENSSL* and so decrease mem usage.

Motivation:

We used ca 2k as maximum overhead for encrypted packets which is a lot more then what is needed in reality by OpenSSL. This could lead to the need of more memory.

Modification:

  • Use a lower overhead of 86 bytes as defined by the spec and openssl itself
  • Fix unit test to use the correct session to calculate needed buffer size

Result:

Less memory usage.

@normanmaurer normanmaurer added this to the 4.0.44.Final milestone Jan 26, 2017

@normanmaurer normanmaurer self-assigned this Jan 26, 2017

@normanmaurer normanmaurer requested review from trustin, nmittler and Scottmitch Jan 26, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 26, 2017

Member

@nmittler @Scottmitch FYI... this highly reduce the needed buffer size when encrypt small buffers and using SslProvider.OPENSSL*.

Member

normanmaurer commented Jan 26, 2017

@nmittler @Scottmitch FYI... this highly reduce the needed buffer size when encrypt small buffers and using SslProvider.OPENSSL*.

@nmittler

LGTM pending discussion with @davidben

// As this is called for the handshake we have no real idea how big the buffer needs to be.
// That said 2048 should give us enough room to include everything like ALPN / NPN data.
// If this is not enough we will increase the buffer in wrap(...).
out = allocateOutNetBuf(ctx, 2048);

This comment has been minimized.

@Scottmitch

Scottmitch Jan 26, 2017

Member

mental note to my self ... we may be able to use something like MAX_BIO_BYTES in the future or create another define for "non-app-data"

@Scottmitch

Scottmitch Jan 26, 2017

Member

mental note to my self ... we may be able to use something like MAX_BIO_BYTES in the future or create another define for "non-app-data"

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 27, 2017

By the way, at some point, BoringSSL will probably have some kind of in-place/buffer-free/BIO-free/bring-your-own-buffer/whatever-you-call-it API that should align much better with the wrap/unwrap thing you're doing.

(Although the first iteration is probably not going to be require contiguous buffers, so you'll probably still need to do some copying.)

davidben commented Jan 27, 2017

By the way, at some point, BoringSSL will probably have some kind of in-place/buffer-free/BIO-free/bring-your-own-buffer/whatever-you-call-it API that should align much better with the wrap/unwrap thing you're doing.

(Although the first iteration is probably not going to be require contiguous buffers, so you'll probably still need to do some copying.)

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 27, 2017

Member

@davidben - Interesting ... would this be better than implanting our own BIO which directly writes/reads from ByteBuffer's direct memory (like in #6201)?

Member

Scottmitch commented Jan 27, 2017

@davidben - Interesting ... would this be better than implanting our own BIO which directly writes/reads from ByteBuffer's direct memory (like in #6201)?

More precise calculate the maximum record size when using SslProvider…
….OPENSSL* and so decrease mem usage.

Motivation:

We used ca 2k as maximum overhead for encrypted packets which is a lot more then what is needed in reality by OpenSSL. This could lead to the need of more memory.

Modification:

- Use a lower overhead of 86 bytes as defined by the spec and openssl itself
- Fix unit test to use the correct session to calculate needed buffer size

Result:

Less memory usage.
*
* 16 (IV) + 48 (MAC) + 1 (Padding_length field) + 15 (Padding) + 1 (ContentType) + 2 (ProtocolVersion) + 2 (Length)
*
* TODO: We may need to review this calculation once TLS 1.3 becomes available.

This comment has been minimized.

@netkins

netkins Jan 27, 2017

INFO Complete the task associated to this TODO comment. rule

@netkins

netkins Jan 27, 2017

INFO Complete the task associated to this TODO comment. rule

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 27, 2017

@Scottmitch You should be able to save the copies in and out of the SSL stack's internal buffer? But I haven't looked very closely at your current code or the PR. Trying to mimic wrap/unwrap over SSL_read also has the problem that you can't "unread" data on BUFFER_UNDERFLOW, which is probably part of why you have a reimplemented record layer? This would avoid that.

(To clarify, this is more a direction we're keeping in mind as we rework the stack thing rather than something you'll have available next week or anything.)

davidben commented Jan 27, 2017

@Scottmitch You should be able to save the copies in and out of the SSL stack's internal buffer? But I haven't looked very closely at your current code or the PR. Trying to mimic wrap/unwrap over SSL_read also has the problem that you can't "unread" data on BUFFER_UNDERFLOW, which is probably part of why you have a reimplemented record layer? This would avoid that.

(To clarify, this is more a direction we're keeping in mind as we rework the stack thing rather than something you'll have available next week or anything.)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 27, 2017

Member

@davidben FYI... added unit-tests and it seems like it "just works" :) Thanks again for the suggestion

Member

normanmaurer commented Jan 27, 2017

@davidben FYI... added unit-tests and it seems like it "just works" :) Thanks again for the suggestion

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 27, 2017

Member

you can't "unread" data on BUFFER_UNDERFLOW

@davidben - Currently we don't attempt to read if we detect there is not enough room to store the data.

part of why you have a reimplemented record layer

By "record layer" do you mean the BIO implementation netty/netty-tcnative#216?

Member

Scottmitch commented Jan 27, 2017

you can't "unread" data on BUFFER_UNDERFLOW

@davidben - Currently we don't attempt to read if we detect there is not enough room to store the data.

part of why you have a reimplemented record layer

By "record layer" do you mean the BIO implementation netty/netty-tcnative#216?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 27, 2017

Member

Cherry-picked into 4.1 (91f050d) and 4.0 (8066af3)

Member

normanmaurer commented Jan 27, 2017

Cherry-picked into 4.1 (91f050d) and 4.0 (8066af3)

@normanmaurer normanmaurer deleted the ssl_overhead_openssl branch Jan 27, 2017

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

normanmaurer added a commit that referenced this pull request Jan 27, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

kiril-me added a commit to kiril-me/netty that referenced this pull request Jan 31, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols

liuzhengyang pushed a commit to liuzhengyang/netty that referenced this pull request Sep 10, 2017

Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment