Support using `https` requests with an `http` proxy #28

davidfestal · 2020-03-18T15:30:55Z

The goal of this PR is to fix issue eclipse-che/che#16356

The error was coming from the fact that axios implementation of https requests through a http proxy is limited. In particular, it seems it doesn't correctly use the CONNECT method that allows tunneling a https request inside a http proxy.
A consequence of this is that the underlying socket connection opened to finally reach the target endpoint is not a ssl socket. This finally results in an error from the Proxy, especially with self-signed certificates.

The fix configures axios to override either http or https agent (according to whether TLS is used) with agents created by the tunnel module.

This PR was tested on a disconnected environment with an HTTP proxy, and TLS enabled with self-signed certificates.

`commit

Signed-off-by: David Festal <[email protected]>

src/index.ts

Signed-off-by: David Festal <[email protected]>

src/index.ts

Conflicts: src/index.ts

davidfestal · 2020-03-18T18:45:51Z

So in order to have the PR ready to merge we still need to:

only add the logging interceptors to the axios instance when some environment variable is setup (and possibly extend this logging mechanism to all returned axios instances)
extract some logic into small methods as @sleshchenko pointed in this comment: Support using https requests with an http proxy #28 (comment)
continue testing it (== integration tests in the context of che-theia on an operator-based install) carefully in a variety of cases (mainly without proxy but with/without TLS, with/without self-signedcerts)
Finally change the package name to switch back to the @eclise-che namespace

davidfestal · 2020-03-18T18:48:17Z

@vinokurig Do you plan to work on those remaining tasks ?

cc @ericwill

src/index.ts

vinokurig · 2020-03-19T09:50:35Z

I'll handle this PR

src/index.ts

vinokurig · 2020-03-19T13:12:41Z

@sleshchenko @olexii4 @akurinnoy The PR is ready for review

akurinnoy

LGTM

davidfestal · 2020-03-19T13:41:57Z

src/index.ts

-            });
-            return axios.create({httpsAgent: agent});
+        if (!this.isItNode()) {
+            this.addLogInterceptorsIfEnabled(axios);


Not sure this makes sense to add interceptor in the browser case, since the process object will not be available ?

I removed the reading of the env variable instead I added a new property to config, so the logging can be controlled by the API

davidfestal · 2020-03-19T16:11:43Z

So last tests results:

without a proxy:
- without TLS : OK
- with TLS and self-signed certs: OK
with a proxy
- without TLS : NOK impossible to join the API server
- with TLS and self-signed certs: OK

So it seems now we lost the access to the the API server with proxies when TLS in not enabled

Additionally, changing the configuration of the request/response logging to push to the che-theia makes it quite difficult to to test the real cause of this failure: the real http error object is hidden from the logs:

2020-03-19 16:03:13.569 root INFO [object Object] 
2020-03-19 16:03:13.573 root ERROR Security problem: Unable to configure separate webviews domain:  Params: Error: Unable to get workspace containers. Cause: Cannot create Che API REST Client
    at CheApiServiceImpl.<anonymous> (/home/theia/node_modules/@eclipse-che/theia-plugin-ext/lib/node/che-api-service.js:292:31)
    at step (/home/theia/node_modules/@eclipse-che/theia-plugin-ext/lib/node/che-api-service.js:38:23)
    at Object.throw (/home/theia/node_modules/@eclipse-che/theia-plugin-ext/lib/node/che-api-service.js:19:53)
    at rejected (/home/theia/node_modules/@eclipse-che/theia-plugin-ext/lib/node/che-api-service.js:11:65)
    at process._tickCallback (internal/process/next_tick.js:68:7)
2020-03-19 16:03:13.590 root ERROR Uncaught Exception:  Params: [object Object]
2020-03-19 16:03:16.234 root INFO PluginTheiaDirectoryHandler: accepting plugin with path Params: /tmp/theia-unpacked/eclipse_che_github_auth_plugin.theia

davidfestal · 2020-03-19T16:26:59Z

src/index.ts

+                        });
+                    }
+                } else {
+                    axiosRequestConfig.httpAgent = proxyIsHttps ? httpsOverHttpsAgent : httpOverHttpsAgent;


afaict here it should be:

axiosRequestConfig.httpAgent = proxyIsHttps ? httpOverHttpsAgent : httpOverHttpAgent,

Of course you would have to define them just above.

TBH I'd rather define all the const just above as you did for httpOverHttpsAgent. To me it would make the logic much clearer and voir this type of errors

davidfestal · 2020-03-19T17:48:03Z

src/index.ts

+                    proxy: mainProxyOptions,
+                    ca: certificateAuthority ? [certificateAuthority] : undefined
+                });
+                const httpsOverHttpsAgent = tunnel.httpsOverHttps(httpsProxyOptions);


I fear that here you will have to also add the ca at the top level, besides the proxyargument.
(that's a case we cannot test for now, but that's how tunnel docs shows in the examples).

ca at the top-level, when target URL is https
ca in the proxy settings, when the proxy itself is https

vinokurig and others added 3 commits March 13, 2020 19:49

2

8fef556

`commit

commit

9430c21

Make it more production ready

64fca32

Signed-off-by: David Festal <[email protected]>

davidfestal requested review from evidolob, l0rd, rhopp and vparfonov as code owners March 18, 2020 15:30

davidfestal requested a review from vinokurig March 18, 2020 15:31

sleshchenko requested review from akurinnoy and olexii4 March 18, 2020 15:34

sleshchenko reviewed Mar 18, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

fixed formatting

e23d681

Signed-off-by: David Festal <[email protected]>

sleshchenko reviewed Mar 18, 2020

View reviewed changes

src/index.ts Show resolved Hide resolved

davidfestal changed the title ~~Make it more production-ready~~ WIP: Make it more production-ready Mar 18, 2020

Code cleanup

b6d975c

Conflicts: src/index.ts

davidfestal changed the base branch from proxyTest to master March 18, 2020 18:12

davidfestal changed the title ~~WIP: Make it more production-ready~~ WIP: Support using https requests with an http proxy Mar 18, 2020

olexii4 reviewed Mar 18, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

vinokurig self-assigned this Mar 19, 2020

akurinnoy reviewed Mar 19, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

akurinnoy reviewed Mar 19, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

akurinnoy reviewed Mar 19, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

fixup! Code cleanup

0006084

vinokurig force-pushed the proxy-tls-fix branch from 45c81d8 to 0006084 Compare March 19, 2020 12:50

vinokurig changed the title ~~WIP: Support using https requests with an http proxy~~ Support using https requests with an http proxy Mar 19, 2020

olexii4 approved these changes Mar 19, 2020

View reviewed changes

akurinnoy approved these changes Mar 19, 2020

View reviewed changes

davidfestal commented Mar 19, 2020

View reviewed changes

vinokurig force-pushed the proxy-tls-fix branch from 61aa8ca to 5921960 Compare March 19, 2020 14:27

fixup! Code cleanup

68aaf8b

vinokurig force-pushed the proxy-tls-fix branch from 5921960 to 68aaf8b Compare March 19, 2020 14:33

davidfestal commented Mar 19, 2020

View reviewed changes

fixup! Code cleanup

b3cbf68

davidfestal commented Mar 19, 2020

View reviewed changes

vinokurig added 2 commits March 19, 2020 20:10

fixup! Code cleanup

c55c21f

fixup! Code cleanup

0bd9b9a

vinokurig merged commit f10f511 into master Mar 19, 2020

vinokurig deleted the proxy-tls-fix branch March 19, 2020 19:54

davidfestal mentioned this pull request Mar 19, 2020

Theia cannot reach the API server with both TLS and proxy enabled eclipse-che/che#16356

Closed

23 tasks

This was referenced Mar 27, 2020

Failed to start Kubernetes runtime of workspace. Cause: Server theia in container theia-ide not available. eclipse-che/che#16439

Closed

Che Theia can't reach Che plugin registry behind the proxy with TLS support eclipse-che/che#16482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support using `https` requests with an `http` proxy #28

Support using `https` requests with an `http` proxy #28

davidfestal commented Mar 18, 2020 •

edited

Loading

davidfestal commented Mar 18, 2020 •

edited

Loading

davidfestal commented Mar 18, 2020

vinokurig commented Mar 19, 2020

vinokurig commented Mar 19, 2020

akurinnoy left a comment

davidfestal Mar 19, 2020

vinokurig Mar 19, 2020

davidfestal commented Mar 19, 2020

davidfestal Mar 19, 2020

vinokurig Mar 19, 2020

davidfestal Mar 19, 2020

vinokurig Mar 19, 2020

Support using https requests with an http proxy #28

Support using https requests with an http proxy #28

Conversation

davidfestal commented Mar 18, 2020 • edited Loading

davidfestal commented Mar 18, 2020 • edited Loading

davidfestal commented Mar 18, 2020

vinokurig commented Mar 19, 2020

vinokurig commented Mar 19, 2020

akurinnoy left a comment

Choose a reason for hiding this comment

davidfestal Mar 19, 2020

Choose a reason for hiding this comment

vinokurig Mar 19, 2020

Choose a reason for hiding this comment

davidfestal commented Mar 19, 2020

davidfestal Mar 19, 2020

Choose a reason for hiding this comment

vinokurig Mar 19, 2020

Choose a reason for hiding this comment

davidfestal Mar 19, 2020

Choose a reason for hiding this comment

vinokurig Mar 19, 2020

Choose a reason for hiding this comment

Support using `https` requests with an `http` proxy #28

Support using `https` requests with an `http` proxy #28

davidfestal commented Mar 18, 2020 •

edited

Loading

davidfestal commented Mar 18, 2020 •

edited

Loading