Removed illegal reflection access in kerberos authentication #839

lilgreenbird · 2018-10-20T00:07:42Z

Removed illegal reflection access, DNSKerberosLocator will handle validation
removed unused test

…another PR

…eanup2

…Test to be consistent with other tests

codecov-io · 2018-10-20T00:30:52Z

Codecov Report

Merging #839 into dev will increase coverage by 0.29%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##                dev     #839      +/-   ##
============================================
+ Coverage     48.29%   48.58%   +0.29%     
- Complexity     2781     2790       +9     
============================================
  Files           116      116              
  Lines         27877    27867      -10     
  Branches       4651     4649       -2     
============================================
+ Hits          13462    13539      +77     
+ Misses        12200    12127      -73     
+ Partials       2215     2201      -14

Flag	Coverage Δ	Complexity Δ
#JDBC42	`48.09% <0%> (+0.28%)`	`2748 <0> (+8)`	⬆️
#JDBC43	`48.49% <0%> (+0.26%)`	`2786 <0> (+7)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...crosoft/sqlserver/jdbc/dns/DNSKerberosLocator.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...m/microsoft/sqlserver/jdbc/KerbAuthentication.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	`43.95% <0%> (-3.3%)`	`15% <0%> (ø)`
...m/microsoft/sqlserver/jdbc/SQLServerException.java	`75.93% <0%> (-1.51%)`	`30% <0%> (-1%)`
...m/microsoft/sqlserver/jdbc/SQLServerResultSet.java	`32.46% <0%> (-0.31%)`	`253% <0%> (-3%)`
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java	`33.49% <0%> (+0.15%)`	`63% <0%> (+1%)`	⬆️
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	`48.09% <0%> (+0.17%)`	`335% <0%> (+1%)`	⬆️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java	`51.96% <0%> (+0.18%)`	`212% <0%> (ø)`	⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java	`62.23% <0%> (+0.23%)`	`0% <0%> (ø)`	⬇️
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	`52.58% <0%> (+0.24%)`	`264% <0%> (+3%)`	⬆️
... and 6 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 5167636...aa6e18e. Read the comment docs.

ulvii · 2018-10-25T18:41:20Z

src/main/java/com/microsoft/sqlserver/jdbc/KerbAuthentication.java

@@ -350,8 +315,8 @@ private String findRealmFromHostname(RealmValidator realmValidator, String hostn
        int index = 0;
        while (index != -1 && index < hostname.length() - 2) {
            String realm = hostname.substring(index);
-            if (authLogger.isLoggable(Level.FINEST)) {
-                authLogger.finest(toString() + " looking up REALM candidate " + realm);
+            if (authLogger.isLoggable(Level.FINER)) {


Why was this necessary?

to make it consistent with the rest of the logging in this file

src/main/java/com/microsoft/sqlserver/jdbc/dns/DNSKerberosLocator.java

ulvii · 2018-10-25T18:44:02Z

src/main/java/com/microsoft/sqlserver/jdbc/dns/DNSKerberosLocator.java

        if (realmName.startsWith(".")) {
            realmName = realmName.substring(1);
        }
        try {
            Set<DNSRecordSRV> records = DNSUtilities.findSrvRecords("_kerberos._udp." + realmName);
            return !records.isEmpty();
        } catch (NameNotFoundException wrongDomainException) {
+            // config error - domain controller can not be located via DNS 


ulvii · 2018-10-25T18:44:12Z

src/main/java/com/microsoft/sqlserver/jdbc/dns/DNSKerberosLocator.java

@@ -30,13 +32,15 @@ public static boolean isRealmValid(String realmName) throws NamingException {
        if (realmName == null || realmName.length() < 2) {
            return false;
        }
+


Please remove this

src/main/java/com/microsoft/sqlserver/jdbc/KerbAuthentication.java

fixed typo

removed change of logging level

cosmetics

removed stupid tab!!

pierresouchay · 2019-01-28T07:39:19Z

This code removal will break Kerberos cross DC authentication on many JVMs, most notably on Windows

ulvii · 2019-01-29T23:48:02Z

Hi @pierresouchay,

Starting from JDK 9, users get a massive warning in their applications complaining about illegal reflective access when doing Kerberos authentication. Unfortunately, it seemed like there was no alternative that would let us preserve the same functionality and we removed the piece of code that contained the reflection. It is, however in our backlog to revisit PR #40 in the future and we would appreciate if you could give us suggestions on how to achieve the same functionality without illegal reflective access.

I should also mention that, the removal of reflection did not break any tests in our test lab and it would be really helpful if you guided us to reproduce the regression with exact test scenario and environment details.

pierresouchay · 2019-01-30T00:44:52Z

@ulvii Yes, I explained a bit the reason, but it is clear it will break the feature. I will blacklist next versions of the driver in our company then. See my comments here: #918 (comment)

lilgreenbird added 30 commits August 30, 2018 14:09

tests cleanup

69fad37

resolved conflict

893d09e

Merge branch 'dev' into dev

4a3e6f8

fixed typo

ded037b

fixes

7d79eea

fixes

bdea814

fixed

a50d513

review comments

a640351

review updates

524143d

review update

6837404

more review changes

6b43e4c

more try-with-resources changes

f5a97de

more

6b10eec

spacing change only

e1038ba

use shared statement in Abstract test again, this will be removed in …

fad3cdb

…another PR

try-with-resources for Stream types

b4060e6

Merge remote-tracking branch 'upstream/dev' into cleanup2

0fec0c8

remove hardcoded table names

49159d8

remove shared AE vars

2041c35

remove hardcoded table names

0d310e4

added comment on driver bug

e037a70

add missing escape characters

d05329e

get rid of unnecessary bvtTestSetup

415c3ec

more hardcode

8389701

Merge branch 'dev' of https://github.com/Microsoft/mssql-jdbc into cl…

6eca59f

…eanup2

fixed issue with AE tests

9234af9

fixed issue with AE tests

3bc3ec7

removed more hardcoded values and modified BatchExecutionWithBulkCopy…

9b118f6

…Test to be consistent with other tests

drop all tables after tests

786c966

format

ba66968

lilgreenbird added 5 commits October 5, 2018 10:15

fix merge errors

58052ba

merge conflicts

6495313

removed illegal reflection access

df682a8

Merge remote-tracking branch 'upstream/dev' into kerb

71d8e56

removed junk file

0447d4e

lilgreenbird changed the title ~~Kerb~~ Removed Illegal reflection access Oct 21, 2018

add comments

b91bd7d

lilgreenbird changed the title ~~Removed Illegal reflection access~~ Removed illegal reflection access in kerberos authentication Oct 21, 2018

cheenamalhotra added this to the 7.1.2 milestone Oct 23, 2018

ulvii reviewed Oct 25, 2018

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/dns/DNSKerberosLocator.java Outdated Show resolved Hide resolved

ulvii reviewed Oct 25, 2018

View reviewed changes

src/main/java/com/microsoft/sqlserver/jdbc/KerbAuthentication.java Show resolved Hide resolved

lilgreenbird added 4 commits October 26, 2018 16:14

Update DNSKerberosLocator.java

611ab2c

fixed typo

Update KerbAuthentication.java

7c51214

removed change of logging level

Update DNSKerberosLocator.java

0023b07

cosmetics

Update DNSKerberosLocator.java

aa6e18e

removed stupid tab!!

ulvii approved these changes Oct 27, 2018

View reviewed changes

cheenamalhotra approved these changes Oct 27, 2018

View reviewed changes

peterbae approved these changes Oct 27, 2018

View reviewed changes

rene-ye approved these changes Oct 27, 2018

View reviewed changes

lilgreenbird merged commit 7f097cd into microsoft:dev Oct 27, 2018

cheenamalhotra mentioned this pull request Jan 3, 2019

[BUG] Unstopped thread when using Kerberos authentication #918

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed illegal reflection access in kerberos authentication #839

Removed illegal reflection access in kerberos authentication #839

lilgreenbird commented Oct 20, 2018 •

edited

Loading

codecov-io commented Oct 20, 2018 •

edited

Loading

ulvii Oct 25, 2018

lilgreenbird Oct 26, 2018

ulvii Oct 25, 2018

ulvii Oct 25, 2018

pierresouchay commented Jan 28, 2019

ulvii commented Jan 29, 2019

pierresouchay commented Jan 30, 2019

Removed illegal reflection access in kerberos authentication #839

Removed illegal reflection access in kerberos authentication #839

Conversation

lilgreenbird commented Oct 20, 2018 • edited Loading

codecov-io commented Oct 20, 2018 • edited Loading

Codecov Report

ulvii Oct 25, 2018

Choose a reason for hiding this comment

lilgreenbird Oct 26, 2018

Choose a reason for hiding this comment

ulvii Oct 25, 2018

Choose a reason for hiding this comment

ulvii Oct 25, 2018

Choose a reason for hiding this comment

pierresouchay commented Jan 28, 2019

ulvii commented Jan 29, 2019

pierresouchay commented Jan 30, 2019

lilgreenbird commented Oct 20, 2018 •

edited

Loading

codecov-io commented Oct 20, 2018 •

edited

Loading