-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
DBZ-1228: MySQL connection with client authentication does not work #867
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.
@Arkoprabho Thanks a lot for the contribution! I've added two comments related to the simlplification of the code. Other than that I'think it is done.
SSLSocketFactory sslSocketFactory = null; | ||
try { | ||
sslSocketFactory = getBinlogSslSocketFactory(connectionContext); | ||
} catch (KeyStoreException e) { |
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 think you can use multiple catch statement and handle it in one branch.
Also I recommend to use a single logging statement like logger.error("Encountered an exception while initiaiting SSL context, e);
Also I think the exception should not be handled but retrown like throw new ConnectException(e)
.
@@ -76,6 +83,7 @@ | |||
|
|||
private static final long INITIAL_POLL_PERIOD_IN_MILLIS = TimeUnit.SECONDS.toMillis(5); | |||
private static final long MAX_POLL_PERIOD_IN_MILLIS = TimeUnit.HOURS.toMillis(1); | |||
private static KeyManager[] KMS = null; |
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.
Is it necessary to have the variable as a static attribute? Cannot it be converted to a local variable in the method that initializes it? If yes please also rename it from uppercase.
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 we can, but that would entail us creating a new variable, since it is being used here
Without putting it as static we get the following error: Variable 'kms' is accessed from within inner class, needs to be final or effectively final
I will do the same and push.
The SSL properties were being reset, and thus the authentication was failing on the second time. When debezium connects to the database the first time in order to test the connections, the required parameters are all set. And thus the test passes. After that the shutdown method is called that clears these properties, and the consecutive connection fails. DBZ-1228: MySQL connection with client authentication does not work Create and pass a socket factory that provides they keystore config DBZ-1228: MySQL connection with client authentication does not work Prevent forcing of SSL. Use SSL only if the required properties are set. DBZ-1228: MySQL connection with client authentication does not work Added better exception messages
Used multiple catch statements and handle in 1 branch. Not using static modifier for keymanager.
@Naros I've simplied the error handling a bit, could you please take a look and merge if you are happy with it? |
Used multiple catch statements and handle in 1 branch. Not using static modifier for keymanager.
Merged, thanks @Arkoprabho and @jpechane |
Summary
A connection to SQL server with client-side SSL authentication (like Google Cloud SQL) cannot be established.
Issue analysis
The issue is caused because of the shutdown method being called here. This is why the connection test succeeds, but fails later. The next time when debezium attempts to connect to the database here, these variables are not set and the connection fails.
Solution
We can set the system variables before the call is made. Since the start method is coded in a way that it sets the values only if the configuration values are provided, we can simply reuse this. This solves the current issue but introduces another one.
On forcing debezium to have the system variables, we get an authentication error. In order to fix this, we need to create and pass a socket factory that provides the keystore config. The changes are made in a way such that any non-SSL connections are allowed when the required configurations are not provided, and the required values are only set if the keystore password is provided.