-
Notifications
You must be signed in to change notification settings - Fork 885
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
CASSJAVA-80: Support configuration to disable DNS reverse-lookups for SAN validation #2018
Conversation
2c6c0f9
to
c8e5b56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Do we need to do the same for ProgrammaticSslEngineFactory and SniSslEngineFactory?
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
c8e5b56
to
2d83d21
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I vaguely remember a similar issue fixed in b2b9bee, so this change makes sense.
Definitely see the value in doing it for the Programmatic one (I see you already covered that, thanks!), for SNI I think there is less utility as you are likely using a DNS name with the IP of the node to target in the SNI, but I think we might as well add it though just in the event that someone is using a literal IP address for the primary endpoint for tunneling through SNI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks great; If you don't mind also adding this for the SNI factory for completeness that'd be good, but i'm +1 either way.
core/src/main/java/com/datastax/oss/driver/internal/core/ssl/SniSslEngineFactory.java
Outdated
Show resolved
Hide resolved
this(sslContext, true); | ||
} | ||
|
||
public SniSslEngineFactory(SSLContext sslContext, boolean allowDnsReverseLookupSan) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't dawn on me at the time that SniSslEngineFactory
does not access DriverContext
so we can't make use of it unless programatically (outside of CloudConfigFactory
), but I suppose if someone wants to use it separately, they can programmatically and in which case this adds some functionality 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 thanks for the fix
… SAN validation patch by Abe Ratnofsky; reviewed by Alexandre Dutra, Andy Tolbert, and Francisco Guerrero for CASSJAVA-80
5f8fe8a
to
b3b9cf1
Compare
No description provided.