Load existing certs from pkcs12 store

paketo-buildpacks / libjvm

A library and helper applications that form the basis for building the different Paketo-style JVM-providing buildpacks

Apache License 2.0

19 stars 20 forks source link

Load existing certs from pkcs12 store #344

Closed c0d1ngm0nk3y closed 6 months ago

c0d1ngm0nk3y commented 8 months ago

Summary

When writing a pkcs12 key store, we read first the existing certificates like done in the jks case. The implementation did remove any existing certs before.

Downport of https://github.com/paketo-buildpacks/libjvm/pull/346

Use Cases

The keystore provided by the JRE already contains some certificates.

Checklist

[X] I have viewed, signed, and submitted the Contributor License Agreement.
[ ] I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
[ ] I have added an integration test, if necessary.
[X] I have reviewed the styleguide for guidance on my code quality.
[X] I'm happy with the commit history on this PR (I have rebased/squashed as needed).

dmikusa commented 8 months ago

I just want to make sure I understand the issue. Are you saying that after we made the PKCS12 certificates writable, i.e. #339, we were not merging in the system certs and the JVM certs?

Off the top of my head, I believe that we were previously doing something like, read system certs, read JVM certs, pruning duplicates, and then writing them all out to the JVM cert location. It sounds like for PKCS12 certs, we're not presently reading the JVM certs and including those with the system certs. Is that correct?

Thanks

modulo11 commented 7 months ago

I just want to make sure I understand the issue. Are you saying that after we made the PKCS12 certificates writable, i.e. #339, we were not merging in the system certs and the JVM certs?

Off the top of my head, I believe that we were previously doing something like, read system certs, read JVM certs, pruning duplicates, and then writing them all out to the JVM cert location. It sounds like for PKCS12 certs, we're not presently reading the JVM certs and including those with the system certs. Is that correct?

Thanks

Yes, that's correct. Currently, the JVM provided certificates are ignored in case of PKCS#12 keystore, as it's always initialized with an empty list.

The behavior for JKS looks fine as it loads the certifictes. Seems to have slipped through 😞

dmikusa commented 6 months ago

I think this is almost ready.

New comment about the test above. That's totally on us, we have a bad pattern there, but if you could switch it. I think it should be easy to switch.
You need to update to the latest branch.

Mention me on this when you update, so I see it, and I'll merge. Thanks for this!

dmikusa commented 6 months ago

Looks like we might have an issue with the permission detection. We have a report that the cert loader isn't working in read-only systems. https://github.com/paketo-buildpacks/libjvm/issues/355

Not sure if this PR addresses this problem as well, or if not, we can fix the issue in this PR. Thoughts?

phil9909 commented 6 months ago

Looks like we might have an issue with the permission detection. We have a report that the cert loader isn't working in read-only systems. #355

Not sure if this PR addresses this problem as well, or if not, we can fix the issue in this PR. Thoughts?

Well, it is addressed in a sense. The step (of adding the OS certificates to the Java Truststore) will be skipped. If this is not needed, the problem is indeed gone. Otherwise, the user will have to provide a writable tmp dir and the helper will then create a temporary truststore on startup.

Mention me on this when you update, so I see it, and I'll merge. Thanks for this!

Should be good now.