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

Always propogate router certificate into trust store #285

Closed
wants to merge 3 commits into from

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented May 29, 2020

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What this PR does

Deprecates selfSignedCert option. CR value is present but actually has no effect.
In case of Openshift router certificate will always be propagated to Che trust store.
In case of Kubernetes certificate is generated and propagated into Che trust store unless use provides single TLS secret (no self signed one): then we consider certificate commonly trusted.

Related issues:

eclipse-che/che#16764

Tests

Tested on the following infrastructures with self-signed certificates:

  • Minikube
  • CRC
  • Openshift (with oAuth)

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

OK with that if it has been deeply tested with running workspaces in various combinations:

  • TLS on a cluster with self-signed routes
  • TLS on a cluster with a valid certificate in the router.

I somehow remember that, long ago, enabling selfsignedcerts (=> adding the self-signed certificate chain extract from a dummy route) when it was not necessary (valid cluster router CA) was leading to problems as well.

pkg/controller/che/tls-secrets.go Show resolved Hide resolved
@@ -65,6 +65,7 @@ replace (
)

require (
github.com/PuerkitoBio/purell v1.1.1 // indirect
Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr May 29, 2020

Choose a reason for hiding this comment

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

I think these unused dependencies spawns older go. We have similar discussion eclipse-che/che-machine-exec#83 (comment) I think you should use a bit newer go to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

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

With newer go: go mod tidy and you will see that newer go will remove these deps from go go.mod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes is come when I build operator image in docker.

Copy link
Contributor

@AndrienkoAleksandr AndrienkoAleksandr left a comment

Choose a reason for hiding this comment

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

Looks good. Only one minor comment.

@mmorhun
Copy link
Contributor Author

mmorhun commented May 29, 2020

@davidfestal do you remember what kind of problems causes an attempt to add commonly trusted certificate into trust store?

@centos-ci
Copy link

Operator updates test passed

@mmorhun
Copy link
Contributor Author

mmorhun commented May 29, 2020

Rebased

README.md Outdated Show resolved Hide resolved
…icate into trust store.

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun
Copy link
Contributor Author

mmorhun commented Jun 1, 2020

Rebased

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@centos-ci
Copy link

Operator updates test passed

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@centos-ci
Copy link

Operator updates test passed

@tolusha tolusha mentioned this pull request Jun 1, 2020
34 tasks
@mmorhun
Copy link
Contributor Author

mmorhun commented Jun 4, 2020

Closing this in favour of #301 because propagation of router certificate all the time leads to malfunction of some components in case of commonly trusted certificate.

@mmorhun mmorhun closed this Jun 4, 2020
@mmorhun mmorhun deleted the che-16764 branch June 5, 2020 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants