-
Notifications
You must be signed in to change notification settings - Fork 91
Wdt 609 ssl db connection #1109
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
Conversation
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.
There are some conflicts with sonar changes, but 👍 once that passes
Will there be any documentation update? |
if (keyStorePassword != null && keyStorePassword != "None") { | ||
sslArgs.append(",javax.net.ssl.keyStorePassword="+ keyStorePassword); | ||
} | ||
sslArgs.append(",oracle.net.ssl_server_dn_match=false"); |
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.
do we need to make ssl_server_dn_match configurable ?
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.
It was in a different branch and was already checked into the documentation main.
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.
I'm not sure we do, from what I have seen everywhere, it is false.
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.
Yes, I know in ATP we hard coded to false and probably we need to make it configurable - just in case user complains against it (most internal users have not much experience anyway, I don't know what it is until I read the doc) - we can make it default to false? I think this is an extra level of security to make sure talking to the right db server.
From the documentation,
https://www.oracle.com/technetwork/database/enterprise-edition/wp-oracle-jdbc-thin-ssl-130128.pdf
The following property also needs to be used to force the JDBC Thin driver to verify the server’s DN:
props.setProperty("oracle.net.ssl_server_dn_match", "true");
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.
documentation was in another branch and it got merged already into documentation.
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.
from what I read online it should always be false. I might be wrong.
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.
Most example doesn't want to have the hassle for user to handle error. The property exists for a reason, even though user may usually set it to false, why not provide a option for user?
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.
Note: I am saying setting it to true, I am just asking to make it a configurable property
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.
sorry, I replied to your first comment twice. changing to a property
I reviewed the security hotspots and don't have any concerns. Derek, you should review these too please. @CarolynRountree, can you please clean up the code smells? I think that those do look valid. |
Kudos, SonarCloud Quality Gate passed! |
RCU SSL DB in WDT createDomain. I used the atp as a template and the code follows the atp code.