Skip to content

Added SSL support #78

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

Closed
wants to merge 3 commits into from
Closed

Added SSL support #78

wants to merge 3 commits into from

Conversation

Mart-Bogdan
Copy link
Contributor

Added SSL support, see #43
Now can be used with Heroku, as heroku supports only SSL.

Now can be used with Heroku, as heroku supports only SSL
@vietj vietj added the to review label May 9, 2017
@vietj
Copy link
Contributor

vietj commented May 9, 2017

@Mart-Bogdan have you signed the Eclipse CA ?

@Mart-Bogdan
Copy link
Contributor Author

@vietj I belive yes, I've made some PRs to one of your repos(if I remember correctly) . But that was quite a while, I do not remember exactly whole process.

@Mart-Bogdan
Copy link
Contributor Author

I've checked, yes I'm signed it. Do I need to sign off commits in this repo?
image

@Mart-Bogdan
Copy link
Contributor Author

Hello, @vietj so this can be merged?

@Narigo
Copy link
Contributor

Narigo commented May 11, 2017 via email

@Mart-Bogdan
Copy link
Contributor Author

@Narigo thanks

Copy link
Contributor

@Narigo Narigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add a test for this change?

* "queryTimeout" : <timeout-in-milliseconds>
* "queryTimeout" : <timeout-in-milliseconds>,
* "sslmode" : <"disable"|"prefer"|"require"|"verify-ca"|"verify-full">,
* "sslrootcert" : <path to file with certificate>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sslMode and sslRootCert would be more appropriate names (maxPoolSize and queryTimeout are camel cased as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I won't change "verify-ca" and "verify-full" as it's direct values passed to underlying library.

@Mart-Bogdan
Copy link
Contributor Author

Would it be possible to add a test for this change?
I'm quite not sure how to test it. AFAIK postgresql by default allows both SSL and non SSL connections in default configuration.

Honestly, I didn't figured out how to run your tests. Can it be run without Docker?
But anyway how can be tested SSL, perhaps try to connect heroku database? and check if it work (they require SSL)

@Mart-Bogdan
Copy link
Contributor Author

@Narigo database test connection is configured at this method io.vertx.ext.asyncsql.PostgreSQLClientTest#init?

But i'm still not sure how to test this functionality.

@Mart-Bogdan
Copy link
Contributor Author

Hello, @Narigo, what do you think?
I have no idea how to check connectivity to ssl, but I would like to see this merged, so I won't have to use custom build from fork in local repository.

@Narigo
Copy link
Contributor

Narigo commented May 22, 2017

Hi @Mart-Bogdan and sorry for being so late with replies here. I try to check the code myself this week and get back to you on the weekend. If @pmlopes has time to have a look, especially regarding testing the SSL stuff, I guess we could use helpful input ;)

@Narigo
Copy link
Contributor

Narigo commented May 28, 2017

I've tried to get this running but had trouble getting the environment to work. Maybe some docker expert could help us here? @cescoffier @vietj @pmlopes ?

Two test cases like this should suffice, but I'm unsure how to get it into a test environment / set up that environment using docker:

  @Test
  public void testCorrectSslConfiguration(TestContext context) {
    Async async = context.async();
    String path = getClass()
      .getResource("/ssl-docker/server.crt")
      .getPath();

    System.out.println("Path = " + path);

    JsonObject sslConfig = new JsonObject()
      .put("sslMode", "require")
      .put("sslRootCert", path);

    client = createClient(vertx, sslConfig);

    client.getConnection(sqlConnectionAsyncResult -> {
      sqlConnectionAsyncResult.cause().printStackTrace();
      context.assertTrue(sqlConnectionAsyncResult.succeeded());
      conn = sqlConnectionAsyncResult.result();
      conn.query("SELECT 1", ar -> {
        if (ar.failed()) {
          context.fail("Should not fail on ssl connection");
        } else {
          async.complete();
        }
      });
    });
  }

  @Test
  public void testWrongSslConfiguration(TestContext context) {
    Async async = context.async();
    client = createClient(vertx,
      new JsonObject()
        .put("host", System.getProperty("db.host", "localhost"))
        .put("sslMode", "verify-ca")
        .put("sslRootCert", "something-wrong.crt")
    );

    client.getConnection(sqlConnectionAsyncResult -> {
      context.assertTrue(sqlConnectionAsyncResult.failed());
      async.complete();
    });
  }

I've created a folder /src/test/resources/ssl-docker with a server.crt and server.key file (which I borrowed from the tests of the underlying driver's SSL implementation). Then I added a file called init.sh with contents found here. Running the PostgreSQL docker through this command: docker run -it --rm -v $(pwd)/src/test/resources/ssl-docker/:/docker-entrypoint-initdb.d/ --name vertx-postgres -e POSTGRES_USER=vertx -e POSTGRES_PASSWORD=password -e POSTGRES_DB=testdb -p 5432:5432 postgres -l and executing mvn test yielded a timeout on the testCorrectSslConfiguration. The terminal running the docker command additionally logs LOG: could not accept SSL connection: EOF detected while the test is running (waiting for the timeout) and LOG: could not receive data from client: Connection reset by peer when the timeout occurred.

I'm pretty sure I'm doing something wrong here, so help is highly appreciated... :)
I guess we need to change the way we run the docker image (otherwise the tests will fail because no SSL is activated) and add this to Travis as well...

@Mart-Bogdan
Copy link
Contributor Author

Hi there, I've managet to get Docler runing on my machine, and configured SSL image of postgreSQL.
I'm planing to add test to this PR this week.

@vietj Would it be ok, if I change port numbers used for my-sql and postgre-sql? Currently default ones are being used, but that won't work if databases are installed on machine.

I want to chage ports for both DBs in tests, and add second instance of postgre, with SSL.

@Narigo
Copy link
Contributor

Narigo commented Sep 3, 2017

@Mart-Bogdan I finally came around fixing this. I've added tests and how to run the ssl docker stuff. I also rebased everything on the latest master, so I'll create a new PR for you to review the tests.

@Narigo
Copy link
Contributor

Narigo commented Sep 3, 2017

I'll close this in favor of the rebased version with tests over here: #88

@Narigo Narigo closed this Sep 3, 2017
@Narigo Narigo removed the to review label Sep 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants