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 to set custom SSLContext #75

Closed
wants to merge 3 commits into from
Closed

Conversation

vvidovic
Copy link

Added code enabling to set custom SSLContext from Java code during
Conjur class initialization.
Usable in k8s/OpenShift environment to setup TLS trust with Conjur
server dynamically from k8s secret and/or configmap data.

Connected to #74.

Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

@vvidovic Thank you for the PR! Just a few comments on the code here.

I would like to see a few things added that are missing from here to get this into the codebase:

  • Its usage in the README and updates to the API
  • Updated CHANGELOG
  • DCO signoff on your commit

PS: Mali svijet 😆

Added code enabling to set custom SSLContext from Java code during
Conjur class initialization. SSLContext enables us to set up a
trust between application and Conjur server from Java code.
Usable in k8s/OpenShift environment to setup TLS trust with Conjur
server dynamicaly from k8s secret and/or configmap data.
README.md file updated with example setup of SSLContext and it's
usage in different Conjur constructors.

Signed-off-by: Vedran Vidovic <vvidovic@email.com>
@vvidovic
Copy link
Author

Thanks for the review, please let me know if I missed something.
I squashed all changes to 1 commit to avoid polluting change history.

The new SSLContext-based usage is more appropriate to have as first item
in usage than JVM-level approach. Wording and nesting of headers was
improved for readability too.
Copy link
Contributor

@sgnn7 sgnn7 left a comment

Choose a reason for hiding this comment

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

Hey @vvidovic,
It looks good to me just needs a bit of wording change for which I've opened a pull request against your repo. If you merge that, I think this branch might be ready for inclusion (I may need to confirm this though).

Move SSLContext README section and improve wording
@codeclimate
Copy link

codeclimate bot commented May 27, 2020

Code Climate has analyzed commit 9dc5f34 and detected 17 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 16
Clarity 1

View more on Code Climate.

@sgnn7
Copy link
Contributor

sgnn7 commented May 27, 2020

/approve-build 9dc5f34

1 similar comment
@JakeQuilty
Copy link
Contributor

/approve-build 9dc5f34

@sgnn7
Copy link
Contributor

sgnn7 commented May 28, 2020

Moved PR to a branch in parent repo and new PR has been opened for the code so that we can build it on internal infra: #76. Closing this PR.

PS: I fixed up the comment from readme and removed the merge commit from my readme update in it.

@sgnn7 sgnn7 closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants