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
27 changes: 22 additions & 5 deletions packages/grpc-js/src/http_proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@
import { URL } from 'url';
import { log } from './logging';
import { LogVerbosity } from './constants';
import { getDefaultAuthority } from './resolver';
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 +160,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,10 +206,23 @@ export function getProxiedConnection(
' through proxy ' +
proxyAddressString
);
resolve({
socket,
realTarget,
});
// The proxy is connecting to a TLS server, so upgrade
// this socket connection to a TLS connection.
mrfelton marked this conversation as resolved.
Show resolved Hide resolved
if ('secureContext' in connectionOptions) {
const cts = tls.connect({
...connectionOptions,
host: getDefaultAuthority(realTarget),
socket: socket,
}, () => {
resolve({ socket: cts, realTarget });
}
);
} else {
resolve({
socket,
realTarget,
});
}
} else {
log(
LogVerbosity.ERROR,
Expand Down
43 changes: 36 additions & 7 deletions packages/grpc-js/src/subchannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import * as logging from './logging';
import { LogVerbosity } from './constants';
import { getProxiedConnection, ProxyConnectionResult } from './http_proxy';
import * as net from 'net';
import * as tls from 'tls';

const clientVersion = require('../../package.json').version;

Expand Down Expand Up @@ -300,7 +301,9 @@ export class Subchannel {
connectionOptions.servername = sslTargetNameOverride;
}
if (proxyConnectionResult.socket) {
connectionOptions.socket = proxyConnectionResult.socket;
connectionOptions.createConnection = (authority, option) => {
return proxyConnectionResult.socket!;
};
}
} else {
/* In all but the most recent versions of Node, http2.connect does not use
Expand All @@ -309,14 +312,15 @@ export class Subchannel {
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);
}
/* 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 +416,32 @@ export class Subchannel {
}

private startConnectingInternal() {
getProxiedConnection(this.subchannelAddress, this.options).then(
const connectionOptions: http2.SecureClientSessionOptions =
mrfelton marked this conversation as resolved.
Show resolved Hide resolved
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