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

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

Merged
merged 9 commits into from
Apr 20, 2020
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
af7f4f7
grpc-js: initiate tls connection through http proxy
mrfelton Apr 18, 2020
c650e59
grpc-js: always explicitly establish connection
mrfelton Apr 18, 2020
5af582e
grpc-js: pass secureContext through to proxied tls connection
mrfelton Apr 18, 2020
4e61f21
grpc-js: do not set host when instantiating tls socket
mrfelton Apr 18, 2020
2c5a8b1
grpc-js: ensure tls connection is used when requested
mrfelton Apr 18, 2020
11965fb
grpc-js: dont set createConnection when connecting with TLS and witho…
mrfelton Apr 18, 2020
b9e84f4
grpc-js: commenting working for node issue 32922
mrfelton Apr 19, 2020
48072d5
grpc-js: setting ALPNProtocols option for tls proxy
mrfelton Apr 19, 2020
eef75a5
grpc-js: use tls.ConnectionOptions type for proxy connection options
mrfelton Apr 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions packages/grpc-js/src/http_proxy.ts
View file
Open in desktop
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,15 @@ export function getProxiedConnection(
' through proxy ' +
proxyAddressString
);
resolve({
socket,
realTarget,
var cts = tls.connect({
...connectionOptions,
host: options.host,
Copy link
Member

Choose a reason for hiding this comment

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

options.host here is the IP address of the proxy, and when socket is set, host is only used to validate the server certificates. Why would you want to validate the server certificates against the proxy's IP address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Also works without setting host - have pushed update that removes.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think you want host: getDefaultAuthority(realTarget) here, so that you don't have to pass the grpc.ssl_target_name_override option when constructing the client. In fact, TLS certificate validation is the whole reason we pass the default authority to http2.connect in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

socket: socket
}, function () {
resolve({
socket: cts,
realTarget,
});
});
} else {
log(
Expand Down
50 changes: 35 additions & 15 deletions packages/grpc-js/src/subchannel.ts
View file
Open in desktop
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 •
edited
Loading

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