-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add support for sslmode=verify-full for mysql and pg #1249
Conversation
aa9d057
to
e3bb413
Compare
7748772
to
b3bb901
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.
The central idea has already been implemented in our MsSQL connector, so I think this looks great.
Just one nit, and some codeclimate things that might need to be resolved. Otherwise, LGTM.
4c3c00a
to
efa1569
Compare
a7f88e5
to
c0da649
Compare
904bb0e
to
939603a
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
86642d9
to
0e9a511
Compare
This means full verification of rootCA and hostname. The value for the hostname to check against is either 'sslhost' or 'host', 'sslhost' takes precedence when it is not empty.
0e9a511
to
1180aff
Compare
Code Climate has analyzed commit 1180aff and detected 5 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 86.9% (50% is the threshold). This pull request will bring the total coverage in the repository to 49.6% (1.2% change). View more on Code Climate. |
@doodlesbykumbi this is good to go! sorry for the delay |
This means full verification of rootCA and hostname. The value for the hostname to check against is either 'sslhost' or 'host', 'sslhost' takes precedence when it is not empty.
What does this PR do?
What's changed? Why were these changes made?
sslhost
option that can override thehost
for certificate hostname verificationHow should the reviewer approach this PR, especially if manual tests are required?
The entrypoint to the feature is
internal/plugin/connectors/tcp/ssl/ssl.go
. This file contains the TLS implementation for both mysql and pg.Are there relevant screenshots you can add to the PR description?
N/A
What ticket does this PR close?
Connected to #548
Checklists
Change log
Test coverage
Documentation
README
s) were updated in this PR, and/or there is a follow-on issue to update docs. https://github.com/cyberark/secretless-docs/issues/255