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

feat: Allow java versioning for cacert #3700

Open
wants to merge 4 commits into
base: release-0.60.0
Choose a base branch
from

Conversation

JGodin-C2C
Copy link
Contributor

@JGodin-C2C JGodin-C2C commented Jun 28, 2024

What

Allow to choose between java versions for the path of CACert

Why

Depending on the java version that is used in the container, we may need to change the path of the cacert

Further thought

This PR uses code that was already present.
I feel we could improve the lisibility by simply passing the cacert mountpath.
Pushing a new PR would be easy and remove some unused code
What is your opinion on this ?

Allow to choose between java versions for the path of CACert

Signed-off-by: Julien Godin <[email protected]>
@JGodin-C2C JGodin-C2C requested a review from a team as a code owner June 28, 2024 12:16
@JGodin-C2C
Copy link
Contributor Author

JGodin-C2C commented Jul 4, 2024

Is it okay to ping some of you guys ? @rokroskar @ableuler @aledegano for a review ?
Or should i wait a lil' bit more ?

@rokroskar
Copy link
Member

Thanks @JGodin-C2C! @olevski @aledegano could you please review?

@rokroskar rokroskar requested review from olevski and aledegano July 4, 2024 08:25
@aledegano
Copy link
Contributor

Will do ASAP

@aledegano
Copy link
Contributor

I think a little bit more context could be helpful here:

  • It seems like this change is needed when running a commit-event-service container whose image is not the one we (Renku/SDSC) create, is that correct? Why is that needed?
  • All "knowledge-graph" containers have the same parameter in their deployment template, shouldn't we modify those too?
  • Yes, I agree that whatever the case might be we need to go for a cleaner solution: it took me way too long to figure out both what the existing code was doing and what this PR was changing 🤣

@JGodin-C2C
Copy link
Contributor Author

Hey @aledegano

I think a little bit more context could be helpful here:

* It seems like this change is needed when running a `commit-event-service` container whose image **is not** the one we (Renku/SDSC) create, is that correct? Why is that needed?

We need to inject the CA in order to get java to query some https servers using custom CAs

* All "knowledge-graph" containers have the same parameter in their deployment template, shouldn't we modify those too?

I'm sorry, i only seeing this one. Am i missing something ?

* Yes, I agree that whatever the case might be we need to go for a cleaner solution: it took me way too long to figure out both what the existing code was doing and what this PR was changing 🤣

My two cents : we should remove these "javacert" from this .tmpl as it is used nowhere at the moment and make it more flexible. Should i commit on this PR or open a new one and close this one ?

@aledegano
Copy link
Contributor

Hey @aledegano

I think a little bit more context could be helpful here:

* It seems like this change is needed when running a `commit-event-service` container whose image **is not** the one we (Renku/SDSC) create, is that correct? Why is that needed?

We need to inject the CA in order to get java to query some https servers using custom CAs

We do support a way to inject custom CA:
https://github.com/SwissDataScienceCenter/renku/blob/master/helm-chart/renku/values.yaml#L197

The parameter that you are modifying in this PR changes the path that Java refers to when including custom CAs, in other words (as far as I understand...) this parameter is only needed when the Java version within the container changes, which it shouldn't unless you are building your own image of this specific service running on a different Java version.

* All "knowledge-graph" containers have the same parameter in their deployment template, shouldn't we modify those too?

I'm sorry, i only seeing this one. Am i missing something ?

Yes, this other templates also refer the same parameter:
image

* Yes, I agree that whatever the case might be we need to go for a cleaner solution: it took me way too long to figure out both what the existing code was doing and what this PR was changing 🤣

My two cents : we should remove these "javacert" from this .tmpl as it is used nowhere at the moment and make it more flexible. Should i commit on this PR or open a new one and close this one ?

Let's figure out exactly what we are trying to achieve first! 😁

@JGodin-C2C
Copy link
Contributor Author

⚠️
The actual deployed chart is version 0.42.1 , have this been reported to you before as a bug and fixed in a future version ? Looked for anything related, but to no available. (i am trying to upgrade)
⚠️

(I feel i should open an issue)
Hey @aledegano i already am using this parameter to inject custom CAs.
So, here is my case :

I have a renku already deployed.

I already am using the customCAs key and the CAs are available in all my pods system. However, it seems that the java cacert is not updated.
Your last docker image for commit-event-service : docker run -it --entrypoint /bin/bash renku/commit-event-service:2.50.0
uses openjdk /opt/java/openjdk/bin/java and the cacert for this version of java is located in /opt/java/openjdk/lib/security/cacerts if i'm not mistaking.

However, when this pod is trying to contact my gitlab instance, it throws me an error :

2024-07-05 12:17:29,395 ERROR application - Scheduled process failed
io.renku.http.client.RestClientError$ClientException: GET https://[THE CORRECT GITLAB URL]/api/v4/projects/38/repository/commits?per_page=1 error: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
	at io.renku.http.client.RestClient$$anonfun$connectionError$1.applyOrElse(RestClient.scala:194)
	at io.renku.http.client.RestClient$$anonfun$connectionError$1.applyOrElse(RestClient.scala:183)
	at cats.ApplicativeError.$anonfun$recoverWith$1(ApplicativeError.scala:172)
	at modify @ io.renku.metrics.MetricsRegistry$EnabledMetricsRegistry.register(MetricsRegistry.scala:70)
	at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
	at uncancelable @ fs2.Compiler$Target.uncancelable(Compiler.scala:165)
	at >>$extension @ fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1242)
	at modify @ io.renku.metrics.MetricsRegistry$EnabledMetricsRegistry.register(MetricsRegistry.scala:70)
	at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
	at uncancelable @ fs2.Compiler$Target.uncancelable(Compiler.scala:165)
	at >>$extension @ fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1242)
	at modify @ io.renku.metrics.MetricsRegistry$EnabledMetricsRegistry.register(MetricsRegistry.scala:70)
	at flatMap @ fs2.Compiler$Target.flatMap(Compiler.scala:163)
	at uncancelable @ fs2.Compiler$Target.uncancelable(Compiler.scala:165)
Caused by: javax.net.ssl.SSLHandshakeException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
	at java.base/sun.security.ssl.Alert.createSSLException(Unknown Source)
	at java.base/sun.security.ssl.TransportContext.fatal(Unknown Source)
	at java.base/sun.security.ssl.TransportContext.fatal(Unknown Source)
	at java.base/sun.security.ssl.TransportContext.fatal(Unknown Source)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(Unknown Source)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(Unknown Source)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(Unknown Source)
	at java.base/sun.security.ssl.SSLHandshake.consume(Unknown Source)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(Unknown Source)
	at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(Unknown Source)
	at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(Unknown Source)
	at java.base/java.security.AccessController.doPrivileged(Unknown Source)
	at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask.run(Unknown Source)
	at fs2.io.net.tls.SSLEngineTaskRunner$$anon$1.$anonfun$runDelegatedTasks$1(SSLEngineTaskRunner.scala:44)
	at delay @ fs2.io.net.tls.InputOutputBuffer$$anon$1.$anonfun$perform$2(InputOutputBuffer.scala:90)
	at get @ fs2.concurrent.SignallingRef$$anon$4.getAndDiscreteUpdatesImpl(Signal.scala:292)
	at flatMap @ fs2.io.net.tls.InputOutputBuffer$$anon$1.$anonfun$perform$1(InputOutputBuffer.scala:89)
	at get @ fs2.concurrent.SignallingRef$$anon$4.getAndDiscreteUpdatesImpl(Signal.scala:292)
	at flatMap @ fs2.io.net.tls.InputOutputBuffer$$anon$1.perform(InputOutputBuffer.scala:88)
	at flatTap @ org.http4s.ember.client.EmberConnection$.apply(EmberConnection.scala:86)
	at flatMap @ fs2.io.net.tls.TLSEngine$$anon$1.unwrapHandshake(TLSEngine.scala:252)
	at >>$extension @ fs2.Pull$.fs2$Pull$$go$1(Pull.scala:1242)
	at delay @ fs2.io.net.tls.InputOutputBuffer$$anon$1.$anonfun$perform$2(InputOutputBuffer.scala:90)
	at get @ fs2.concurrent.SignallingRef$$anon$4.getAndDiscreteUpdatesImpl(Signal.scala:292)
	at flatMap @ fs2.io.net.tls.InputOutputBuffer$$anon$1.$anonfun$perform$1(InputOutputBuffer.scala:89)
	at get @ fs2.concurrent.SignallingRef$$anon$4.getAndDiscreteUpdatesImpl(Signal.scala:292)
	at flatMap @ fs2.io.net.tls.InputOutputBuffer$$anon$1.perform(InputOutputBuffer.scala:88)

Some more stuffs for more insight


renku-commit-event-service-7b7b445b59-wp22q:/opt/commit-event-service$ keytool -list -keystore /etc/ssl/certs/java/cacerts | wc -l
Enter keystore password:  

*****************  WARNING WARNING WARNING  *****************
* The integrity of the information stored in your keystore  *
* has NOT been verified!  In order to verify its integrity, *
* you must provide your keystore password.                  *
*****************  WARNING WARNING WARNING  *****************

313
renku-commit-event-service-7b7b445b59-wp22q:/opt/commit-event-service$ keytool -list -cacerts | wc -l
Enter keystore password:  

*****************  WARNING WARNING WARNING  *****************
* The integrity of the information stored in your keystore  *
* has NOT been verified!  In order to verify its integrity, *
* you must provide your keystore password.                  *
*****************  WARNING WARNING WARNING  *****************

287
renku-commit-event-service-7b7b445b59-wp22q:/opt/commit-event-service$ keytool -list -keystore /opt/java/openjdk/lib/security/cacerts | wc -l
Warning: use -cacerts option to access cacerts keystore
Enter keystore password:  

*****************  WARNING WARNING WARNING  *****************
* The integrity of the information stored in your keystore  *
* has NOT been verified!  In order to verify its integrity, *
* you must provide your keystore password.                  *
*****************  WARNING WARNING WARNING  *****************

287

Logs when the pod is starting.


→ k logs -n aml-renku renku-commit-event-service-7b7b445b59-2lqsd                                                                                                                                                                                                                         
Defaulted container "commit-event-service" out of: commit-event-service, init-certificates (init)
2024-07-05 12:44:02,054 INFO  application - No client certificate found
2024-07-05 12:44:12,682 INFO  application - COMMIT_SYNC: Subscribed for events with http://10.42.3.8:9006/events, id = 20240705124401-4626
2024-07-05 12:44:12,682 INFO  application - GLOBAL_COMMIT_SYNC: Subscribed for events with http://10.42.3.8:9006/events, id = 20240705124401-4626
→ k logs -n aml-renku renku-commit-event-service-7b7b445b59-2lqsd -c init-certificates                                                                                                                                                                                                    
Purging old certificates
Updating certificates
WARNING: ca-certificates.crt does not contain exactly one certificate or CRL: skipping
De-reference symlinks

Also, it seems that the problem is general :

renku-event-log-75f8f78fd4-jkmb9:/opt/event-log$ keytool -list -cacerts | wc -l
Enter keystore password:  

*****************  WARNING WARNING WARNING  *****************
* The integrity of the information stored in your keystore  *
* has NOT been verified!  In order to verify its integrity, *
* you must provide your keystore password.                  *
*****************  WARNING WARNING WARNING  *****************

287

@JGodin-C2C
Copy link
Contributor Author

For the moment, i will update and check if i have the same problem.
I close this and will come back if i encounter the same problem.

@JGodin-C2C
Copy link
Contributor Author

Hello !
I am re-opening this pull-request for the same reasons mentionned before.
My renku deployment is now 0.57.1 ( latest AFAIK )
Regards,
J\x00
@aledegano

@Panaetius Panaetius changed the base branch from master to release-0.60.0 October 9, 2024 14:30
@aledegano
Copy link
Contributor

@JGodin-C2C I am looking again at this PR, I understand a little bit better the problem, I am not convinced that this is the best way to solve it and, as I mentioned already above, there are several containers that have a similar configuration to commit-event-service and that, at least in principle, would need the same treatment.

In the meanwhile that I am investigating this issue further: could you tell us which services in your stack are using self-signed certificates?
I see that Gitlab is one and that's why commit-event-service is failing, are there others?

@JGodin-C2C
Copy link
Contributor Author

Sure,
Basically every ingress is using a self-issued certificate (the certificates are distributed internally to our workstations as well).
So, every service that rely on the public ingress ( as opposed to internal kubernetes service ) of our infrastructure will rely on this certificate.

Keycloak is one of them.

@JGodin-C2C
Copy link
Contributor Author

JGodin-C2C commented Oct 11, 2024

Solved in #3804

@Panaetius Panaetius force-pushed the release-0.60.0 branch 3 times, most recently from 7502cda to ee8bbeb Compare October 28, 2024 14:31
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.

5 participants