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

New connection property: sslProtocol #422

Merged
merged 12 commits into from
Aug 18, 2017
Merged

Conversation

ulvii
Copy link
Contributor

@ulvii ulvii commented Jul 31, 2017

Adding a new connection property to let the users specify TLS protocol keyword. Possible values: "TLS", "TLSv1", "TLSv1.1", "TLSv1.2". The default is "TLS". These keywords might behave differently depending on JRE( Oracle, IBM, SAP ), so the users should read the documentation before using them.

Example use-case:
When Suite B or SP800-131a standards are used, only TLSv1.2 must be enabled.

@msftclas
Copy link

@ulvii,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@codecov-io
Copy link

codecov-io commented Jul 31, 2017

Codecov Report

Merging #422 into dev will increase coverage by 0.21%.
The diff coverage is 89.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #422      +/-   ##
============================================
+ Coverage     45.92%   46.13%   +0.21%     
- Complexity     2198     2206       +8     
============================================
  Files           108      108              
  Lines         25210    25246      +36     
  Branches       4164     4169       +5     
============================================
+ Hits          11577    11647      +70     
+ Misses        11711    11678      -33     
+ Partials       1922     1921       -1
Flag Coverage Δ Complexity Δ
#JDBC41 45.8% <89.18%> (+0.12%) 2187 <0> (+2) ⬆️
#JDBC42 45.98% <89.18%> (+0.17%) 2200 <0> (+8) ⬆️
Impacted Files Coverage Δ Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerResource.java 100% <ø> (ø) 4 <0> (ø) ⬇️
.../microsoft/sqlserver/jdbc/SQLServerDataSource.java 46.61% <0%> (-0.52%) 66 <0> (ø)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 46.08% <100%> (+0.12%) 289 <0> (+1) ⬆️
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 55.84% <100%> (+0.59%) 0 <0> (ø) ⬇️
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 77.09% <100%> (+1.64%) 25 <0> (ø) ⬇️
...om/microsoft/sqlserver/jdbc/SimpleInputStream.java 54.81% <0%> (-1.49%) 15% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.42% <0%> (-0.19%) 0% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.86% <0%> (-0.13%) 242% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 40.22% <0%> (ø) 85% <0%> (-1%) ⬇️
...ncurrentlinkedhashmap/ConcurrentLinkedHashMap.java 42.64% <0%> (+0.21%) 47% <0%> (+1%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7741ee9...94cc5e2. Read the comment docs.

@ulvii ulvii mentioned this pull request Jul 31, 2017
@@ -117,6 +117,46 @@ else if (value.toLowerCase(Locale.US).equalsIgnoreCase(ColumnEncryptionSetting.D
}
}

enum SSLProtocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like enum... Good One.

* @throws Exception
*/
@Test
public void testConnectWithWrongProtocols() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

SUGGESTION: You can use JUnit's ability of parametrized test.

In future JUnit might give (or may be available) to pass parameters from some configuration file so no need to touch test code to add extra tests just like TestNG

   @DisplayName("TestForWrongValues") 
    @ParameterizedTest
    @ValueSource(strings={"SSLv1111","SSLv2222","SSLv3111", "SSLv2Hello1111","TLSv1.11","TLSv2.4","HTTPS"}) 
    public void testConnectWithWrongProtocol(String sslProtocol) throws Exception {
        testWithUnSupportedProtocols(sslProtocol); 
    }

You need to upgrade JUnit 1.0.0-M4 & add following dependency

               <dependency>
			<groupId>org.junit.jupiter</groupId>
			<artifactId>junit-jupiter-params</artifactId>
			<version>5.0.0-M4</version>
			<scope>test</scope>
		</dependency>	

Copy link
Contributor Author

@ulvii ulvii Aug 2, 2017

Choose a reason for hiding this comment

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

Hi @nsidhaye,
Thank you for suggestion. I modified the test a bit to not introduce a new dependency and make it compatible with previous JUnit versions. I also agree that, we should consider using new JUnit features.

@ulvii
Copy link
Contributor Author

ulvii commented Aug 2, 2017

@ulvii ulvii merged commit 93e087c into microsoft:dev Aug 18, 2017
@ulvii ulvii deleted the TLSVersionFix branch August 18, 2017 19:47
static SSLProtocol valueOfString(String value) throws SQLServerException {
SSLProtocol protocol = null;

if (value.toLowerCase(Locale.ENGLISH).equalsIgnoreCase(SSLProtocol.TLS.toString())) {
Copy link

Choose a reason for hiding this comment

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

The toLowerCase(Locale.ENGLISH) is unnecesary since equalsIgnoreCase is used.

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

Successfully merging this pull request may close these issues.

8 participants