Skip to content

Commit 7ecbf56

Browse files
kannanjgithubkannanjgithub
committed
Fix review comments
1 parent 671444b commit 7ecbf56

File tree

4 files changed

+70
-52
lines changed

4 files changed

+70
-52
lines changed

‎netty/src/main/java/io/grpc/netty/GrpcHttp2OutboundHeaders.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,12 @@ private GrpcHttp2OutboundHeaders(AsciiString[] preHeaders, byte[][] serializedMe
6868

6969
@Override
7070
public CharSequence authority() {
71-
CharSequence authority = null;
7271
for (int i = 0; i < preHeaders.length / 2; i++) {
7372
if (preHeaders[i].equals(Http2Headers.PseudoHeaderName.AUTHORITY.value())) {
74-
authority = preHeaders[i + 1];
73+
return preHeaders[i * 2 + 1];
7574
}
7675
}
77-
assert authority != null;
78-
return authority;
76+
return null;
7977
}
8078

8179
@Override

‎netty/src/main/java/io/grpc/netty/NettyClientHandler.java‎

Lines changed: 41 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -604,57 +604,55 @@ private void createStream(CreateStreamCommand command, ChannelPromise promise)
604604
}
605605

606606
CharSequence authorityHeader = command.headers().authority();
607-
if (authorityHeader != null) {
608-
// No need to verify authority for the rpc outgoing header if it is same as the authority
609-
// for the transport
610-
if (!authority.contentEquals(authorityHeader)) {
611-
Status authorityVerificationStatus = peerVerificationResults.get(
612-
authorityHeader.toString());
613-
if (authorityVerificationStatus == null) {
614-
if (attributes.get(GrpcAttributes.ATTR_AUTHORITY_VERIFIER) != null) {
615-
authorityVerificationStatus = attributes.get(GrpcAttributes.ATTR_AUTHORITY_VERIFIER)
616-
.verifyAuthority(authorityHeader.toString());
617-
peerVerificationResults.put(authorityHeader.toString(), authorityVerificationStatus);
618-
if (!authorityVerificationStatus.isOk() && !enablePerRpcAuthorityCheck) {
619-
logger.log(Level.WARNING, String.format("%s.%s",
620-
authorityVerificationStatus.getDescription(),
621-
enablePerRpcAuthorityCheck
622-
? "" : " This will be an error in the future."),
623-
InternalStatus.asRuntimeExceptionWithoutStacktrace(
624-
authorityVerificationStatus, null));
625-
}
626-
} else {
627-
authorityVerificationStatus = Status.UNAVAILABLE.withDescription(
628-
"Authority verifier not found to verify authority");
629-
command.stream().setNonExistent();
630-
command.stream().transportReportStatus(
631-
authorityVerificationStatus, RpcProgress.PROCESSED, true, new Metadata());
632-
promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace(
633-
authorityVerificationStatus, null));
634-
return;
635-
}
636-
}
637-
if (authorityVerificationStatus != null && !authorityVerificationStatus.isOk()) {
638-
if (enablePerRpcAuthorityCheck) {
639-
command.stream().setNonExistent();
640-
command.stream().transportReportStatus(
641-
authorityVerificationStatus, RpcProgress.PROCESSED, true, new Metadata());
642-
promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace(
643-
authorityVerificationStatus, null));
644-
return;
645-
}
646-
}
647-
}
648-
} else {
607+
if (authorityHeader == null) {
649608
Status authorityVerificationStatus = Status.UNAVAILABLE.withDescription(
650609
"Missing authority header");
651610
command.stream().setNonExistent();
652611
command.stream().transportReportStatus(
653-
Status.UNAVAILABLE, RpcProgress.DROPPED, true, new Metadata());
612+
Status.UNAVAILABLE, RpcProgress.PROCESSED, true, new Metadata());
654613
promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace(
655614
authorityVerificationStatus, null));
656615
return;
657616
}
617+
// No need to verify authority for the rpc outgoing header if it is same as the authority
618+
// for the transport
619+
if (!authority.contentEquals(authorityHeader)) {
620+
Status authorityVerificationStatus = peerVerificationResults.get(
621+
authorityHeader.toString());
622+
if (authorityVerificationStatus == null) {
623+
if (attributes.get(GrpcAttributes.ATTR_AUTHORITY_VERIFIER) == null) {
624+
authorityVerificationStatus = Status.UNAVAILABLE.withDescription(
625+
"Authority verifier not found to verify authority");
626+
command.stream().setNonExistent();
627+
command.stream().transportReportStatus(
628+
authorityVerificationStatus, RpcProgress.PROCESSED, true, new Metadata());
629+
promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace(
630+
authorityVerificationStatus, null));
631+
return;
632+
}
633+
authorityVerificationStatus = attributes.get(GrpcAttributes.ATTR_AUTHORITY_VERIFIER)
634+
.verifyAuthority(authorityHeader.toString());
635+
peerVerificationResults.put(authorityHeader.toString(), authorityVerificationStatus);
636+
if (!authorityVerificationStatus.isOk() && !enablePerRpcAuthorityCheck) {
637+
logger.log(Level.WARNING, String.format("%s.%s",
638+
authorityVerificationStatus.getDescription(),
639+
enablePerRpcAuthorityCheck
640+
? "" : " This will be an error in the future."),
641+
InternalStatus.asRuntimeExceptionWithoutStacktrace(
642+
authorityVerificationStatus, null));
643+
}
644+
}
645+
if (!authorityVerificationStatus.isOk()) {
646+
if (enablePerRpcAuthorityCheck) {
647+
command.stream().setNonExistent();
648+
command.stream().transportReportStatus(
649+
authorityVerificationStatus, RpcProgress.PROCESSED, true, new Metadata());
650+
promise.setFailure(InternalStatus.asRuntimeExceptionWithoutStacktrace(
651+
authorityVerificationStatus, null));
652+
return;
653+
}
654+
}
655+
}
658656
// Get the stream ID for the new stream.
659657
int streamId;
660658
try {

‎netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -836,7 +836,9 @@ public AsciiString scheme() {
836836
public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
837837
ChannelHandler upgradeHandler =
838838
new Http2UpgradeAndGrpcHandler(grpcHandler.getAuthority(), grpcHandler);
839-
return new WaitUntilActiveHandler(upgradeHandler, grpcHandler.getNegotiationLogger());
839+
ChannelHandler plaintextHandler =
840+
new PlaintextHandler(upgradeHandler, grpcHandler.getNegotiationLogger());
841+
return new WaitUntilActiveHandler(plaintextHandler, grpcHandler.getNegotiationLogger());
840842
}
841843

842844
@Override
@@ -1033,7 +1035,9 @@ static final class PlaintextProtocolNegotiator implements ProtocolNegotiator {
10331035
@Override
10341036
public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
10351037
ChannelHandler grpcNegotiationHandler = new GrpcNegotiationHandler(grpcHandler);
1036-
ChannelHandler activeHandler = new WaitUntilActiveHandler(grpcNegotiationHandler,
1038+
ChannelHandler plaintextHandler =
1039+
new PlaintextHandler(grpcNegotiationHandler, grpcHandler.getNegotiationLogger());
1040+
ChannelHandler activeHandler = new WaitUntilActiveHandler(plaintextHandler,
10371041
grpcHandler.getNegotiationLogger());
10381042
return activeHandler;
10391043
}
@@ -1047,6 +1051,22 @@ public AsciiString scheme() {
10471051
}
10481052
}
10491053

1054+
static final class PlaintextHandler extends ProtocolNegotiationHandler {
1055+
PlaintextHandler(ChannelHandler next, ChannelLogger negotiationLogger) {
1056+
super(next, negotiationLogger);
1057+
}
1058+
1059+
@Override
1060+
protected void protocolNegotiationEventTriggered(ChannelHandlerContext ctx) {
1061+
ProtocolNegotiationEvent existingPne = getProtocolNegotiationEvent();
1062+
Attributes attrs = existingPne.getAttributes().toBuilder()
1063+
.set(GrpcAttributes.ATTR_AUTHORITY_VERIFIER, (authority) -> Status.OK)
1064+
.build();
1065+
replaceProtocolNegotiationEvent(existingPne.withAttributes(attrs));
1066+
fireProtocolNegotiationEvent(ctx);
1067+
}
1068+
}
1069+
10501070
/**
10511071
* Waits for the channel to be active, and then installs the next Handler. Using this allows
10521072
* subsequent handlers to assume the channel is active and ready to send. Additionally, this a

‎netty/src/main/java/io/grpc/netty/X509AuthorityVerifier.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package io.grpc.netty;
1818

19+
import static com.google.common.base.Preconditions.checkNotNull;
20+
1921
import io.grpc.Status;
2022
import io.grpc.internal.AuthorityVerifier;
2123
import java.lang.reflect.InvocationTargetException;
@@ -51,15 +53,15 @@ final class X509AuthorityVerifier implements AuthorityVerifier {
5153
}
5254

5355
public X509AuthorityVerifier(SSLEngine sslEngine, X509TrustManager x509ExtendedTrustManager) {
54-
this.sslEngine = sslEngine;
56+
this.sslEngine = checkNotNull(sslEngine);
5557
this.x509ExtendedTrustManager = x509ExtendedTrustManager;
5658
}
5759

5860
@Override
5961
public Status verifyAuthority(@Nonnull String authority) {
60-
if (sslEngine == null || x509ExtendedTrustManager == null) {
62+
if (x509ExtendedTrustManager == null) {
6163
return Status.UNAVAILABLE.withDescription(
62-
"Can't allow authority override in rpc when SslEngine or X509ExtendedTrustManager"
64+
"Can't allow authority override in rpc when X509ExtendedTrustManager"
6365
+ " is not available");
6466
}
6567
Status peerVerificationStatus;

0 commit comments

Comments
 (0)