-
Notifications
You must be signed in to change notification settings - Fork 0
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
introduce opportunistic option for TLS #2
Changes from 6 commits
e2e3727
af5f647
8a42f98
963c585
b0a8e6d
08d14e1
e1945c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
/* | ||
* Copyright 2015 The gRPC Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package io.grpc.netty; | ||
|
||
import io.grpc.Channel; | ||
import io.grpc.netty.ProtocolNegotiators.PlaintextProtocolNegotiator; | ||
import io.netty.buffer.ByteBuf; | ||
import io.netty.buffer.ByteBufAllocator; | ||
import io.netty.channel.ChannelHandler; | ||
import io.netty.channel.ChannelHandlerContext; | ||
import io.netty.handler.codec.ByteToMessageDecoder; | ||
import io.netty.handler.ssl.SslContext; | ||
import io.netty.handler.ssl.SslHandler; | ||
import io.netty.util.ReferenceCountUtil; | ||
import io.netty.util.concurrent.ImmediateExecutor; | ||
import io.netty.util.internal.ObjectUtil; | ||
|
||
import java.util.concurrent.Executor; | ||
import javax.annotation.Nullable; | ||
import javax.net.ssl.SSLContext; | ||
import javax.net.ssl.SSLParameters; | ||
import java.util.List; | ||
|
||
/** | ||
* {@link CustomOptionalSslHandler} is a utility decoder to support both SSL and non-SSL handlers | ||
* based on the first message received. | ||
* | ||
* <p> This is equivalent to Netty's {@link io.netty.handler.ssl.OptionalSslHandler} with a few | ||
* added features: | ||
* <li> The {@link CustomOptionalSslHandler} constructor takes an {@link Executor} so it can be used | ||
* to create a new SslHandler if dealing with Ssl traffic. This is more in line with how the | ||
* SslHandler is created when created from | ||
* {@link io.grpc.netty.ProtocolNegotiators.ServerTlsHandler#handlerAdded(ChannelHandlerContext)} | ||
* <li> When dealing with plaintext traffic, it's not enough to remove only the | ||
* {@link CustomOptionalSslHandler}. We need to also remove the | ||
* {@link io.grpc.netty.ProtocolNegotiators.ServerOpportunisticTlsHandler} so that the channel pipeline | ||
* behaves the same as the traditional plaintext pipeline, and we need to call | ||
* {@link ChannelHandlerContext#fireUserEventTriggered(Object)} to continue along the pipeline | ||
*/ | ||
public class CustomOptionalSslHandler extends ByteToMessageDecoder { | ||
|
||
private final SslContext sslContext; | ||
private final Executor executor; | ||
private final ChannelHandler next; | ||
|
||
public CustomOptionalSslHandler(SslContext sslContext, ChannelHandler next, @Nullable Executor executor) { | ||
this.sslContext = ObjectUtil.checkNotNull(sslContext, "sslContext"); | ||
this.next = ObjectUtil.checkNotNull(next, "next"); | ||
if (executor != null) { | ||
this.executor = executor; | ||
} else { | ||
this.executor = ImmediateExecutor.INSTANCE; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what cases do we need anything other than ImmediateExecutor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it's typically a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @joybestourous do you know how/where the executor is used in the direct TLS flow? I would expect this sort of execution policy to live outside of the pipelines and, in most cases, the pipelines are oblivious to it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used to create the SslHandler, and then used here in the SslHandler |
||
} | ||
} | ||
|
||
@Override | ||
protected void decode(ChannelHandlerContext context, ByteBuf in, List<Object> out) throws Exception { | ||
// SslUtils.SSL_RECORD_HEADER_LENGTH = 5 | ||
if (in.readableBytes() < 5) { | ||
return; | ||
} | ||
if (SslHandler.isEncrypted(in)) { | ||
handleSsl(context); | ||
} else { | ||
handleNonSsl(context); | ||
} | ||
} | ||
|
||
private void handleSsl(ChannelHandlerContext context) { | ||
SslHandler sslHandler = null; | ||
try { | ||
sslHandler = newSslHandler(context, sslContext); | ||
context.pipeline().replace(this, newSslHandlerName(), sslHandler); | ||
sslHandler = null; | ||
} finally { | ||
// Since the SslHandler was not inserted into the pipeline the ownership of the SSLEngine was not | ||
// transferred to the SslHandler. | ||
if (sslHandler != null) { | ||
ReferenceCountUtil.safeRelease(sslHandler.engine()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is copied from OptionalSslHandler, but I don't understand why we need refCounts here. Are we expected to call this every time we create a new engine and we are done with using it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all SslEngines are ReferenceCounted meaning they need explicit deallocation. If there was an sslHandler that took ownership of the lifecycle here, it would handle release when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC, this complexity comes from the fact netty leverages off-heap memory extensively. To manage that safely, netty uses explicit ref counting. |
||
} | ||
} | ||
} | ||
|
||
private void handleNonSsl(ChannelHandlerContext context) { | ||
ChannelHandler handler = newNonSslHandler(context); | ||
if (handler != null) { | ||
context.pipeline().replace(this, newNonSslHandlerName(), handler); | ||
} else { | ||
context.pipeline().remove(this); | ||
context.pipeline().replace(ProtocolNegotiators.ServerOpportunisticTlsHandler.class, null, next); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not what original OptionalSslHandler was doing, why do we need this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, The idea here is we want the context.pipeline to match that of the plaintext case. In the tls case the pipeline looks like this: |
||
context.fireUserEventTriggered(ProtocolNegotiationEvent.DEFAULT); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we firing this even here and why we don't do the same in case when we handle ssl? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we handle Ssl, this is called when the SslHandshake is completed. Without this, the pipeline gets stuck here in the same way I mentioned above; this asks the pipeline to move on. |
||
} | ||
} | ||
|
||
/** | ||
* Optionally specify the SSL handler name, this method may return {@code null}. | ||
* @return the name of the SSL handler. | ||
*/ | ||
protected String newSslHandlerName() { | ||
return null; | ||
} | ||
|
||
/** | ||
* Override to configure the SslHandler eg. {@link SSLParameters#setEndpointIdentificationAlgorithm(String)}. | ||
* The hostname and port is not known by this method so servers may want to override this method and use the | ||
* {@link SslContext#newHandler(ByteBufAllocator, String, int)} variant. | ||
* | ||
* @param context the {@link ChannelHandlerContext} to use. | ||
* @param sslContext the {@link SSLContext} to use. | ||
* @return the {@link SslHandler} which will replace the {@link CustomOptionalSslHandler} in the pipeline if the | ||
* traffic is SSL. | ||
*/ | ||
protected SslHandler newSslHandler(ChannelHandlerContext context, SslContext sslContext) { | ||
return sslContext.newHandler(context.alloc(), executor); | ||
} | ||
|
||
/** | ||
* Optionally specify the non-SSL handler name, this method may return {@code null}. | ||
* @return the name of the non-SSL handler. | ||
*/ | ||
protected String newNonSslHandlerName() { | ||
return null; | ||
} | ||
|
||
/** | ||
* Override to configure the ChannelHandler. | ||
* @param context the {@link ChannelHandlerContext} to use. | ||
* @return the {@link ChannelHandler} which will replace the {@link CustomOptionalSslHandler} in the pipeline | ||
* or {@code null} to simply remove the {@link CustomOptionalSslHandler} if the traffic is non-SSL. | ||
*/ | ||
protected ChannelHandler newNonSslHandler(ChannelHandlerContext context) { | ||
return null; | ||
} | ||
} |
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.
Can we make this a subclass of netty.OptionalSslHandler? It is public and if we need to use a custom executor (which I don't understand why) we can also overwrite protected
newSslHandler
method.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.
Yeah this is only not a subclass because of that executor constructor. If we decide we dont need that we can simplify this.