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

Enable using PKCS#12 based Java keystores #336

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Conversation

modulo11
Copy link
Contributor

Summary

Use Cases

  • Re-enables adding ca-certificates to the Java keystore for Java 18 and above

Checklist

  • 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.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@modulo11 modulo11 requested a review from a team as a code owner October 31, 2023 13:08
@dmikusa dmikusa added semver:minor A change requiring a minor version bump type:enhancement A general enhancement labels Nov 4, 2023
Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

Code looks great! Thanks for putting this together. Just one question for you.

In the meantime, I'm going to give this a try with a few buildpacks and see how it runs.

keystore.go Show resolved Hide resolved
keystore.go Outdated Show resolved Hide resolved
@dmikusa
Copy link
Contributor

dmikusa commented Nov 4, 2023

Tested with Bellsoft buildpack:

  • current version with Java 21 displays warning and does not load certs at runtime
  • new version of libjvm same buildpack version with Java 21, loads certs at build time and runtime (no warning)
  • new version of libjvm same buildpack version with Java 17, works as it did before
  • looks correct

Tested with Adoptium, same behavior. Working correctly.

Tested with Azul Zulu, same behavior. Working correctly.

I'm happy with the testing.

@modulo11 did you test with the sap-machine buildpack?
@anthonydahanne or @pivotal-david-osullivan do you want to test this as well? or good to merge?

@modulo11
Copy link
Contributor Author

modulo11 commented Nov 7, 2023

@modulo11 did you test with the sap-machine buildpack? @anthonydahanne or @pivotal-david-osullivan do you want to test this as well? or good to merge?

Yes, tested locally with SapMachine and seems to work nicely.

Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
@dmikusa dmikusa self-requested a review November 7, 2023 13:57
@dmikusa dmikusa merged commit 493c263 into main Nov 7, 2023
3 checks passed
@dmikusa dmikusa deleted the pkcs12-cert-loader branch November 7, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants