-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Import all certificates from propagated bundle #18504
Conversation
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
[crw-ci-test --rebuild] |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me in general. Please ensure that the necessary documentation has been updated. And it looks similar to https://github.com/eclipse/che/pull/17563/files can you take a look?
@skabashnyuk mentioned by you PR solves problem with permissions for java keystore. This PR aimed to improve importing flow to allow importing of whole bundle from a file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we make this change, we can also improve things in downstream so I don't have to patch entrypoint.sh to work in Openshift on RHEL8.
Key parts to fix are noted here, but there are a couple other improvements we can do in #17563 so that there's more explicit file permissions being set in alpine (required in RHEL, implied in Alpine, but I like being explicit so we don't have to assume things).
I've also added an additional inline test to the Dockerfile here, that will fail the docker build if startup script is in the wrong place or permissions are wrong:
I've investigated how we deal with keystores in |
@nickboldt I updated PR and tested it where I could. Please review and, if you have a chance, test. |
Signed-off-by: Mykola Morhun <[email protected]>
Signed-off-by: Mykola Morhun <[email protected]>
Signed-off-by: Mykola Morhun <[email protected]>
Signed-off-by: Mykola Morhun <[email protected]>
Signed-off-by: Mykola Morhun <[email protected]>
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally understood what's changed here, and it makes sense. Fairly confident it'll work in CRW too.
Signed-off-by: Mykola Morhun [email protected]
What does this PR do?
Makes it possible to import several CA certificates from a single file into Che server's java trust store.
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
keytool
imports the only one certificate from the file #18339How to test this PR?
1.1. Generate a few root CA certificates:
1.2. Join them into a single bundle:
$ kubectl label configmap custom-ca app.kubernetes.io/part-of=che.eclipse.org -n che && kubectl label configmap custom-ca app.kubernetes.io/component=ca-bundle -n che
4.1. Exec into Che server's container and go to
/home/user
directory4.2. Make sure that the certs are imported into
cacerts
keystore:$ keytool -list -keystore cacerts -storepass changeit | grep bundle
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.