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

mrfelton · 2020-04-18T09:08:35Z

Trying to get ssl connection to establish correctly when connecting via proxy.

In reference to #992

murgatroid99 · 2020-04-18T15:54:02Z

packages/grpc-js/src/http_proxy.ts

-          realTarget,
+        var cts = tls.connect({
+            ...connectionOptions,
+            host: options.host,


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?

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

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.

murgatroid99 · 2020-04-18T16:07:03Z

packages/grpc-js/src/subchannel.ts

@@ -302,21 +302,20 @@ export class Subchannel {
      if (proxyConnectionResult.socket) {
        connectionOptions.socket = proxyConnectionResult.socket;


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

murgatroid99 · 2020-04-18T16:09:05Z

packages/grpc-js/src/subchannel.ts

+        /* 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);


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

Let's not worry about this for the moment.

Have updated this so that we still have the plaintext conditional

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.

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

mrfelton · 2020-04-18T16:53:59Z

Do I need to do something special to get all of the tests to pass? I get the following results:

  123 passing (587ms)
  2 pending
  27 failing

This is the same result I get in master branch.

murgatroid99 · 2020-04-18T17:21:18Z

You should undo that last commit. Using the target name override there is correct, that option just shouldn't be necessary in most cases. In general, when using the grpc-js API, you should not be passing the grpc.ssl_target_name_override option at all; it is mainly good for testing.

murgatroid99 · 2020-04-18T17:23:47Z

I've been making these small corrections, but overall I still don't understand how this change could cause any difference in behavior from what the existing code does. It looks to me like it is simply pulling out part of the behavior of http2.connect into the proxy handling code.

mrfelton · 2020-04-18T17:51:55Z

To be honest, I as hoping you'd be able to explain it! Somehow this forces the tls connection to the real target to be established.

mrfelton · 2020-04-18T19:03:24Z

I've pushed another update to this. I think this resolves the issues in your previous comments.

My basic understanding is that when you initially set up the connection to the proxy it doesn't support tls, but if the proxy is connecting to a TLS server the socket needs upgrading to a TLS connection.

Does that makes sense fro your perspective?

murgatroid99 · 2020-04-18T20:16:46Z

The thing is that http2.connect already calls tls.connect internally with the options passed to it if it's connecting to an https address. So the original code that passes the socket from the proxy in the http2.connect options should be doing the exact same thing that this code is doing.

mrfelton · 2020-04-18T20:18:14Z

I know in theory that is supposed to be the case, but in practice it doesn't. Or something about the way we are calling http2.connect is preventing that from working as it should.

murgatroid99 · 2020-04-18T20:18:44Z

packages/grpc-js/src/subchannel.ts

-        connectionOptions.socket = proxyConnectionResult.socket;
+        return proxyConnectionResult.socket;
+      } else if ('secureContext' in connectionOptions) {
+        return tls.connect(this.subchannelAddress);


This isn't better because you're not passing in the options. When connecting with TLS and without a proxy, it should simply not be passing a createConnection option at all.

My typescript is not up to par :( What I really wanted to do in the ssl block was like this:

if (proxyConnectionResult.socket) { connectionOptions.createConnection = (authority, option) => { return proxyConnectionResult.socket; }; }

But I get typescript errors.

Ah, the issue is that TypeScript thinks that proxyConnectionResult.socket might become null between when you check it and when the callback is called. This should work:

if (proxyConnectionResult.socket) { connectionOptions.createConnection = (authority, option) => { return proxyConnectionResult.socket!; //Exclamation mark here }; }

Nice one! updated.

murgatroid99 · 2020-04-18T20:20:59Z

I'm not talking about "in theory", I'm talking about the http2.connect source code

mrfelton · 2020-04-18T20:24:31Z

I'm not talking about "in theory", I'm talking about the http2.connect source code

Understood - but in reality it simply doesn't work the way it was.

murgatroid99 · 2020-04-18T20:38:08Z

So, one difference between what you're doing here and what http.connect is doing is that initializeTLSOptions function call before passing the options to tls.connect. What if you copy that function (and the initializeOptions function that it calls) and call it the same way in the proxy code? Does your request through TOR still work?

mrfelton · 2020-04-18T20:57:42Z

Yes, the tor connection works correctly even after wrapping my tls.connect options in initializeTLSOptions

mrfelton · 2020-04-18T21:08:01Z

If I call like this it still works:

          const cts = tls.connect(Number(options.port), getDefaultAuthority(realTarget), initializeTLSOptions({ ...connectionOptions, socket}, getDefaultAuthority(realTarget)), () => {
            resolve({ socket: cts, realTarget });
          });

So wrapping the options in initializeTLSOptions doesn't seem to make a difference here.

…ut a proxy

murgatroid99 · 2020-04-18T21:25:57Z

I have another idea: log calls to tls.connect by monkey patching it like this:

const originalTlsConnect = tls.connect;
tls.connect = (...args) => {
  console.log(args);
  return originalTlsConnect(...args);
};

Then check what the output is for your test case, both with this change (including the version with initializeTlsOptions, to minimize confounding factors) and with the original code.

mrfelton · 2020-04-18T21:42:00Z

My version (works):

[
  9065,
  'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion',
  {
    secureContext: SecureContext { context: SecureContext {} },
    socket: Socket {
      connecting: false,
      _hadError: false,
      _parent: null,
      _host: null,
      _readableState: [ReadableState],
      readable: true,
      _events: [Object: null prototype] {},
      _eventsCount: 0,
      _maxListeners: undefined,
      _writableState: [WritableState],
      writable: true,
      allowHalfOpen: false,
      _sockname: null,
      _pendingData: null,
      _pendingEncoding: '',
      server: null,
      _server: null,
      parser: null,
      _httpMessage: null,
      [Symbol(asyncId)]: 133,
      [Symbol(kHandle)]: [TCP],
      [Symbol(lastWriteQueueSize)]: 0,
      [Symbol(timeout)]: null,
      [Symbol(kBuffer)]: null,
      [Symbol(kBufferCb)]: null,
      [Symbol(kBufferGen)]: null,
      [Symbol(kBytesRead)]: 0,
      [Symbol(kBytesWritten)]: 0
    },
    settings: {},
    Http1IncomingMessage: [Function: IncomingMessage],
    Http1ServerResponse: [Function: ServerResponse],
    Http2ServerRequest: [Function: Http2ServerRequest],
    Http2ServerResponse: [Function: Http2ServerResponse],
    ALPNProtocols: [ 'h2' ],
    servername: 'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion'
  },
  [Function]
]

Old version (doesn't work):

[
  '443',
  'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion',
  {
    secureContext: SecureContext { context: SecureContext {} },
    socket: Socket {
      connecting: false,
      _hadError: false,
      _parent: null,
      _host: null,
      _readableState: [ReadableState],
      readable: true,
      _events: [Object: null prototype] {},
      _eventsCount: 0,
      _maxListeners: undefined,
      _writableState: [WritableState],
      writable: true,
      allowHalfOpen: false,
      _sockname: null,
      _pendingData: null,
      _pendingEncoding: '',
      server: null,
      _server: null,
      parser: null,
      _httpMessage: null,
      [Symbol(asyncId)]: 135,
      [Symbol(kHandle)]: [TCP],
      [Symbol(lastWriteQueueSize)]: 0,
      [Symbol(timeout)]: null,
      [Symbol(kBuffer)]: null,
      [Symbol(kBufferCb)]: null,
      [Symbol(kBufferGen)]: null,
      [Symbol(kBytesRead)]: 0,
      [Symbol(kBytesWritten)]: 0
    },
    host: '127.0.0.1',
    port: 9065,
    settings: {},
    Http1IncomingMessage: [Function: IncomingMessage],
    Http1ServerResponse: [Function: ServerResponse],
    Http2ServerRequest: [Function: Http2ServerRequest],
    Http2ServerResponse: [Function: Http2ServerResponse],
    ALPNProtocols: [ 'h2' ],
    servername: 'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion'
  }
]

Different host and port

murgatroid99 · 2020-04-18T21:56:07Z

OK, it should be pretty simple to change that: in the unmodified code, just put this line in a if (!proxyConnectionResult.socket) block. Does that make your code work?

mrfelton · 2020-04-18T22:08:10Z

Unfortunately not. I have also tried other things to make the tls.connect from within http2.connect be called in exactly the same way, and even when the args are the same as in my version it still doesn't work.

murgatroid99 · 2020-04-18T22:25:29Z

Just to verify in the other direction, if you modify your version of the code to pass the same arguments that the old code passes, your version still works?

mrfelton · 2020-04-18T22:43:23Z

Yes, it does.

i.e. I can change to this and it still works:

const cts = tls.connect(443, getDefaultAuthority(realTarget), initializeTLSOptions({ ...connectionOptions, socket, host: '127.0.0.1', port: 9065}, getDefaultAuthority(realTarget)), () => {
  resolve({ socket: cts, realTarget });
});

Which gives me call opts that look like those from the original code:

[
  443,
  'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion',
  {
    secureContext: SecureContext { context: SecureContext {} },
    socket: Socket {
      connecting: false,
      _hadError: false,
      _parent: null,
      _host: null,
      _readableState: [ReadableState],
      readable: true,
      _events: [Object: null prototype] {},
      _eventsCount: 0,
      _maxListeners: undefined,
      _writableState: [WritableState],
      writable: true,
      allowHalfOpen: false,
      _sockname: null,
      _pendingData: null,
      _pendingEncoding: '',
      server: null,
      _server: null,
      parser: null,
      _httpMessage: null,
      [Symbol(asyncId)]: 134,
      [Symbol(kHandle)]: [TCP],
      [Symbol(lastWriteQueueSize)]: 0,
      [Symbol(timeout)]: null,
      [Symbol(kBuffer)]: null,
      [Symbol(kBufferCb)]: null,
      [Symbol(kBufferGen)]: null,
      [Symbol(kBytesRead)]: 0,
      [Symbol(kBytesWritten)]: 0
    },
    host: '127.0.0.1',
    port: 9065,
    settings: {},
    Http1IncomingMessage: [Function: IncomingMessage],
    Http1ServerResponse: [Function: ServerResponse],
    Http2ServerRequest: [Function: Http2ServerRequest],
    Http2ServerResponse: [Function: Http2ServerResponse],
    ALPNProtocols: [ 'h2' ],
    servername: 'zapn34qfeedw2l5y26p3hnnkusqnbhxcxw64lq5cojmvq45yw4bc3sqd.onion'
  },
  [Function]
]

There is a different though, which is the callback function as the 4th arg.

It seems to me that in my version when calling tls.connect from within request.once('connect' in the proxy module it waits for the socket to be established before resolving it from getProxiedConnection. Which in turn means that the socket that gets passed to http2.connect is already fully established and tls enabled.

In http2 core they don't call tls.connect with a callback.

They will however call a listener that you can pass to http2.connect as a third argument, which to me looks like it would call once the socket has been upgraded for tls. See here:

https://github.com/nodejs/node/blob/ef2df6986b6a3c34c6a0a97c120d5f8d05c4a1ce/lib/internal/http2/core.js#L2952-L2953

However we don't pass in a `listener third arg.

I may be barking up the wrong tree, but it seems that our socket is just not ready to use with tls before we start trying to use it.

murgatroid99 · 2020-04-19T01:34:13Z

It should be pretty easy to check if passing a callback argument to http2.connect is the issue. The session.once('connect') callback from a few lines down could be passed directly as the third argument to http2.connect.

In the other direction, what if in this PR you have the proxy code return the TLS socket immediately, instead of passing a callback?

murgatroid99 · 2020-04-19T01:59:59Z

I think I may have a smoking gun: the Http2Session constructor waits for a TLSSocket's secureConnect event if socket.connecting is true, but continues immediately otherwise. (https://github.com/nodejs/node/blob/ef2df6986b6a3c34c6a0a97c120d5f8d05c4a1ce/lib/internal/http2/core.js#L1084-L1090). However, if the TLSSocket constructor is passed a socket, then it only sets its own connecting value to true if the underlying socket's connecting is also true, but it emits connect, not secureConnect at that time. (https://github.com/nodejs/node/blob/master/lib/_tls_wrap.js#L791-L800). This looks like a mismatch of expectations: Http2Session is expecting that connecting means "has not yet emitted the secureConnect event", but in this situation TLSSocket is using it to mean "has not yet emitted the connect event".

If this is true, passing a callback to http.connect will not fix the problem, and returning the result of tls.connect without waiting for it to finish will make the code in this PR behave like the broken version.

murgatroid99 · 2020-04-19T05:06:49Z

See nodejs/node#32922 for a demonstration of the problem I described independent of both grpc and HTTP CONNECT.

mrfelton · 2020-04-19T07:20:00Z

It should be pretty easy to check if passing a callback argument to http2.connect is the issue. The session.once('connect') callback from a few lines down could be passed directly as the third argument to http2.connect.

Does't work this way.

In the other direction, what if in this PR you have the proxy code return the TLS socket immediately, instead of passing a callback?

Also does't work this way.

See nodejs/node#32922 for a demonstration of the problem I described independent of both grpc and HTTP CONNECT.

Wow ok, so this really does look like a bug in node itself. With that in mind, how do you feel about the workaround proposed in this PR?

murgatroid99

Now that I understand what the problem is, I'm OK with adding the workaround, with a few changes.

murgatroid99 · 2020-04-19T15:42:19Z

packages/grpc-js/src/subchannel.ts

+        /* 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);


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

packages/grpc-js/src/subchannel.ts

packages/grpc-js/src/http_proxy.ts

mrfelton · 2020-04-19T18:11:13Z

Thanks. Have made those updates.

murgatroid99

Thank you so much for working through this with me.

mrfelton · 2020-04-20T17:53:42Z

Thanks for your help - solving this and making it possible to do grpc over tor is a big deal. 👍

mrfelton · 2020-04-21T07:02:01Z

Unfortunately this no longer works after merging :(

mrfelton · 2020-04-21T07:25:41Z

I think it's this PR that conflicts #1364

murgatroid99 · 2020-04-21T17:07:28Z

I think I fixed the conflict in #1375.

murgatroid99 · 2020-04-21T23:36:20Z

Version 1.0.1 has that fix.

mrfelton · 2020-04-22T18:16:50Z

progress, but still not working unfortunately. Now getting Received RST_STREAM with code 2 error and the tls session again doesn't look to being getting established correctly.

grpc-js: initiate tls connection through http proxy

af7f4f7

mrfelton mentioned this pull request Apr 18, 2020

GRPC connecting over TOR #992

Closed

mrfelton added 2 commits April 18, 2020 12:56

grpc-js: always explicitly establish connection

c650e59

grpc-js: pass secureContext through to proxied tls connection

5af582e

murgatroid99 reviewed Apr 18, 2020

View reviewed changes

grpc-js: do not set host when instantiating tls socket

4e61f21

murgatroid99 reviewed Apr 18, 2020

View reviewed changes

mrfelton force-pushed the proxy-tls branch from a3a7534 to 4e6cb64 Compare April 18, 2020 18:57

grpc-js: ensure tls connection is used when requested

2c5a8b1

mrfelton force-pushed the proxy-tls branch from 4e6cb64 to 2c5a8b1 Compare April 18, 2020 19:25

murgatroid99 reviewed Apr 18, 2020

View reviewed changes

grpc-js: dont set createConnection when connecting with TLS and witho…

11965fb

…ut a proxy

mrfelton force-pushed the proxy-tls branch from 3eadd5a to 11965fb Compare April 18, 2020 21:16

murgatroid99 requested changes Apr 19, 2020

View reviewed changes

mrfelton added 3 commits April 19, 2020 20:07

grpc-js: commenting working for node issue 32922

b9e84f4

grpc-js: setting ALPNProtocols option for tls proxy

48072d5

grpc-js: use tls.ConnectionOptions type for proxy connection options

eef75a5

mrfelton requested a review from murgatroid99 April 19, 2020 18:10

murgatroid99 added the kokoro:run label Apr 20, 2020

kokoro-team removed the kokoro:run label Apr 20, 2020

murgatroid99 approved these changes Apr 20, 2020

View reviewed changes

murgatroid99 merged commit c5424a5 into grpc:master Apr 20, 2020

mrfelton mentioned this pull request Apr 22, 2020

grpc-js: do not prematurely resolve proxy socket #1379

Closed

		@@ -302,21 +302,20 @@ export class Subchannel {
		if (proxyConnectionResult.socket) {
		connectionOptions.socket = proxyConnectionResult.socket;

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

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

Conversation

mrfelton commented Apr 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murgatroid99 Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrfelton commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020

mrfelton commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020

Choose a reason for hiding this comment

mrfelton Apr 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murgatroid99 commented Apr 18, 2020 • edited Loading

mrfelton commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020

mrfelton commented Apr 18, 2020 • edited Loading

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020

murgatroid99 commented Apr 18, 2020

mrfelton commented Apr 18, 2020 • edited Loading

murgatroid99 commented Apr 19, 2020

murgatroid99 commented Apr 19, 2020

murgatroid99 commented Apr 19, 2020

mrfelton commented Apr 19, 2020

murgatroid99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrfelton commented Apr 19, 2020

murgatroid99 left a comment

Choose a reason for hiding this comment

mrfelton commented Apr 20, 2020

mrfelton commented Apr 21, 2020

mrfelton commented Apr 21, 2020 • edited Loading

murgatroid99 commented Apr 21, 2020

murgatroid99 commented Apr 21, 2020

mrfelton commented Apr 22, 2020

murgatroid99 Apr 18, 2020 •

edited

Loading

mrfelton Apr 18, 2020 •

edited

Loading

murgatroid99 commented Apr 18, 2020 •

edited

Loading

mrfelton commented Apr 18, 2020 •

edited

Loading

mrfelton commented Apr 18, 2020 •

edited

Loading

mrfelton commented Apr 21, 2020 •

edited

Loading