wix-incubator / wix-embedded-mysql

embedded mysql based on https://github.com/flapdoodle-oss/de.flapdoodle.embed.process
BSD 3-Clause "New" or "Revised" License
382 stars 81 forks source link

embedded mysql should listen by default on localhost #122

Open jefimm opened 6 years ago

jefimm commented 6 years ago

The mysql opens ipv6 socket to the world, which is not secure and causes unnecessary warnings on MacOS.

from lsof output IPv6 0x47e09a7bb638616f 0t0 TCP *:dyna-access (LISTEN)

Embedded mysql should listen on localhost only by default

viliusl commented 6 years ago

@jefimm yes, mysql does listen on * instead of explicit localhost mostly due to some issues I had/saw others having when Java on OSX in some occasions for localhost lookup returns you not 127.0.0.1, but instead your external interface. What happens is then mysqld is listening on 127.0.0.1 but java connection is being established on external IP and it fails like https://github.com/jruby/activerecord-jdbc-adapter/issues/264

Now in wix-embedded-mysql I would rather have a less secure set-up, but with a higher chance of working than vice-versa. It's not a daemon that runs continuously, but just a thing with a short lifecycle. You can do a PR and add an option to force where mysqld should bind to.

jefimm commented 6 years ago

If MysqldConfig would return the hostname to connect to (set to 127.0.0.1) it would be completely avoided

viliusl commented 6 years ago

@jefimm what I could do is to add option to MysqldConfig like:

MysqldConfig config = aMysqldConfig(v5_6_23)
  .withListenOn('*' || 'loclahost' || '127.0.0.1') // withHost(''), withBindTo('') - need to figure out the correct name
  .build();

Not sure if I would change default - current default '*' is most bulletproof, but whoever is worried about it could change it. Wdyt?

jefimm commented 6 years ago

This would be a step in right direction I would use .withListenOn('127.0.0.1') and use the same one for the jdbc url

bobtiernay-okta commented 6 years ago

Totally agree here. This is super annoying on Mac due to:

image

I don't want to disable my firewall, and it appears that each process is unique so it comes up all the time.

bobtiernay-okta commented 6 years ago

Is there a work around in the meantime?

bobtiernay-okta commented 6 years ago

@viliusl I don't mind having to set this via reflection, etc. if there is a workaround.

Your guidance is much appreciated. And thanks for such a great project!

viliusl commented 6 years ago

@bobtiernay-okta - I there was a suggestion for an extension of api to make it opt-in for now. PR's are welcome and it should be fairly simple addition. If it works as expected we could toggle it on by default in upcoming versions.

bobtiernay-okta commented 6 years ago

@viliusl I would be willing to take a look if you could point me to the right part of the code. Iirc, this is in my.conf. Not sure how to affect that from the code.

Thanks!

viliusl commented 6 years ago

@bobtiernay-okta so the way I see it there are several things that would need to change given

MysqldConfig config = aMysqldConfig(v5_6_23)
  .withListenOn('*' || 'loclahost' || '127.0.0.1')
  .build();

would be provided:

together with a test. test could be just reading server variable as in https://github.com/wix/wix-embedded-mysql/blob/master/wix-embedded-mysql/src/test/scala/com/wix/mysql/EmbeddedMysqlTest.scala#L30

hopefully I did not miss anything:)

bobtiernay-okta commented 6 years ago

Just a heads up, I was able to get this working without any changes:

    private EmbeddedMysql createMysqld() {
        MysqldConfig config = aMysqldConfig(v5_5_52)
                .withServerVariable("bind-address", "localhost")
                .build();

        return anEmbeddedMysql(config)
                .start();
    }

So perhaps we good already and just need to update the docs?

viliusl commented 6 years ago

@bobtiernay-okta :) once withServerVariable was added, you can actually tune most of stuff without adding explicit builder additions. Now I would rather add it to builder as there were at least 2 cases were users were bitten by current behavior. Adding to builder and maybe to defaults. You can leave it be of course and I will leave ticket open:)

ddcprg commented 5 years ago

How did you manage to get around this?

https://github.com/wix/wix-embedded-mysql/blob/master/wix-embedded-mysql/src/main/java/com/wix/mysql/MysqlClient.java#L59

The client still uses localhost, I get the following exception when I try to start the embedded instance:

com.wix.mysql.exceptions.CommandFailedException: Command 'CREATE USER 'someuser'@'%' IDENTIFIED BY '';' on schema 'information_schema' failed with errCode '1' and output 'mysql: [Warning] Using a password on the command line interface can be insecure.
ERROR 2003 (HY000): Can't connect to MySQL server on 'localhost' (61)
'
        at com.wix.mysql.MysqlClient.execute(MysqlClient.java:74)
        at com.wix.mysql.MysqlClient.executeCommands(MysqlClient.java:49)
        at com.wix.mysql.EmbeddedMysql.<init>(EmbeddedMysql.java:48)
        at com.wix.mysql.EmbeddedMysql$Builder.start(EmbeddedMysql.java:169)

I can push a fix, should be a matter of fetching the bind address from the config. First add getters to com.wix.mysql.config.MysqldConfig.ServerVariable and then:

...
private void execute(String sql) {
...
  Process p = new ProcessBuilder(new String[]{
                    Paths.get(executable.getBaseDir().getAbsolutePath(), "bin", "mysql").toString(),
                    "--protocol=tcp",
                    format("--host=%s", getBindAddress()),
                    "--password=",
                    format("--default-character-set=%s", effectiveCharset.getCharset()),
                    format("--user=%s", SystemDefaults.USERNAME),
                    format("--port=%s", config.getPort()),
                    schemaName}).start();
....
}

private String getBindAddress() {
  for(ServerVariable sv : config.getServerVariables()) {
    if ("bind-address".equals(sv.getName()) && sv.getValue() != null) {
      return sv.getValue().toString();
    }
  }
  return "localhost"; 
}
...

I believe this only happens if the config contains a user

atcollins06 commented 5 years ago

We used the workaround suggested by bobtiernay-okta above i.e. adding .withServerVariable("bind-address", "localhost") to the configuration and this worked as advertised to prevent the popups from happening on the Mac.

A side-effect seems to have cropped up though. Now, when the shutdown goes to terminate any mysqld instances, the spawned mysqladmin processes sometimes hang. It is intermittent and inconsistent how many processes hang. But, since we are running as part of our JUnit tests, this causes the tests to hang too.

We ultimately ended up writing our own shutdown hook to manually terminate all the leftover mysqld and mysqladmin processes. But, we'd like to avoid this if possible. Has anyone else seen this behavior and, if so, have you come up with a solution or better workaround? Interestingly, the code in stopUsingMysqldadmin in MysqldProcess has a comment saying that the code should include a timeout to wait on the mysqladmin process to return. Unfortunately, that has not been implemented yet so the mysqladmin process just hangs indefinitely.