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

Jump to bottom

getProtocol() scope, LOCAL INFILE option, SSL fix, and SSL Cert Enhancement #25

Merged
merged 3 commits into from Aug 4, 2015
Merged

getProtocol() scope, LOCAL INFILE option, SSL fix, and SSL Cert Enhancement #25

merged 3 commits into from Aug 4, 2015

Conversation

sehrope
Copy link
Contributor

@sehrope sehrope commented Aug 2, 2015

This PR is split out into four separate commits:

  1. Change MySQLConnection.getProtocol() method scope to "public".
    • This allows using it to issue a query cancellation.
  2. Add a new option allowLocalInfile defaulting to "true"
    • When disabled (i.e. set to false) it throws an error if a ... LOCAL INFILE ... query is executed without first specifying an InputStream for it. This prevents a query from specifying an arbitrary file path via a URL syntax.
    • The default is set to "true" so that the existing behavior is preserved (i.e. no compatibility issue).
  3. Change SSLSocketFactory to set autoClose to true when creating the SSLSocket
    • This fixes SSL connections from hanging when they're closed.
    • See this JDK bug for more info
  4. Enhance custom SSL certificate handling to allow specifying multiple certificates.
    • The existing SSL certificate code (originally written by me :)) only handles loading a single certificate. This patch allows more than one certificate to be specified.
    • A use case for this is having both the old and new versions of the CA cert for your database. That way when the SSL certificate on your server is rotated there won't be any downtime for your app.

Let me know if I should split them out into separate PRs (rather than just separate commits). I figured this would be easier as it avoids creating a bunch of intermediate merge commits.

@sehrope
Copy link
Contributor Author

sehrope commented Aug 2, 2015

FYI about half of the Travis-CI test matrix failed but it's not related to this PR. It's some dependency issue fetching/installing the server to test against:

W: Failed to fetch http://ftp.igh.cnrs.fr/pub/mariadb/repo/10.0/ubuntu/dists/precise/main/binary-i386/Packages 500 Internal Server Error

The tests all pass for the environments that get past the server installation and actually run the tests.

@sehrope
Copy link
Contributor Author

sehrope commented Aug 4, 2015

I rebased the PR atop the Travis fixes (PR #26). Now all the tests pass.

@rusher
Copy link
Collaborator

rusher commented Aug 4, 2015

I don't like the idea of changing the protocol access to public.
1 - This object is internal and changed a lot. If some rely on it , it may break code.
2 - The query cancellation is standard in JDBC using Statement.cancel(), better to keep it standard too.

Good to have add allowLocalInfile option

(originally written by me :))

SSL original writer ! good, keep the good work so !

@sehrope
Add allowLocalInfile connection property defaulting to true.
1a28083 
Adds a new connection property, allowLocalInfile (defaulting to false).
It indicates whether or not the connection should respond to LOCALINFILE
packets when an explicit InputStream for the connection has not been set.
Previously the driver would open whatever local file or URL stream the
server requested it to open. With allowLocalInfile=false the driver will
throw a QueryException if localInfileInputStream is null (ie. it will not
allow reading from the local file system or URL).
@sehrope
Change SSLSocketFactory.createSocket to use autoClose=true
159d791 
This fixes the connection hanging when closing it when using an SSL connection.
JDK bug (closed as "not a defect"): http://bugs.java.com/bugdatabase/view_bug.do?bug_id=6932592
@sehrope
Allow multiple CA certs to be specified
a9be62f 
Allow specifying more than one CA cert file via the serverSslCert option.
Validation of the remote server's SSL certificate will succeed if it can
be validated by any of the specified CA certs.
@sehrope
Copy link
Contributor Author

sehrope commented Aug 4, 2015

I don't like the idea of changing the protocol access to public.

That's fair. I've removed that commit from the PR. I'll figure out a workaround as accessing the Protocol directly is convenient.

SSL original writer ! good, keep the good work so !

Well not the whole SSL stack, just the piece that validates SSL certificates. Thanks though! :)

rusher added a commit that referenced this pull request Aug 4, 2015
@rusher
Merge pull request #25 from sehrope/misc-fixes
8779672 
[CONJ-175] [CONJ-176] getProtocol() scope, LOCAL INFILE option, SSL fix, and SSL Cert Enhancement
@rusher rusher merged commit 8779672 into mariadb-corporation:master Aug 4, 2015
@sehrope
Copy link
Contributor Author

sehrope commented Aug 4, 2015

Awesome. Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@sehrope @rusher