-
Notifications
You must be signed in to change notification settings - Fork 426
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
Add connection properties for specifying custom TrustManager #74
Conversation
ecc461d
to
eafeb29
Compare
138105b
to
b206b0f
Compare
b206b0f
to
d2c6476
Compare
Codecov Report
@@ Coverage Diff @@
## dev #74 +/- ##
============================================
+ Coverage 46.26% 46.29% +0.02%
- Complexity 2201 2204 +3
============================================
Files 108 108
Lines 25227 25264 +37
Branches 4172 4178 +6
============================================
+ Hits 11672 11695 +23
- Misses 11538 11541 +3
- Partials 2017 2028 +11
Continue to review full report at Codecov.
|
I've rebased this atop the formatted dev branch so it applies cleanly. Any idea when / if this is going to get merged? It's been in the "Under review" tag for two months. |
@sehrope - we are currently considering the PR and trying to understand if there are potential security implications of providing support for third party trust managers. |
d2c6476
to
aee7eaf
Compare
aee7eaf
to
5f4100e
Compare
Why was this closed? I'd still like to get something that provides this type of extensibility added to the driver At the moment it's not possible as you'd have to muck with the local JVM wide keystore. |
Sorry @sehrope , I missed your last comment. We removed our previous Dev branch and all PRs created for Dev were closed automatically. We have re-opened the PR. I am sorry for the trouble. We are looking at this PR, we will get back to you soon. Thank you. |
Hi @sehrope, Thank you for the contribution. We started the review process, stay tuned :) |
Great. It's been a while since this was put together. I think it still applies cleanly but if not let me know and I'll rebase it. |
// if so instantiate it, optionally specifying a constructor argument to customize it. | ||
else if (con.getTrustManagerClass() != null) | ||
{ | ||
Class<?> tmClazz = Class.forName(con.getTrustManagerClass()); |
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.
Please rename tmClazz
to tmClass
:)
Yes I saw and submitted some feedback on it. Overall looks great! I'll get that merged, update the naming of clazz/class, and then push a new version. Should have it out some time tomorrow. |
{ | ||
Class<?> tmClazz = Class.forName(con.getTrustManagerClass()); | ||
if (!TrustManager.class.isAssignableFrom(tmClazz)) { | ||
throw new IllegalArgumentException("The class specified by the trustManagerClass property must implement javax.net.ssl.TrustManager"); |
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.
Please also use the Microsoft code formatter for the changes.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com. Once you sign the agreement, the |
Adds two new connection properties that can be used to specify a custom TrustManager implementation: trustManagerClass - Class name of the custom TrustManager trustManagerConstructorArg - Optional argument to pass to the constructor constructor of the custom TrustManager. If encryption is enabled and the trustManagerClass property is specified, it will be retrieved via Class.forName(...). If the optional property trustManagerConstructorArg is specified, then a constructor will be retrieved via getDeclaredConstructors(String.class). The TrustManager will then be instantiated by specified the optional argument as a parameter. If the optional property trustManagerConstructorArg is not specfied, then the default no argument constructor of the class will be retrieved and instantiated.
Previous package name used camel case. Corrects naming to be lower case.
5f4100e
to
bb481c1
Compare
Pushed a bunch of formatting and test cleanup related updates. Ideally I'd like to add a more complicated integration test that validates the server's specific certificate (or CA) but that will have to wait as I don't think the CI infrastructure can support that yet. It'd require changing the startup of the test server to either use an explicit cert or retrieve it prior to the test being run so both the Travis and AppVeyor config would be modified. |
I signed the CLA a while back but I think this PR predates the setup of the bot so it's likely confused. You may need to manually remove the tag. |
Formatting is complete. To minimize the diff I only ran the formatter on the changes themselves. Surrounding code was not reformatted. Also, the enum declarations in SQLServerDriver don't completely align anymore because the name of the new one is longer than the rest (line 281). I'd suggest having that corrected in a separate PR as otherwise the diff would clobber any other PRs that are adding new properties. |
Awesome. Thanks for merging this in! |
…#540) * update readme to master * updated changelog for dev * update changelog for master * Update CHANGELOG.md “more Junit tests for Always Encrypted” instead of “AlwaysEncrpyted “ * Update CHANGELOG.md “more Junit tests for Always Encrypted” instead of “AlwaysEncrpyted “. * Adding another value that will let the users specify default * Replace explicit types with <> (The diamond operator). This removes duplicated information when instantiating variables, and increases the readability. For example: ```Java EnumMap<CallableHandles, HandleAssociation> handleMap = new EnumMap<CallableHandles, HandleAssociation>(CallableHandles.class); ``` becomes ```Java EnumMap<CallableHandles, HandleAssociation> handleMap = new EnumMap<>(CallableHandles.class); ``` and ```Java private static final EnumMap<SSType.Category, EnumSet<JDBCType.Category>> conversionMap = new EnumMap<SSType.Category, EnumSet<JDBCType.Category>>(SSType.Category.class); ``` becomes ```Java private static final EnumMap<SSType.Category, EnumSet<JDBCType.Category>> conversionMap = new EnumMap<>(SSType.Category.class); ``` * Replace for and while loops with foeach loops This increases the overall readability of the code, and has been supported since Java 5. * make TLS keyword default * minor fixes * fix issue when calling getString() on unique identifier parameter * Update SQLServerResource.java Replace tabs with spaces * Update SSLProtocolTest.java * add test * better fix * fix an issue * use properties to specify versions in pom file * modify pom file to skip tests that are taged as slow * use tag to skip certain tests, e.g. mvn clean install -Pbuild42 -DskipTestTag=slow * Applying formatting * Reverting back to tabs * Minor fix * Update SSLProtocolTest.java * Junit changes - added CallableStatementTest and PrecisionScaleTest. Removed RandomData inside jdbc.datatypes beacuse it's redundant * Fix timezone issues with testing * Fix more issues with timezone * update readme on master * update readme on master * update readme on dev * Update README.md * Update README.md * Add 6.2.1 to README * Update README.md * Update 6.2 download link * Update 6.2 download link * Update README.md * Add javadoc/fix issues * Got rid of all the underscores in method names * Add datetime/smalldatetime support for tvp Merge branch 'dev' of https://github.com/Microsoft/mssql-jdbc into tvpDatetimeSmallDateTime # Conflicts: # src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java # src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDataTable.java * fixed issue with sql_variant * Updated version to 6.3.1 and updated the changelog * setNull depends on sendStringParameterAsUnicode * make sendStringParameterAsUnicode impact updateNull() as well * do not override the maxlength type in sql_variant when reading the values * add snapshot to pom file * Fixed issue with throwing error message for unsupported datatype. Fixed issue with returning class type of time object in sql_variant * update version number * add test * rename a variable * update error messages for localization * fix misspelled words * adding one error message back * too bad.. I forgot to fix the misspelled word.... * fix tests * change cancellded to canceled * Fix typo in Javadoc * Fixes all statement leaks in the driver. * Fix issues with resultset getting closed before it's consumed / closing statements when it's return type * test * Removed logic for closing a statement that is still used somewhere, and added a statement field that can be closed after the prepared statement is closed. * remove unnecessary import * Throw the initial batch exception * Change formatting * updated test file * added test for the fix. * Setting the loginTimout to default in case of a zero value (#456) * fix indentation of SQLServerResource.java * added test * Fix classloader leak issue * comment change * add slow tag to tests * Fixing a few mistakes in SQLServerResource (#459) * Fixing a few mistakes in SQLServerResource * Fix the test * Removing connection property - fipsProvider (#460) * Removing fipsProvider connection property * Minor fix * Update IOBuffer.java Fix typo * update FipsTest * Add a way to remove the entries in the map after they're finished being used * Replace explicit types with <> * Revert "Replace explicit types with <>" This reverts commit e277087. * Remove explicit extends object All objects in Java implicity extend `java.lang.Object` so `extends Object` is redundant. Removing it will reduce confusion, and increase code readability. * Removes redundant if/else statements. For example: ``` if (foo()) { return true; } else { return false; } ``` can be simplified to: ``` return foo(); ``` * Remove unnecessary return statements `return;` is unnecessary if it is the last statement in a `void` method * Simplify overly complex boolean expressions |Example|Replacement| |-|-| |`condition ? true : false`|`condition`| |`condition ? false : true`|`!condition`| |`value == null ? null : value`|`value`| |`result != 0 ? result : 0`|`result`| |`a == b ? a : b`|`b`| * Add missing newlines to appveyor.yml * Add missing newline to .travis.yml * Remove comment from .travis.yml * TimeoutTimer: Check for destroyed TheadGroup Running a query that uses a timer can cause an IllegalThreadStateException if the underlying ThreadGroup has been destroyed * TimeoutTimer: Forgot reference Forgot to add AtomicReference * Use ConcurrentHashMap instead * update 6.3.1 to 6.3.2 * update changelog * update changelog format * increase version with SNAPSHOT after release * refactor checkAndInitActivityId * removing java.xml.bind.DatatypeConverter packages * modified open source tests to run with java 9 * removed redundant methods * remove extra line * indentation * works now, need comments and clean * added comments * add support for "" * removing unused import * removing deprecated APIs in java 9 * fix getExportedKeys() * add switchCatalogs() from the current implementation * fix getCrossReference() * adding tests * fix scenario when catalog is "" and add some tests for it * add tests for getCrossReference() * use batch instead of each statement.execute to reduce performance impact * change column names as JDBC required * fixed the exception thrown by getters on null * replace [ with ' * use PreparedStatement to populate temp table #fkeys_results * change String to Int for some columns * refactor a bit * Add connection properties for specifying custom TrustManager (#74) * Add connection properties to specify a custom TrustManager Adds two new connection properties that can be used to specify a custom TrustManager implementation: trustManagerClass - Class name of the custom TrustManager trustManagerConstructorArg - Optional argument to pass to the constructor constructor of the custom TrustManager. If encryption is enabled and the trustManagerClass property is specified, it will be retrieved via Class.forName(...). If the optional property trustManagerConstructorArg is specified, then a constructor will be retrieved via getDeclaredConstructors(String.class). The TrustManager will then be instantiated by specified the optional argument as a parameter. If the optional property trustManagerConstructorArg is not specfied, then the default no argument constructor of the class will be retrieved and instantiated. * Adding a few simple test to verify the newly added connection properties * Rename custom trustmanager test package name Previous package name used camel case. Corrects naming to be lower case. * Add missing newlines to trustmanager test classes * Refactor references to trust managers in tests * Refactor whitespace and unneeded extends Object * Add resource cleanup to trust manager tests * Refactor failure handling for trust manager test * Rename tmClazz to tmClass * Add new trust manager properties to SQLServerDataSource * Reword comment * Format custom trustmanager changes with auto formatter * recognize CallableStatement parameter names with leading '@' * Revert "removing javax.xml.bind package dependency " * Revert "Removing deprecated APIs in java 9" * Replace manual array copy Replace manual array copying with System.arraycopy(). * Remove redundant String.toString() Calling toString() on a String object is redundant * Replace bare literals Replace bare literals with magic constants. For example: `cal.set(1, 1, 577738, 0, 0, 0);` becomes `cal.set(1, Calendar.FEBRUARY, 577738, 0, 0, 0);` * release process PR * Revert "Remove redundant if/else statements" * remove PR 470 from changelist, and modify the readme file * remove preview from pom * make urls work * remove developer name from POM file * cleanup tables after test * more cleaning of tables and procedures * add snapshot to pom.xml * Make it 6.3.4 * Implement checkDuplicateColumnName to check duplicate columns * Revert "Implement checkDuplicateColumnName to check duplicate columns" This reverts commit e77a046. * Revert "Revert "Implement checkDuplicateColumnName to check duplicate columns"" This reverts commit 8f69956. * Apply same logic for TVP * update changelog and readme for dev branch * Committing fixes for Junit Tests * Failing tests disabled temporarily * Faoling tests disabled temporarily * use try with resources * add another try-with-resource * drop a not needed method * try-with-resources implementation commit 1 * Update .travis.yml Updated docker image tag for mssql-server-linux * Updating travis build script to use :2017-latest tag for SQL Server docker image * Update .travis.yml Updating docker run command as well. * Update docker run command as well * Fix AESetup issue with dropCEK * try-with-resources implementation commit 2 * avoid creating connection for termination * remove null check and change Set object name * tweak to preserve original parameter name for exception message * Refactoring the logic for checking duplicate column into Util class and adding a test case for this * Remove explicit interface reference * Modified bvtTests to be able to test resultSet closing explicitly. * updates gradle dependencies * update * newline * 6.3.4 release (#528) * 6.3.4-preview release * Add 6.3.5-SNAPSHOT to pom file (#530) * add snapshot to pom file * revise the way that pom file uses version number * add -preview * temporarily skipping amedParamMultiPartTest.update5 and amedParamMultiPartTest.update6 * Adding @disabled tag for failing tests
…643) * replace [ with ' * use PreparedStatement to populate temp table #fkeys_results * change String to Int for some columns * refactor a bit * Add connection properties for specifying custom TrustManager (#74) * Add connection properties to specify a custom TrustManager Adds two new connection properties that can be used to specify a custom TrustManager implementation: trustManagerClass - Class name of the custom TrustManager trustManagerConstructorArg - Optional argument to pass to the constructor constructor of the custom TrustManager. If encryption is enabled and the trustManagerClass property is specified, it will be retrieved via Class.forName(...). If the optional property trustManagerConstructorArg is specified, then a constructor will be retrieved via getDeclaredConstructors(String.class). The TrustManager will then be instantiated by specified the optional argument as a parameter. If the optional property trustManagerConstructorArg is not specfied, then the default no argument constructor of the class will be retrieved and instantiated. * Adding a few simple test to verify the newly added connection properties * Rename custom trustmanager test package name Previous package name used camel case. Corrects naming to be lower case. * Add missing newlines to trustmanager test classes * Refactor references to trust managers in tests * Refactor whitespace and unneeded extends Object * Add resource cleanup to trust manager tests * Refactor failure handling for trust manager test * Rename tmClazz to tmClass * Add new trust manager properties to SQLServerDataSource * Reword comment * Format custom trustmanager changes with auto formatter * recognize CallableStatement parameter names with leading '@' * Revert "removing javax.xml.bind package dependency " * Revert "Removing deprecated APIs in java 9" * Replace manual array copy Replace manual array copying with System.arraycopy(). * Remove redundant String.toString() Calling toString() on a String object is redundant * Replace bare literals Replace bare literals with magic constants. For example: `cal.set(1, 1, 577738, 0, 0, 0);` becomes `cal.set(1, Calendar.FEBRUARY, 577738, 0, 0, 0);` * release process PR * Revert "Remove redundant if/else statements" * remove PR 470 from changelist, and modify the readme file * remove preview from pom * make urls work * removing java.xml.bind dependency * remove developer name from POM file * removed dependency of java.xml.bind and updated pom file with latest javadoc pluggin. * disabled appveyor and updated travic to use java 9 * Fixed NoSuchMethodError when compiling with Java 9 but running on java 8 and lower. * add support for JDBC 4.3 APIs and throw unsupported exceptions for those not implemented yet. Also updated the jdbc file to include building jre8 jar. * fixed the javadoc warnings. * update SQLServerJdbc41 to throw unsupportedError for 4.3 APIs * cleanup tables after test * more cleaning of tables and procedures * add snapshot to pom.xml * Make it 6.3.4 * Implement checkDuplicateColumnName to check duplicate columns * Fix for EnablePrepareOnFirstPreparedStatementCall with statement pooling turned off. * Revert "Implement checkDuplicateColumnName to check duplicate columns" This reverts commit e77a046. * Revert "Revert "Implement checkDuplicateColumnName to check duplicate columns"" This reverts commit 8f69956. * Apply same logic for TVP * works for AAD integrated on driver side changes * update changelog and readme for dev branch * use try with resources * add another try-with-resource * drop a not needed method * try-with-resources implementation commit 1 * Update .travis.yml Updated docker image tag for mssql-server-linux * Updating travis build script to use :2017-latest tag for SQL Server docker image * Update .travis.yml Updating docker run command as well. * Update docker run command as well * Fix AESetup issue with dropCEK * remove hardcoded username from exception message * try-with-resources implementation commit 2 * avoid creating connection for termination * remove null check and change Set object name * tweak to preserve original parameter name for exception message * Check for "Account Locked" SQL Exception 18486 while connecitng to SQL Server 2008/2012 and throw Exception * Updated comment for Locked User Account in SQLServerConnection.java * Refactoring the logic for checking duplicate column into Util class and adding a test case for this * Remove explicit interface reference * Modified bvtTests to be able to test resultSet closing explicitly. * updates gradle dependencies * 6.3.4 release (#528) * 6.3.4-preview release * Merge dev to master for 6.3.4-preview release (#529) * TimeoutTimer: Check for destroyed TheadGroup Running a query that uses a timer can cause an IllegalThreadStateException if the underlying ThreadGroup has been destroyed * TimeoutTimer: Forgot reference Forgot to add AtomicReference * recognize CallableStatement parameter names with leading '@' * Replace manual array copy Replace manual array copying with System.arraycopy(). * Remove redundant String.toString() Calling toString() on a String object is redundant * Replace bare literals Replace bare literals with magic constants. For example: `cal.set(1, 1, 577738, 0, 0, 0);` becomes `cal.set(1, Calendar.FEBRUARY, 577738, 0, 0, 0);` * cleanup tables after test * more cleaning of tables and procedures * Implement checkDuplicateColumnName to check duplicate columns * Revert "Implement checkDuplicateColumnName to check duplicate columns" This reverts commit e77a046. * Revert "Revert "Implement checkDuplicateColumnName to check duplicate columns"" This reverts commit 8f69956. * Apply same logic for TVP * use try with resources * add another try-with-resource * drop a not needed method * try-with-resources implementation commit 1 * Update .travis.yml Updated docker image tag for mssql-server-linux * Update .travis.yml Updating docker run command as well. * Fix AESetup issue with dropCEK * try-with-resources implementation commit 2 * avoid creating connection for termination * remove null check and change Set object name * tweak to preserve original parameter name for exception message * Refactoring the logic for checking duplicate column into Util class and adding a test case for this * Remove explicit interface reference * Modified bvtTests to be able to test resultSet closing explicitly. * 6.3.4 release (#528) * 6.3.4-preview release * Add 6.3.5-SNAPSHOT to pom file (#530) * add snapshot to pom file * cleanup * remove error message when running AAD integrated on non-windows OS * read user name from kerberos TGT client name * add error message when Future's outcome has no AuthenticationResult but exception * revise the way that pom file uses version number * add -preview * Fix for static logger member in abstract class 'SQLServerClobBase' * fix handle not fund bug for metadata caching * fix tests * fix failures in requesthandling test * Version Update Configuration Rules. * Added `outdated-dependencies.txt` in .gitignore * remove prepStmtHandle = -1 which disables metadata caching, and added fix for BatchTriggerTest * remove fix for BatchTriggerTest, it breaks PreparedStatementTest * fix hasPreparedStatementHandle() * add the fix for batch trigger * fix "handle is not valid" error. If context changed once, metadata caching is disabled on the connection * clear cached prepared statement handle when pooled connection is removed * merge the fix of batch trigger * change back the test to catch BatchUpdateException exception * disable metadata caching after switching database on connection directly * process all batch first, then throw batch exception is there is one, otherwise throw sql server exception * fix for 477 * add check for updatable resultsets. * re-enable metadata caching after context change * Updated Changelog for 6.3.5 Release * Update pom.xml Fixed minor issue that came during conflict resolution * Updating POM File for next preview snapshot * use preparedStatementHandleCache with database name * retrieve db name without check connection * Adding changes to logger implementation for avoiding logger lookup * if password is null, it means AAD integrated auth * update ADAL4J version to 1.3.0 and also add it into readme file based on request * remove apache code for encoder and remove 4.1 compaitble jars * updated dependency versions in pom file for jdbc 4.3 branch * update samples in JDBC 4.3 branch + change the the junit.platform.version * update sendTimeAsDateTime property to false. * remove deprecated APIs * Fix issue with rowDeleted() causing issues with testing * Revert "Fix issue with rowDeleted() causing issues with testing" This reverts commit 823f08e. * Reverting PR 563, due to test failures * 6.3.6 preview release * add more catch blocks for new getDeclaredConstructor method. * update Travis script with official support of jdk 9 * fix comment * update readme file * make recovery work after ms dtc restart * use javax.security.auth.kerberos instead of sun.security.krb5.Credentials * add log * upadate comments and remove an unused import * remove JNI functions for AAD integrated auth * update the ADAL4J version to 1.4.0 in pom, this will break the compilation for now. * Stress test changes - the static meodifier was causing concurrency issues during stress testing. * use wrapper for sharding APIs * fix another issue with Java 9 regarding bulkcopy connection check * added another fix * added cast to Buffer * change Base64.getEncoder().encodeToString() * Make sure the right wrapper is used for 4.2 and above. * make SQLServerConnection43 throw SQLServerException for new Unsupported APIs * update readme for AKV * & is not allowed in error strings or parsing fails. * update the dtc_xa_interface to use the correct flag for xa_start * fixed random assertion error race condition where 2 requestCompletes executed and the first one set the value to true. place test inside synchronized to fix * spacing * logger security fix don't print keystoresecrets * sonarQube fixes * use dll authentication when DLL is loaded and windows is the operating system * Added XML header * Policheck issue fixes * Updated as per review comments * Update POM XML with SNAPSHOT Tag * change back to retryLogic and change the default of prepared statement caching to false * check if statementPooling is enabled for all retry logics * Revert "update sendTimeAsDateTime property to false." This reverts commit f1674af. * add the setter and getter methods for disableStatementPooling property * move same code in a function. * update for jdk9 * prepare cache in setDisableStatementPooling() * a small fix * fixed the double insertion issue with retry * fixed some review comments and updated test to use raiseError * added a fix with connection property * added unit Tests * Revert back needsPrepare check in retryBasedOnFailedReuseOfCachedHandle * Remove unwanted method parameter * Update changelog and driver version for RTW release * Minor Fix * Minor change to update to jre9 * Added needsPrepare Check and removed error code 99586 check * Added check for needsPrepare and removed 99586 check + Moved NeedsPrepare to class level declaration to avoid multi-threading issues. * Moved 'needsPrepare' to local variable. * Defaulting needsPrepare to true. * Moving JDK9 support to added section * Update SNAPSHOT for upcoming preview release. * Fix conflicts * Revert "Updating connectionResiliency branch with the changes from dev branch (#540)" This reverts commit 06abd7f. * Fix a few merging issues * Disabling 2 more tests, Fixing the issues with Pooling cache size
This PR adds two new connection properties:
trustManagerClass
- The fully qualified class name of a custom javax.net.ssl.TrustManagertrustManagerConstructorArg
- An optional argument to pass to the constructor of the TrustManager.If trustManagerClass is specified and an encrypted connection is requested, the custom TrustManager will be used rather than the default system JVM keystore based TrustManager.
The primary use case for this is to dynamically specify the specific set of certificates that are trusted on a per connection basis without modifying the global settings for the JVM environment (i.e. no need to use a shared / global JKS).
For the error message localization code on line 1803, I've added handling for the case where the caught exception does not provide a localized message. That situation can arise when a custom TrustManager implementation is used as the Driver has no control over that code (it's use specified).
The new code tries to get a localized message, if one is not available then it tries the non-localized message, and if that is not available it defaults to an empty string. The final empty string default is required as subsequent code assumes that all caught exceptions will have a localized error message and would otherwise throw a NullPointerException from within the error handling code.
There's no additional tests for it in the test suite but I've tested it externally using both the specific custom TrustManager we'll be using as well as custom implementation of PermissiveX509TrustManager (which effectively mimics the behavior of trustServerCertificate=true).
See the prior PR submitted to the master branch, #70, for additional commentary and an example use case.