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.

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

MySQL SSL parameters in URI are ignored #374

Closed
dmitry-naumovich opened this issue Jun 19, 2017 · 19 comments
Closed

MySQL SSL parameters in URI are ignored #374

dmitry-naumovich opened this issue Jun 19, 2017 · 19 comments

Comments

@dmitry-naumovich
Copy link

dmitry-naumovich commented Jun 19, 2017

Hi all!
After executing my own sql script MySQL TestContainer outputs the next warning:

Mon Jun 19 15:41:52 MSK 2017 WARN: Establishing SSL connection without server's identity verification is not recommended. According to MySQL 5.5.45+, 5.6.26+ and 5.7.6+ requirements SSL connection must be established by default if explicit option isn't set. For compliance with existing applications not using SSL the verifyServerCertificate property is set to 'false'. You need either to explicitly disable SSL by setting useSSL=false, or set useSSL=true and provide truststore for server certificate verification.

It's a well known issue so I've tried to use the next URIs:
jdbc:tc:mysql://hostname/databasename?TC_INITSCRIPT=initial_sql.sql&useSSL=false

jdbc:tc:mysql://hostname/databasename?TC_INITSCRIPT=initial_sql.sql&autoReconnect=true&useSSL=false

jdbc:tc:mysql://hostname/databasename?TC_INITSCRIPT=initial_sql.sql&verifyServerCertificate=false&useSSL=true

But neither of them worked and the warning still occurs.
I'm wondering, as soon as hostname and database name are ignored, maybe all these ssl settings are ignored too? And is there a way to define them in another way?

Would appreciate any help, thanks.

@kiview
Copy link
Member

kiview commented Jun 23, 2017

I didn't get the warning when running our JDBCDriverTest with jdbc:tc:mysql://hostname/databasename?TC_INITSCRIPT=somepath/init_mysql.sql&verifyServerCertificate=false&useSSL=true as connection string. Which JDBC driver and pool are you using?

@dmitry-naumovich
Copy link
Author

The project is based on Spring Boot, so everything is autoconfigured. I use mysql-connector-java v 5.1.4 as a JDBC driver (which is managed with spring-boot-dependencies v1.4.4)

@kiview
Copy link
Member

kiview commented Jun 25, 2017

Can you share an example project? Might have something to do with the way Spring-Boot parses the JDBC-Url, it might be confused by the TC parameter, not sure though.

Maybe #345 plays into this issue as well.

@npetzall
Copy link

npetzall commented Nov 1, 2017

It might be related to how waitUntilContainerStarted() is implemented in JdbcDatabaseContainer.

It uses a createConnection("") and as such it has no queryString.

But then again that warning should come in the beginning. Right after container startup and not after execution of any scripts.

@philbrock
Copy link

I'm seeing the same thing, but as npetzall says - it's down to the implementation of waitUntilContainerStarted(). If you set up some tests with a rule like this:

    @Rule
    public MySQLContainer mysql = new MySQLContainer();

Then you'll see it logged before every test.

E.g. this test that doesn't even use the container will log the warning:

public class MySQLContainerTest {

  @Rule
  public MySQLContainer mysql = new MySQLContainer();

  @Test
  public void demonstrateMySQLRuleLogging() {
    assertTrue(true);
  }
}

@npetzall
Copy link

A really dirty way of removing the error is using an older mysql-connector-java. Since it's a message from the driver.

@iNikem
Copy link
Contributor

iNikem commented Jan 25, 2018

I think testcontainer should add useSSL=false automatically. What'a the point of pretending and trying to use SSL when you communicate with test db on your local machine?

@npetzall
Copy link

Am I right in that now any subclass of Jdbc Container get useSSL=false which might be a vendor specific argument? So I need to validate that all my jdbc drivers won't freakout if useSSL=false is supplied as a query string? What about withDefaultQueryString and an overloaded getConnection() that uses default? Then in mysql just set it in the constructor an if you want to remove just supply empty string/null to withDefaultQueryString.

@iNikem
Copy link
Contributor

iNikem commented Jan 28, 2018

I thought about this. But as all current tests passed I assumed that at least currently supported containers work just fine with it. If some DB fails on this, I will be happy to investigate more elaborate solution.

@npetzall
Copy link

I understand your point, but due to change in superclass it literally affects all jdbc drivers in the universe that can be used with testcontainers. Writing a new or custom(informix, hive, impala)

Everything is not lost, it's a simple override.

@npetzall
Copy link

I just have vague recollection of oracle jdbc drivers not keen on unsupported query strings.

@kiview
Copy link
Member

kiview commented Jan 30, 2018

And it seems @npetzall was right about this, which lead to #568.
This is mainly a problem with the way the modules are organized, build and tested of course, for which we will find a solution soon.

@npetzall
Copy link

It would probably be good to either mark JdbcContainer as internal or public api. Since with that information you either need to take consumer/user subclasses into account or just your own.

If it would be marked as internal and my informix subclass would fail, well that's all on me for using an internal api (you only need to verify your usage and not take anyone else's into account), if it was public and it changed how it talks to all jdbc driver I would consider it a possible breaking change, major version bump and all that.

Hypothetical it could have worked with oracle and SQL server but not with informix, hive, impala, firebird, db2, netezza.....

@iNikem
Copy link
Contributor

iNikem commented Jan 31, 2018

Well, it seems I have to solve it properly :) I will propose a new PR in the next week.

@npetzall
Copy link

npetzall commented Jan 31, 2018

As a side-note the separator & is not globally defined, for instance some uses ';' or ':' might be the real issue with Oracle and SQL Server and not actually the useSSL part or the ? since I think neither is part of the jdbc spec, which truly doesn't even matter since there I a plethora of non-compliant jdbc drivers.

Add
protected String defaultQueryString = "";
to JdbcDatabaseContainer
Add

public SELF withDefaultQueryString(String queryString) {
  defaultQueryString = queryString;
  return self();
}

to JdbcDatabaseContainer

Change JdbcDatabaseContainer.createConnection(String queryString)
from
final String url = appendDisableSslConfig(this.getJdbcUrl() + queryString);
to
final String url = this.getJdbcUrl() + Optional.ofNullable(queryString).orElse(defaulQueryString);
in JdbcDatabaseContainer

Change JdbcDatabaseContainer.waitUntilContainerStarted()
from
try (Connection connection = DB_CONNECT_RATE_LIMIT.getWhenReady(() -> createConnection(""))) {
to
try (Connection connection = DB_CONNECT_RATE_LIMIT.getWhenReady(() -> createConnection(null))) {

Add
withDefaultQueryString("?useSSL=false")
to constructor of MySQLContainer

This is after revert of #561 and it would allow others to customize the check connection which might be desirable in user subclasses.

@iNikem
Copy link
Contributor

iNikem commented Feb 4, 2018

@rnorth I think the suggestions of @npetzall above make total sense. If they are implemented, then this issue #374 will be solved, #568 will not occur. Then #345 can be solved just by stripping all parameters from tc: jdbc connection string. Because all non-TC parameters can be provided via new method withDefaultQueryString. But that will be breaking change :(

But with your approval I can make them.

@npetzall
Copy link

npetzall commented Feb 4, 2018

When using TC jdbc URL you don't have access to the container so you won't be able to set the parameters.
So the breaking change can not be done.
Just adding the withDefaultQueryString is minor change added functionality.

@iNikem
Copy link
Contributor

iNikem commented Feb 4, 2018

Hm, but then what exactly withDefaultQueryString will add to the current state? After PR #569 I mean. useSSL is fixed, what is the other use-case for default query string on the container level, as opposed to the application configuration?

@npetzall
Copy link

npetzall commented Feb 4, 2018

Proposal was written moments before the PR. The proposal allows for setting the waitForContainerStart queryString without subclassing and without any duplicate check.

The not subclassing is nice, also I could declare the queryString only once in default and call connect with null. Only interesting if multiple connections are required and the jdbc implementation allows multiple connections too single thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants