Skip to content

New connection property: sslProtocol#422

Merged
ulvii merged 12 commits intomicrosoft:devfrom
ulvii:TLSVersionFix
Aug 18, 2017
Merged

New connection property: sslProtocol#422
ulvii merged 12 commits intomicrosoft:devfrom
ulvii:TLSVersionFix

Conversation

@ulvii
Copy link
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

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
}
}

enum SSLProtocol {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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.

9 participants