Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove dedicated SSL network write buffer #41283

Merged
merged 8 commits into from
Apr 25, 2019
Merged

Remove dedicated SSL network write buffer #41283

merged 8 commits into from
Apr 25, 2019

Conversation

Tim-Brooks
Copy link
Contributor

This is related to #27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.

@Tim-Brooks
Remove dedicated SSL network write buffer
b3f07b6 
This is related to elastic#27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@s1monw
Copy link
Contributor

s1monw commented Apr 25, 2019

I don't feel I am comfortable enough with SSL code to give a good review. I looked over it and nothing looks terrible. So I am relying on @original-brownbear for this.

@original-brownbear
Copy link
Member

I'm on it today, sorry for missing this one!

original-brownbear
original-brownbear approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

libs/nio/src/main/java/org/elasticsearch/nio/FlushOperation.java Outdated


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: double empty line

.../plugin/security/src/main/java/org/elasticsearch/xpack/security/transport/nio/SSLDriver.java Outdated
while (true) {
SSLEngineResult result;
ByteBuffer networkBuffer = outboundBuffer.nextWriteBuffer(packetSize);
try {
if (buffers.length == 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little off-topic but I figured I'd mention it while we're changing these lines anyway:
For the default implementation of the JDK's SSLEngine, wrap for just a single buffer source argument src reads:

    public SSLEngineResult wrap(ByteBuffer src,
            ByteBuffer dst) throws SSLException {
        return wrap(new ByteBuffer [] { src }, 0, 1, dst);
    }

=> I think we can just remove this if and always pass the buffer array even in the single value case?

libs/nio/src/main/java/org/elasticsearch/nio/Page.java Outdated

public class Page implements Closeable {

private static final Runnable DEFAULT_CLOSEABLE = () -> {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: why not just inline this lambda?

@Tim-Brooks
Copy link
Contributor Author

@original-brownbear Thanks. I've made your suggested changes and will merge once CI passes.

@Tim-Brooks Tim-Brooks merged commit 0ca375e into elastic:master Apr 25, 2019
Tim-Brooks added a commit that referenced this pull request Apr 25, 2019
@Tim-Brooks
Remove dedicated SSL network write buffer (#41283)
f65a86c 
This is related to #27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.
@dnhatn dnhatn mentioned this pull request Apr 25, 2019
Open
Tim-Brooks added a commit that referenced this pull request Apr 26, 2019
@Tim-Brooks
Revert "Remove dedicated SSL network write buffer (#41283)"
1f8ff05 
This reverts commit f65a86c.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 26, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into java-11-collection…
62d7b7f 
…s-in-all-the-places

* elastic/master: (70 commits)
  Remove experimental label froms script_score query (elastic#41572)
  [Ml-Dataframe] Update URLs in Data frame client java doc (elastic#41539)
  Reenable bwc Tests in master (elastic#41540)
  Fix search_as_you_type's sub-fields to pick their names from the full path of the root field (elastic#41541)
  Remove search analyzers from DocumentFieldMappers (elastic#41484)
  Update community client and integration docs (elastic#41513)
  Remove Version.V_6_x_x constants use in security (elastic#41185)
  Mute testDriverConfigurationWithSSLInURL
  Remove dedicated SSL network write buffer (elastic#41283)
  Disable max score optimization for queries with unbounded max scores (elastic#41361)
  [DOCS] Explicitly set section IDs for Asciidoctor migration (elastic#41547)
  [ML] add multi node integ tests for data frames (elastic#41508)
  [Docs] Fix common word repetitions (elastic#39703)
  [DOCS] Note TESTRESPONSE can't be used immediately after TESTSETUP (elastic#41542)
  Update configuring-ldap-realm.asciidoc (elastic#40427)
  Fixed very small typo in date (elastic#41398)
  Refactor GeoHashUtils (elastic#40869)
  Make 0 as invalid value for `min_children` in `has_child` query (elastic#41347)
  field_caps: adapt bwc version after backport (elastic#41427)
  Remove Exists Check from S3 Repository Deletes (elastic#40931)
  ...
Tim-Brooks added a commit to Tim-Brooks/elasticsearch that referenced this pull request Apr 29, 2019
@Tim-Brooks
Remove dedicated SSL network write buffer (elastic#41283)
6b372da 
This is related to elastic#27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.
akhil10x5 pushed a commit to akhil10x5/elasticsearch that referenced this pull request May 2, 2019
@Tim-Brooks @akhil10x5
Remove dedicated SSL network write buffer (elastic#41283)
6503132 
This is related to elastic#27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@Tim-Brooks
Remove dedicated SSL network write buffer (elastic#41283)
08a97c7 
This is related to elastic#27260. Currently for the SSLDriver we allocate a
dedicated network write buffer and encrypt the data into that buffer one
buffer at a time. This requires constantly switching between encrypting
and flushing. This commit adds a dedicated outbound buffer for SSL
operations that will internally allocate new packet sized buffers as
they are need (for writing encrypted data). This allows us to totally
encrypt an operation before writing it to the network. Eventually it can
be hooked up to buffer recycling.
@Tim-Brooks Tim-Brooks deleted the support_rolling_writes branch April 30, 2020 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@original-brownbear original-brownbear original-brownbear approved these changes

@s1monw s1monw Awaiting requested review from s1monw

@ywelsch ywelsch Awaiting requested review from ywelsch

Assignees
No one assigned
Labels
>non-issue :Security/TLS SSL/TLS, Certificates v7.2.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Tim-Brooks @elasticmachine @s1monw @original-brownbear @jakelandis