Support custom public certificates #824

vzhukovs · 2020-08-04T11:42:09Z

What does this PR do?

This changes proposal adds an ability to provide custom certificates.
Custom certificates should be located in /public-certs/*.crt

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

What issues does this PR fix or reference?

eclipse-che/che#17440

Hapy Path Channel

HAPPY_PATH_CHANNEL=stable

vzhukovs · 2020-08-04T11:43:56Z

PR's build can be failed due to separate PR in workspace client library: eclipse-che/che-workspace-client#34

I'll fork workspace-client library with changes and provide custom image for che-theia with changes from current PR and PR from workspace-client repository.

tolusha · 2020-08-04T11:49:15Z

If I understand correctly theia will use '/tmp/che/secret/ca.crt' plus certificates located in /public-certs folder.
It is ok for me.

vzhukovs · 2020-08-04T11:52:31Z

If I understand correctly theia will use '/tmp/che/secret/ca.crt' plus certificates located in /public-certs folder.

That's correct, /tmp/che/secret/ca.crt is using in conjunction with public certificates.

RomanNikitenko

I see that /tmp/che/secret/ca.crt is used here https://github.com/eclipse/che-theia/blob/5155d757a7228f55975f53609d8b8a43668787f0/plugins/workspace-plugin/src/theia-commands.ts#L21

Should we have some changes here?

vzhukovs · 2020-08-04T12:00:56Z

I see that /tmp/che/secret/ca.crt is used here

https://github.com/eclipse/che-theia/blob/5155d757a7228f55975f53609d8b8a43668787f0/plugins/workspace-plugin/src/theia-commands.ts#L21

Should we have some changes here?

Nice catch! I'll add parameters to the curl call. Thanks!

vzhukovs · 2020-08-05T08:27:26Z

I see that /tmp/che/secret/ca.crt is used here

https://github.com/eclipse/che-theia/blob/5155d757a7228f55975f53609d8b8a43668787f0/plugins/workspace-plugin/src/theia-commands.ts#L21

Should we have some changes here?

According to curl documentation it is enough to pass only parameter -k if any certificate is found:

-k, --insecure

(TLS) By default, every SSL connection curl makes is verified to be secure. This option allows curl to proceed and operate even for server connections otherwise considered insecure.

The server connection is verified by making sure the server's certificate contains the right name and verifies successfully using the cert store.

See this online resource for further details:  https://curl.haxx.se/docs/sslcerts.html

See also --proxy-insecure and --cacert.

not sure we should pass every certificate as parameters in this case, so, leave this code as is.

RomanNikitenko · 2020-08-05T08:43:56Z

According to curl documentation it is enough to pass only parameter -k if any certificate is found
not sure we should pass every certificate as parameters in this case, so, leave this code as is.

is it possible that /tmp/che/secret/ca.crt is NOT found,
but
/public-certs is found?

If it's possible, should we pass parameter -k for such case?
https://github.com/eclipse/che-theia/blob/5155d757a7228f55975f53609d8b8a43668787f0/plugins/workspace-plugin/src/theia-commands.ts#L209

vzhukovs · 2020-08-05T09:06:23Z

is it possible that /tmp/che/secret/ca.crt is NOT found

I suppose no, it's impossible. @tolusha is it possible to run che on http environment?

RomanNikitenko

I suppose no, it's impossible.

OK

vzhukovs · 2020-08-05T13:49:34Z

Here is an image for testing purposes: vzhukovs/che-theia:17440. It was built based on eclipse-che/che-workspace-client#34 and #824 changes.

azatsarynnyy · 2020-08-05T14:44:04Z

I believe we can merge this part and the whole flow will be checked once all the other parts are finished within the downstream issue.
@tolusha WDYT?

tolusha · 2020-08-06T07:24:33Z

@azatsarynnyy
Pls merge. If we find any issues we will let you know.

che-bot · 2020-08-06T10:02:38Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:824
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:824

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

vzhukovs · 2020-08-06T11:35:34Z

[ci-build]

vzhukovs · 2020-08-06T13:46:50Z

crw-ci-test

che-bot · 2020-08-27T15:57:27Z

❌ E2E Happy path tests failed ❗

See Details

name	link
che-theia	docker.io/maxura/che-theia:824
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:824

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

mmorhun

I tested this changes on Openshift 4 with institutional (self-signed) certificate and cluster wide proxy which is configured to intercept TLS. Everything works as expected.

azatsarynnyy · 2020-08-27T16:48:15Z

Thank you very much @mmorhun for testing it!

@vzhukovskii now we can proceed with getting it merged:

get the HappyPath tests passed
create a corresponding PR for workspace-client

tolusha · 2020-08-28T08:46:28Z

@azatsarynnyy
Could you merge changes in 7.18.x branch as well ?
In this case the fix will be available in CRW 2.4

azatsarynnyy · 2020-08-28T09:35:56Z

@azatsarynnyy
Could you merge changes in 7.18.x branch as well ?
In this case the fix will be available in CRW 2.4

Sure. That's the plan.
@tolusha thanks for reminding.

vzhukovs · 2020-08-28T15:19:00Z

crw-ci-test

che-bot · 2020-08-28T15:54:49Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:824
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:824

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

davidfestal · 2020-08-28T16:02:13Z

is it possible that /tmp/che/secret/ca.crt is NOT found, but /public-certs is found?

@tolusha According to our today discussion, I would say it is possible, no ?

If the router CA extracted by the Che operator through a dummy route is not seen as self-signed, then the self-signed-certificate secret will not be created, and no /tmp/che/secret/ca.crt will be found, right ?
But I assume this should not prevent an administrator to add custom public certificates to the ca-certs config map and expect to have them propagated up to che-theia, so that the /public-certs would be found ?

davidfestal · 2020-08-28T16:08:59Z

extensions/eclipse-che-theia-plugin-ext/src/node/che-plugin-service.ts

@@ -127,11 +128,11 @@ export class ChePluginServiceImpl implements ChePluginService {
                const httpOverHttpsAgent = tunnel.httpOverHttps({ proxy: httpsProxyOptions });


It seems to me that the httpsProxyOptions might need to have the public-certs as well, since this is the bundle that contains the intercepting proxy CA (coming from the cluster-wide-proxy).

Does it make sense @tolusha ?

The same change would have to be done on the corresponding code in the che-workspace-client library.

extensions/che-theia-hosted-plugin-manager-extension/package.json

tolusha · 2020-08-31T06:58:31Z

@davidfestal

is it possible that /tmp/che/secret/ca.crt is NOT found, but /public-certs is found?

Yes, it is possible accordingly to our discussion.

tolusha · 2020-08-31T07:06:34Z

@davidfestal

It seems to me that the httpsProxyOptions might need to have the public-certs as well, since this is the bundle that contains the > > intercepting proxy CA (coming from the cluster-wide-proxy).
Does it make sense @tolusha ?

It is hard to answer, I am not familiar with that how it works in theia.

che-bot · 2020-09-01T09:34:09Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:824
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:824

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>

vzhukovs · 2020-09-01T09:48:36Z

@davidfestal

is it possible that /tmp/che/secret/ca.crt is NOT found, but /public-certs is found?

Yes, it is possible accordingly to our discussion.

I suppose, it would be better to implement this in separate issue/pr. This is out of scope of current issue. @davidfestal, @tolusha wdyt?

azatsarynnyy · 2020-09-01T10:15:16Z

I suppose, it would be better to implement this in separate issue/pr. This is out of scope of current issue

I'm +1 for making a separate issue for that case.

che-bot · 2020-09-01T10:20:01Z

✅ E2E Happy path tests succeed 🎉

See Details

name	link
che-theia	docker.io/maxura/che-theia:824
che-theia-endpoint-runtime-binary	docker.io/maxura/che-theia-endpoint-runtime-binary:824

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

tolusha · 2020-09-01T10:36:07Z

@azatsarynnyy

is it possible that /tmp/che/secret/ca.crt is NOT found, but /public-certs is found?

Accordingly to private getCertificateAuthority(): you already handle the this case. Am I right?

vzhukovs · 2020-09-01T10:39:19Z

@azatsarynnyy

is it possible that /tmp/che/secret/ca.crt is NOT found, but /public-certs is found?

Accordingly to private getCertificateAuthority(): you already handle the this case. Am I right?

You're right. This logic is in master branch now and in current PR.

vzhukovs · 2020-09-01T12:20:58Z

Are there any objections, which prevent merging this PR to master? @tolusha @davidfestal @azatsarynnyy

tolusha · 2020-09-01T12:22:55Z

I don't have any.

azatsarynnyy · 2020-09-01T12:55:46Z

Are there any objections, which prevent merging this PR to master?

I've already approved it.

…issions (#824) If a sidecar adds any .sh files, check that they are executable to prevent entrypoint errors. Fixes eclipse-che/che#18737 Signed-off-by: Eric Williams <ericwill@redhat.com>

vzhukovs requested review from benoitf, azatsarynnyy, tolusha and RomanNikitenko August 4, 2020 11:42

vzhukovs requested review from ericwill, svor, tsmaeder, vinokurig and vitaliy-guliy as code owners August 4, 2020 11:42

vzhukovs self-assigned this Aug 4, 2020

tolusha approved these changes Aug 4, 2020

View reviewed changes

eclipse-che deleted a comment from che-bot Aug 4, 2020

RomanNikitenko reviewed Aug 4, 2020

View reviewed changes

azatsarynnyy approved these changes Aug 5, 2020

View reviewed changes

azatsarynnyy mentioned this pull request Aug 5, 2020

Editors Sprint 188 eclipse-che/che#17573

Closed

11 tasks

RomanNikitenko self-requested a review August 5, 2020 09:06

RomanNikitenko approved these changes Aug 5, 2020

View reviewed changes

mmorhun approved these changes Aug 27, 2020

View reviewed changes

azatsarynnyy added this to the 7.19 milestone Aug 27, 2020

azatsarynnyy mentioned this pull request Aug 28, 2020

Editors Sprint 189 eclipse-che/che#17707

Closed

13 tasks

davidfestal reviewed Aug 28, 2020

View reviewed changes

vitaliy-guliy reviewed Aug 30, 2020

View reviewed changes

extensions/che-theia-hosted-plugin-manager-extension/package.json Outdated Show resolved Hide resolved

Add an ability to export devfile to file system

acd565a

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>

vzhukovs force-pushed the che#17440 branch from 85120cc to acd565a Compare September 1, 2020 09:46

vzhukovs marked this pull request as ready for review September 1, 2020 09:46

vzhukovs merged commit 11c02f7 into master Sep 1, 2020

vzhukovs deleted the che#17440 branch September 1, 2020 13:00

vzhukovs mentioned this pull request Sep 1, 2020

Add an ability to use custom public certificates #844

Merged

		@@ -127,11 +128,11 @@ export class ChePluginServiceImpl implements ChePluginService {
		const httpOverHttpsAgent = tunnel.httpOverHttps({ proxy: httpsProxyOptions });

Support custom public certificates #824

Support custom public certificates #824

Conversation

vzhukovs commented Aug 4, 2020

What does this PR do?

What issues does this PR fix or reference?

Hapy Path Channel

vzhukovs commented Aug 4, 2020

tolusha commented Aug 4, 2020

vzhukovs commented Aug 4, 2020 • edited Loading

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

vzhukovs commented Aug 4, 2020

vzhukovs commented Aug 5, 2020

RomanNikitenko commented Aug 5, 2020

vzhukovs commented Aug 5, 2020

RomanNikitenko left a comment

Choose a reason for hiding this comment

vzhukovs commented Aug 5, 2020

azatsarynnyy commented Aug 5, 2020

tolusha commented Aug 6, 2020

che-bot commented Aug 6, 2020

vzhukovs commented Aug 6, 2020

vzhukovs commented Aug 6, 2020

che-bot commented Aug 27, 2020

mmorhun left a comment

Choose a reason for hiding this comment

azatsarynnyy commented Aug 27, 2020

tolusha commented Aug 28, 2020

azatsarynnyy commented Aug 28, 2020

vzhukovs commented Aug 28, 2020

che-bot commented Aug 28, 2020

davidfestal commented Aug 28, 2020

davidfestal Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

tolusha commented Aug 31, 2020 • edited Loading

tolusha commented Aug 31, 2020

che-bot commented Sep 1, 2020

vzhukovs commented Sep 1, 2020

azatsarynnyy commented Sep 1, 2020

che-bot commented Sep 1, 2020

tolusha commented Sep 1, 2020

vzhukovs commented Sep 1, 2020

vzhukovs commented Sep 1, 2020

tolusha commented Sep 1, 2020

azatsarynnyy commented Sep 1, 2020

vzhukovs commented Aug 4, 2020 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

davidfestal Aug 28, 2020 •

edited

Loading

tolusha commented Aug 31, 2020 •

edited

Loading