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.

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

grpc-js: initiate tls connection through http proxy #1369

Merged
merged 9 commits into from
Apr 20, 2020
16 changes: 12 additions & 4 deletions packages/grpc-js/src/http_proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import { LogVerbosity } from './constants';
import { parseTarget } from './resolver-dns';
import { Socket } from 'net';
import * as http from 'http';
import * as http2 from 'http2';
mrfelton marked this conversation as resolved.
Show resolved Hide resolved
import * as tls from 'tls'
import * as logging from './logging';
import {
SubchannelAddress,
Expand Down Expand Up @@ -157,7 +159,8 @@ export interface ProxyConnectionResult {

export function getProxiedConnection(
address: SubchannelAddress,
channelOptions: ChannelOptions
channelOptions: ChannelOptions,
connectionOptions: http2.SecureClientSessionOptions
mrfelton marked this conversation as resolved.
Show resolved Hide resolved
): Promise<ProxyConnectionResult> {
if (!('grpc.http_connect_target' in channelOptions)) {
return Promise.resolve<ProxyConnectionResult>({});
Expand Down Expand Up @@ -202,9 +205,14 @@ export function getProxiedConnection(
' through proxy ' +
proxyAddressString
);
resolve({
socket,
realTarget,
var cts = tls.connect({
...connectionOptions,
socket: socket
}, function () {
resolve({
socket: cts,
realTarget,
});
});
} else {
log(
Expand Down
50 changes: 35 additions & 15 deletions packages/grpc-js/src/subchannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,21 +302,20 @@ export class Subchannel {
if (proxyConnectionResult.socket) {
connectionOptions.socket = proxyConnectionResult.socket;
Copy link
Member

@murgatroid99 murgatroid99 Apr 18, 2020

Choose a reason for hiding this comment

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

With your changes, this line should do nothing because the createConnection option below takes precedence.

}
} else {
/* In all but the most recent versions of Node, http2.connect does not use
* the options when establishing plaintext connections, so we need to
* establish that connection explicitly. */
connectionOptions.createConnection = (authority, option) => {
if (proxyConnectionResult.socket) {
return proxyConnectionResult.socket;
} else {
/* net.NetConnectOpts is declared in a way that is more restrictive
* than what net.connect will actually accept, so we use the type
* assertion to work around that. */
return net.connect(this.subchannelAddress);
}
};
}
/* In all but the most recent versions of Node, http2.connect does not use
* the options when establishing plaintext connections, so we need to
* establish that connection explicitly. */
connectionOptions.createConnection = (authority, option) => {
if (proxyConnectionResult.socket) {
return proxyConnectionResult.socket;
} else {
/* net.NetConnectOpts is declared in a way that is more restrictive
* than what net.connect will actually accept, so we use the type
* assertion to work around that. */
return net.connect(this.subchannelAddress);
Copy link
Member

Choose a reason for hiding this comment

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

Doing this unconditionally breaks the unproxied TLS case, because you are forcing the use of a plaintext connection here.

Copy link
Member

Choose a reason for hiding this comment

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

Let's not worry about this for the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have updated this so that we still have the plaintext conditional

Copy link
Member

Choose a reason for hiding this comment

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

You have the exact same createConnection function in the secure and insecure case. The problem was, and is, that you're always doing net.connect, which creates a plain TCP connection. In the secure case, you should only be using the createConnection to pass in the socket from the proxy.

Copy link
Member

Choose a reason for hiding this comment

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

This section of the change now doesn't functionally change anything, so I'd rather not change it at all.

}
};
connectionOptions = Object.assign(
connectionOptions,
this.subchannelAddress
Expand Down Expand Up @@ -412,7 +411,28 @@ export class Subchannel {
}

private startConnectingInternal() {
getProxiedConnection(this.subchannelAddress, this.options).then(
let connectionOptions: http2.SecureClientSessionOptions =
this.credentials._getConnectionOptions() || {};

if ('secureContext' in connectionOptions) {
mrfelton marked this conversation as resolved.
Show resolved Hide resolved
// If provided, the value of grpc.ssl_target_name_override should be used
// to override the target hostname when checking server identity.
// This option is used for testing only.
if (this.options['grpc.ssl_target_name_override']) {
const sslTargetNameOverride = this.options[
'grpc.ssl_target_name_override'
]!;
connectionOptions.checkServerIdentity = (
host: string,
cert: PeerCertificate
): Error | undefined => {
return checkServerIdentity(sslTargetNameOverride, cert);
};
connectionOptions.servername = sslTargetNameOverride;
}
}

getProxiedConnection(this.subchannelAddress, this.options, connectionOptions).then(
(result) => {
this.createSession(result);
},
Expand Down