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.

Sign up for GitHub

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

Jump to bottom

Add SslOptions #3980

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Add SslOptions #3980

wants to merge 50 commits into from

Conversation

sazzad16
Copy link
Collaborator

Similar to SslOptions in Lettuce library.

atakavci
atakavci reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@atakavci atakavci left a comment

Choose a reason for hiding this comment

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

having a trust manager like this smells off. May be its because i am lack of context. What is the rationale ?

src/main/java/redis/clients/jedis/DefaultJedisClientConfig.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/DefaultJedisClientConfig.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/DefaultJedisClientConfig.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslOptions.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/DefaultJedisSocketFactory.java Show resolved Hide resolved
@tishun
Copy link

tishun commented Oct 1, 2024

having a trust manager like this smells off. May be its because i am lack of context. What is the rationale ?

I have the same question. The motivation of the change is important in this case.

@uglide
Copy link
Contributor

uglide commented Oct 2, 2024

@tishun @atakavci This is an attempt to create a TLS config similar to what we have in Lettuce, as a more developer-friendly option compared to the current approach with SSLSocketFactory, etc.

@sazzad16
Copy link
Collaborator Author

sazzad16 commented Oct 2, 2024

having a trust manager like this smells off. May be its because i am lack of context. What is the rationale ?

I have the same question. The motivation of the change is important in this case.

@tishun

  1. Lettuce has similar support.
  2. Users want insecurity.

tishun
tishun reviewed Oct 9, 2024
View reviewed changes
Copy link

@tishun tishun left a comment

Choose a reason for hiding this comment

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

I may not be getting the whole idea, so please excuse me if my comments are not making sense.

My current assumption is that we only want to be able to enable and disable hostname verification, as part of the SSL verification process.

src/main/java/redis/clients/jedis/DefaultJedisClientConfig.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslHostnameVerifyMode.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslOptions.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslOptions.java Outdated Show resolved Hide resolved
@sazzad16 sazzad16 mentioned this pull request Nov 28, 2024
Closed
@sazzad16 sazzad16 mentioned this pull request Nov 28, 2024
Closed
@sazzad16 sazzad16 marked this pull request as ready for review December 17, 2024 17:49
@sazzad16 sazzad16 requested review from a team, ggivo, atakavci, tishun and uglide December 17, 2024 18:47
tishun
tishun previously approved these changes Dec 18, 2024
View reviewed changes
Copy link

@tishun tishun left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

src/main/java/redis/clients/jedis/SslOptions.java Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslOptions.java Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslOptions.java Outdated Show resolved Hide resolved
src/main/java/redis/clients/jedis/SslVerifyMode.java Show resolved Hide resolved
@sazzad16 sazzad16 requested a review from tishun December 18, 2024 16:27
tishun
tishun previously approved these changes Dec 19, 2024
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tishun tishun tishun left review comments

@ggivo ggivo Awaiting requested review from ggivo

@atakavci atakavci Awaiting requested review from atakavci

@uglide uglide Awaiting requested review from uglide

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement experimental ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sazzad16 @tishun @uglide @atakavci @ggivo