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

Added automatic detection of REALM in SPN needed for Cross Domain authentication. #40

Merged
merged 7 commits into from
Apr 17, 2017

Conversation

pierresouchay
Copy link
Contributor

@pierresouchay pierresouchay commented Nov 28, 2016

The original driver only computes the SPN without its REALM. That is why the driver
fails in a cross-domain authentication (ex: user@REALM1 try to log on server@REALM2
while REALM2 and REALM1 are in trust).

This commit solves this by trying to compute the REALM when REALM has not been
provided in the SPN. Which includes both generated SPN and User-Provided SPN.

It also enable Kerberos authentication when only IP is provided as long as reverse
DNS are present since when SPN is provided, and REALM lookup did fail, it will
also try with canonical name and if it works, override the hostname in SPN (feature
not activated when user did provide an SPN)

See Issue 36: #36

This fixes #36

@pierresouchay
Copy link
Contributor Author

Note: Compatibility with IBM JVM could be trivially implemented using: com.ibm.security.jgss.mech.krb5.Krb5RealmUtil.mapHostToRealm( String )

(But I have not the interest nor the IBM JVM to test)

@v-nisidh v-nisidh added the Under Review Used for pull requests under review label Nov 30, 2016
@pierresouchay
Copy link
Contributor Author

Note: this patch only work with realms having the same name (uppercased) than the DNS name of machine as I did not find a way to find a easy way to bind a DNS name to a realm. (It is possible to have different configurations in krb5.conf in theory)

But this is not a real issue with most configurations (and Active Directory enforces this constraint)

The patch has been tested on Oracle and OpenJDk JVMs with Heimdall, MIT Kerberos and Windows implemetation on the following OS:

  • Mac OS (JVM 7 and 8)
  • Linux Debian and centOs
  • Windows 7, server 2008 and 2012

@AfsanehR-zz
Copy link
Contributor

@pierresouchay Thank you for submitting PR. Have you tried adding serverSPN connection property? which in your example in issue #36 should be explicitly written as serverSpn=MSSQLSvc/sqlserver:PORT@MYOTHERTRUSTEDREALM.COM
Adding serverspn connection property will work in cross realm authentication scenario.

We tried testing your pull request in our test lab, and without specifying a serverSPN it is failing in a cross realm authentication scenario and was unable to get the correct realm. Thank you.

@AfsanehR-zz AfsanehR-zz added the Waiting for Response Waiting for a reply from the original poster, or affiliated party label Dec 22, 2016
@AfsanehR-zz AfsanehR-zz self-assigned this Dec 22, 2016
@pierresouchay
Copy link
Contributor Author

pierresouchay commented Dec 22, 2016

Hello @v-afrafi Yes, we do not use serverSpn, we call directly setSpn() on the connection, but this code is a port of code we are using in one of our libraries to cross authenticate accross domains.

This code works with a /etc/krb5.conf properly configured (or c:\windows\krb5.ini under Windows). In order to make it works, it requires:

In the [domain_realm] sections an entry like:

[domain_realm]
  .myothertrusterrealm.com = MYOTHERTRUSTEDREALM.COM

(It can also work if .TXT entries are properly set up in DNS)

In my example, I authenticate using a keytab call svc-myservice@MYREALM.com

and I have a $HOME/.java.login.config file with the following data:

SQLJDBCDriver {
    com.sun.security.auth.module.Krb5LoginModule required useKeyTab=true
    useTicketCache=false renewTGT=false doNotPrompt=true storeKey=false
    debug=false  keyTab="/path/to/keytab" principal="svc-myservice@MYREALM.COM";
};

It works well in our environments (we use up to 6 cross domains realms) with Windows, Linux and Mac

Fell free to post your /etc/krb5.conf configuration and your jaas login file

@pierresouchay
Copy link
Contributor Author

Note: on our side we cannot set the Spn for historical reasons in the jdbc URL and sometimes, we used to set IPs in the jdbc URL, that is why DNS canonicalization is useful.

This code is currently working outside the driver in our code (we use a data source over the SQL server datasource) and we call setSpn computed in the same way as this PR on the datasource of your driver.

But I have tested it successfully with this PR applied. The test to ensure everything works:

Create a cross domain SQL connection that works with setSpn() then, use my patch, remove the setSpn() and it still works.

Also works with an IP instead of FQN DNS name for the server as long as reverse PTR for SQL server is properly set up.

@AfsanehR-zz
Copy link
Contributor

@pierresouchay Thank you very much for the information you provided.

This is the config file we are using:
SQLJDBCDriver {
com.sun.security.auth.module.Krb5LoginModule required useKeyTab=true useTicketCache=false renewTGT=false doNotPrompt=true storeKey=false
debug=false keyTab="PathToKeytab" principal="user@MYDOMAIN.AD";
};
and the other trusted realm is also added in the krb5.ini file.

When we don't set the spn at all, what we see is generated as enrichedspn is MSSQLSvc/host.example.com@host.example.com.REALMNAME, however it should be
in the MSSQLSvc/host.example.com@REALMNAME format.
If we set the serverSpn property but don't add the realm name, the pattern you provided is not able to find a match at all.
Feel free to post any other information of your environment setup that might be useful for our testing. Thanks again.

@pierresouchay
Copy link
Contributor Author

Are you sure you are using the right code ?

The enriched Spn is built with this code:

StringBuilder sb = new StringBuilder("MSSQLSvc/"); sb.append(dnsName).append(":").append(portOrInstance).append("@").append(realm.toUpperCase(Locale.ENGLISH))

So it should have at least one ":"

@AfsanehR-zz
Copy link
Contributor

hi @pierresouchay,
Yes, we are using your code. I forgot to add the portnumber in the comment. please check the code comment. Thanks.

int index = 0;
while (index != -1 && index < hostname.length() - 2) {
String realm = hostname.substring(index + 1);
if (realmValidator.isRealmValid(realm)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue in our test lab is that it when it checks for the kdclist in the getRealmValidator
method, it obtains the default kdc instead of remote kdc resulting in producing the following
spn as targetServer.myRealm.com:portNumber@TARGETSERVER.MYREALM.COM:portNumber.OTHERREALM.COM

however, if we add a check to make sure it is not picking up the default kdc it will result in the following spn: targetServer.myRealm.com:portNumber@OTHERREALM.COM

Choose a reason for hiding this comment

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

Below you'll find changes that we made that fixed this issue on our end.

@pierresouchay , can you confirm that these changes don't cause any regression within your tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, tested on my side, but it does not change anything. The result is still the same, it works.

However, I don't understand your patch :

In realm validator, we are supposed to validate a sub part of the hostname until we find a valid realm, but you propose to test against the default realm each time after testing against the realm we are currently testing ?

This test could be DONE outside the realm validator and if successful, simply return a always false validator.

In order to investigate this more closely, I have this test example:

SQLServerDataSource msSQLdataSource = new SQLServerDataSource();    
msSQLdataSource.setURL("jdbc:sqlserver://"+ipOrHostname+":1033;Database=MyDatabase;integratedSecurity=true;authenticationScheme=JavaKerberos");
msSQLdataSource.getConnection().isValid(5);
System.out.println("is ok")

We have 2 DC with realms NY8.AD.MYCOMPANY.COM and PAR.AD.MYCOMPANY.COM

Test with IP address
The result (with a few debug messages) is the following with ipOrHostname = 10.50.4.6

realmValidator: Checking realm=0.50.4.6	; default_realm=NY8.AD.MYCOMPANY.COM
realmValidator: Checking realm=50.4.6	; default_realm=NY8.AD.MYCOMPANY.COM
realmValidator: Checking realm=4.6	; default_realm=NY8.AD.MYCOMPANY.COM
REVERSE DNS FOUND: sql07.par.ad.mycompany.com
realmValidator: Checking realm=sql07.par.ad.mycompany.com	; default_realm=NY8.AD.MYCOMPANY.COM
realmValidator: Checking realm=par.ad.mycompany.com	; default_realm=NY8.AD.MYCOMPANY.COM
SPN before enrichment: MSSQLSvc/10.50.4.6:1033 ; AFTER enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033@PAR.AD.MYCOMPANY.COM
is ok

I saw incidentally during this test that we should not add +1 at line 410 when index is 0... it may cause issue if the SQL server is on the Active Directory machine (Is it you case btw?)

Test with FQDN
The same example with the FQDN name of machine (not its IP address): sql07.par.ad.mycompany.com

Checking realm=sql07.par.ad.mycompany.com	; default_realm=NY8.AD.MYCOMPANY.COM
Checking realm=par.ad.mycompany.com	; default_realm=NY8.AD.MYCOMPANY.COM
SPN before enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033 ; AFTER enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033@PAR.AD.MYCOMPANY.COM

Test with FQDN in same realm

Same 2 examples, but I changed the default realm and my identity to use the same realm as the MS SQL server (and an identity on the same realm):

Checking realm=0.50.4.6	; default_realm=PAR.AD.MYCOMPANY.COM
Checking realm=50.4.6	; default_realm=PAR.AD.MYCOMPANY.COM
Checking realm=4.6	; default_realm=PAR.AD.MYCOMPANY.COM
Checking realm=sql07-par.par.ad.mycompany.com	; default_realm=PAR.AD.MYCOMPANY.COM
Checking realm=par.ad.mycompany.com	; default_realm=PAR.AD.MYCOMPANY.COM
SPN before enrichment: MSSQLSvc/10.50.4.6:1033 ; AFTER enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033@PAR.AD.MYCOMPANY.COM
is ok
Checking realm=ql07-par.mycompany.com	; default_realm=PAR.AD.MYCOMPANY.COM
Checking realm=par.ad.mycompany.com	; default_realm=PAR.AD.MYCOMPANY.COM
SPN before enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033 ; AFTER enrichment: MSSQLSvc/sql07.par.ad.mycompany.com:1033@PAR.AD.MYCOMPANY.COM
is ok

Possible explanation of the behaviour you have in your lab

I don't understand the result. Factually, there is a bug in my implementation in the case where the SQL Server is on the same machine as the realm since it will cut the first char of the domain name. This bug may trigger something tricky if the domain of the SQL Server you are trying to reach is part of a wider realm however.

I imagine you have a domain such as :

DEV.MSFT.COM

and you create another realm with trust called:

MYLAB.DEV.MSFT.COM

and you install an SQL server on it and use mylab.dev.msft.com as server name.

In that case, at in the realm validator, it won't check mylab.dev.msft.com but will check ylab.dev.microsoft.com instead -> no domain found and then will find dev.msft.com to be the correct domain -> realm DEV.MSFT.COM is correct -> it would explain the bug.

Am I correct ?

In that case, the fix is trivial, at line 410, replace
String realm = hostname.substring(index + 1);
by
String realm = hostname.substring(index);
and at line 414, replace:
index = hostname.indexOf(".", index + 1);
by

index = hostname.indexOf(".", index + 1);
if (index != -1)
 index++;

If it solves your issue, that's cool, in other case, I still don't understand the issue and I don't get your proposal for patch.

I will soon update the MR with my change.

Regards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@v-ahibr Ok, I updated the MR and rebased.

I included the bug with index, meaning that if the SQL server is on the same machine as the KDC it should work now.

Does it solves your issue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@v-ahibr about my previous comment, was I correct about having the SQL server instance on same node as the KDC (see my comment with MYLAB.DEV.MSFT.COM and DEV.MSFT.COM)?

@AfsanehR-zz AfsanehR-zz removed the Waiting for Response Waiting for a reply from the original poster, or affiliated party label Jan 4, 2017
Class<?> clz = Class.forName("sun.security.krb5.Config");
Method getInstance = clz.getMethod("getInstance", new Class[0]);
final Method getKDCList = clz.getMethod("getKDCList", new Class[] { String.class });
final Object instance = getInstance.invoke(null);

Choose a reason for hiding this comment

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

changes made to test if RealmValidator is validating with default realm instead of realm we are attempting to access:

final Method getDefaultRealm = clz.getMethod("getDefaultRealm", new Class[0]);
final Object realmDefault = getDefaultRealm.invoke(instance);

@Override
public boolean isRealmValid(String realm) {
try {
Object ret = getKDCList.invoke(instance, realm);

Choose a reason for hiding this comment

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

changes made to test if RealmValidator is validating with default realm instead of realm we are attempting to access:

Object defRet = getKDCList.invoke(instance, realmDefault);

if(ret.toString().equalsIgnoreCase(defRet.toString())) {
    return false;
}

@AfsanehR-zz
Copy link
Contributor

Hi @pierresouchay. Do you have any updates on this pull request?

@pierresouchay
Copy link
Contributor Author

Hello @v-afrafi

Actually, I already updated my MR long time ago, see: #40 (comment)

I did not understand why the change proposed by @v-ahibr could work. I proposed an explanation and updated my patch.

Are my assumptions correct (See my comments on Jan, 5)... ?

@AfsanehR-zz
Copy link
Contributor

AfsanehR-zz commented Feb 21, 2017

Thanks @pierresouchay for clarification. Still with the updated patch, it is not working.
Here is the setup and outputs:
In our setup we have two realms:
EXAMPLE.AD and ANOTHER.AD
kdc for example.ad = kdc1.example.ad
kdc for another.ad = kdc2.another.ad

Running a test from host-01.another.ad and connecting to a sql server on: sql-01.example.ad
(serverSpn should be MSSQLSvc/sql-01.example.ad:1433@EXAMPLE.AD)

When no spn is set:
default_realm=ANOTHER.AD
SPN before enrichment: MSSQLSvc/sql-01.example.ad:1433 ; AFTER enrichment: MSSQLSvc/sql-01.example.ad:1433@SQL-01.EXAMPLE.AD
However it should be MSSQLSvc/sql-01.example.ad:1433@EXAMPLE.AD

when serverSpn is set incomplete (without @)
SPN before enrichment: MSSQLSvc/sql-01.example.ad:1433 after enrichment: SPN before enrichment: MSSQLSvc/SQL-01.example.ad:1433

I can see your patch is working in your setup. Where are your kdcs located?

@pierresouchay
Copy link
Contributor Author

Hi @ v-afrafi,

Thank you for the quick answer.

My Configuration

Our configuration is :

  • PAR.AD.MYCOMPANY.COM is an Active directory realm with client.
  • NY8.AD.MYCOMPANY.COM is another AD realm with mutual trusts with PAR.

Client is located in PAR, has a defaut realm set to PAR.AD... and that's all.

KDC list is retrieved using DNS SRV records. (And all our DNS reverse are properly set)

My Analysis

All of this is weird. To me, it means that getKDCList() returns a list when called with sql-01.example.ad in your case, while it should return it only when called with example.ad.

While looking at the code of JDK, I found this:

  1. JDK6: http://hg.openjdk.java.net/jdk6/jdk6/jdk/file/5f4bfda58ef8/src/share/classes/sun/security/krb5/Config.java#l1061
  2. JDK7: http://hg.openjdk.java.net/jdk7/modules/jdk/file/a37326fa7f95/src/share/classes/sun/security/krb5/Config.java#l1083
  3. JDK8: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/security/krb5/Config.java#l1023

As you can see, implementation greatly differ between JDK6 and JDK7+ ... especially on Windows where some dirty hacks are performed with env variable LOGONSERVER (BTW, are you testing on JDK6?)

JDK7+ looks more promising (most notably with getRealmFromDNS() calls within this method) - which is probably why it succeed with my tested configurations.

In any case, there are lots of ways for getKDCList() to return a list of KDC even when argument is not a realm:

  • if property java.security.krb5.kdc is set for instance - is it your case?
  • if JDK6 or JDK8 (but not JDK7) and os = "Windows" (since it then always use the LOGONSERVER environment variable)

So, in my conclusion:

  1. my solution works well with JDK 7/8 with DNS lookups properly activated OR with kdc set up properly in [realms] section in krb5.conf (or .ini on Windows)
  2. However, the solution fails on Windows with JDK6/8 since it always fallback on LOGONSERVER or USERDNSDOMAIN environment variable OR if user has set Java Property java.security.krb5.kdc -> which always return the same value.

So, in my understanding, it works in the following cases :

  1. [realms] sections properly configured OR dns lookups working well and configured (I am in both cases)
  2. (os = Windows AND JVM=7 BUT not JVM8) OR (os = Unix AND JVM7/8+)

What about your lab - why is it not working ?

I suspect you are running a JVM 8 on Windows... I am right? In that case, it probably fallback to to LOGONSERVER in any case.

All of this is really scary. This method cannot be used as it is.

My Propositions

Step 1: Decide whether getKDCList() is reliable or not

We can check easily whether getKDCList() return some stupid answers (Windows or JDK6 or java.security.krb5.kdc java property set) by prepending some random hostname as a first check. Which means that in your case, at startup, before even trying to resolve the realm.

In your case, it would mean:
at startup, we take the full name specified: sql-01.example.ad and we preprend with "unlikely.to.exist" -> if getKDCList("unlikely.to.exist.sql-01.example.ad") returns something, it means the getKDCList() cannot be trusted (ex: on Windows)

That would at least disable the feature where my patch would do more harm than good. But on systems where it works reliably (Linux/Mac with JVM 7+ and ([realms] sections properly filled OR DNS lookups kdc/realms activated) OR Windows with JVM7 and DNS well configured).

I like the getKDCList() function because it allows the user to use either DNS or specify by himself the REALMS and kdc in the [realms] section of krb5.conf/ini, so I think not using it is not an option.

Possible solution 1

If getKDCList() is reliable, use it, otherwise, switch to dns resolution of realms. This solution has the advantage of working with other JVMs as the one from Oracle as well (such as IBM). Might be a good compromise.

Possible solution 2

if getKDCList() is reliable, use it, otherwise, give up and do not enrich with the realm.

Possible solution 3

if getKDCList() is reliable, use it, otherwise, canonicalize host name and if it works, remove the first part of hostname canonicalized and assume the remaining part is a valid realm name. (so, in you case, if you would use a CNAME such as sql01.msft.com, it would resolve to sql01.example.ad and assume that EXAMPLE.AD is the realm name. Of course, all of this works well if reverse DNS are well configured and if canonical name DOES contains the REALM name, in any case if canonical form of hostname is not the REALM, it would infer a false REALM and fail.

What do you think ?

Do you want to to propose a modified PR with solution 1?
Do you have other ideas ?
(Or maybe my analysis is plain wrong :)

@AfsanehR-zz
Copy link
Contributor

Hi @pierresouchay . Thank you for the thorough description! Just wanted to give an update that I tried your patch in Ubuntu and it is working both in the scenarios of not giving a serverSpn name or giving an IP address!
The issue seems to be with Windows. I was running your pr with Java 8 on Windows 10 and switching to Java 7 is not working either.
The tests are still running in the same realms (another.ad and example.ad, I only changed the OS version, so the setup is correct.)
You mentioned: "if property java.security.krb5.kdc is set for instance - is it your case?"
In windows I do pass this as -Djava.security.krb5.conf=krb5.ini
In windows setup, I also tried giving an incorrect realm as you mentioned, and it is still able to return a value in getKDCList(). We will try to investigate a bit more the windows issue, meanwhile, if you can try running on a windows 10 and you come across any updates do let us know. Thank you!

@pierresouchay
Copy link
Contributor Author

pierresouchay commented Feb 22, 2017

hi @v-afrafi ,

Ok, we are making progress then :)

I actually made it work on Windows using a c:\windows\krb5.ini with all the realms properly set up + options to use DNS.

What do you think about my possible solutions ?
Do you have any preference ?

Actually, I implemented the first solution. I am confident it will work on Windows as well as with other JVMs this time :)

Regards

…hentication.

The original driver only computes the SPN without its REALM. That is why the driver
fails in a cross-domain authentication (ex: user@REALM1 try to log on server@REALM2
while REALM2 and REALM1 are in trust).

This commit solves this by trying to compute the REALM when REALM has not been
provided in the SPN. Which includes both generated SPN and User-Provided SPN.

It also enable Kerberos authentication when only IP is provided as long as reverse
DNS are present since when SPN is provided, and REALM lookup did fail, it will
also try with canonical name and if it works, override the hostname in SPN (feature
not activated when user did provide an SPN)

Fixed issue when SQL Server is the same machine as kdc
@codecov-io
Copy link

codecov-io commented Feb 23, 2017

Codecov Report

Merging #40 into dev will increase coverage by 3.61%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev      #40      +/-   ##
============================================
+ Coverage     29.77%   33.38%   +3.61%     
- Complexity     1251     1493     +242     
============================================
  Files            97      101       +4     
  Lines         23303    23573     +270     
  Branches       3871     3874       +3     
============================================
+ Hits           6939     7871     +932     
+ Misses        15014    14140     -874     
- Partials       1350     1562     +212
Flag Coverage Δ Complexity Δ
#JDBC41 33.3% <0%> (+3.68%) 1488 <0> (+241) ⬆️
#JDBC42 33.26% <0%> (+3.54%) 1485 <0> (+238) ⬆️
Impacted Files Coverage Δ Complexity Δ
...crosoft/sqlserver/jdbc/dns/DNSKerberosLocator.java 0% <0%> (ø) 0 <0> (?)
...com/microsoft/sqlserver/jdbc/dns/DNSUtilities.java 0% <0%> (ø) 0 <0> (?)
...m/microsoft/sqlserver/jdbc/KerbAuthentication.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...com/microsoft/sqlserver/jdbc/dns/DNSRecordSRV.java 0% <0%> (ø) 0 <0> (?)
.../com/microsoft/sqlserver/jdbc/SQLServerJdbc41.java 33.33% <0%> (-26.67%) 0% <0%> (ø)
.../com/microsoft/sqlserver/jdbc/SQLServerJdbc42.java 25% <0%> (-25%) 0% <0%> (ø)
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 19.78% <0%> (-1.11%) 27% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 44.98% <0%> (-1.06%) 182% <0%> (-1%)
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java 12.72% <0%> (-0.07%) 20% <0%> (ø)
... and 37 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 6e8dc59...74de140. Read the comment docs.

@pierresouchay pierresouchay force-pushed the dev branch 2 times, most recently from a9a961c to bbe8285 Compare February 27, 2017 21:10
@pierresouchay
Copy link
Contributor Author

@v-nisidh Great, last amended commits were just cleanups to have cleaner patch

@pierresouchay
Copy link
Contributor Author

Hello @v-afrafi and @v-nisidh

Any news ? Are you happy with the current state of the patch?

Regards

@AfsanehR-zz
Copy link
Contributor

Hi @pierresouchay. I can confirm with not using setspn I see your changes working on windows too! We consider this as a longer term goal and we still need to do more review and internal testing. We do appreciate your patience and will definitely update you here once we made a decision on it. Thank you again.

@pierresouchay
Copy link
Contributor Author

@v-afrafi if you are doing major review of Kerberos, you might also consider this: #163

@v-nisidh v-nisidh added this to the 6.1.7 milestone Apr 4, 2017
@pierresouchay
Copy link
Contributor Author

@v-afrafi & @v-nisidh as requested by @v-suhame in #163 I reformatted the code to use your code formatter. No other change in latest patch.

@v-nisidh v-nisidh added under testing and removed Under Review Used for pull requests under review labels Apr 10, 2017
@pierresouchay
Copy link
Contributor Author

Hello @v-afrafi and @v-nisidh,

Still no issues with this patch? Do you feel confident it can be integrated?

I'll be pleased to help if you have questions or issues

Regards

return serverName.compareTo(o.serverName);
}

public int getPriority() {
Copy link
Contributor

Choose a reason for hiding this comment

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

just wondering if user can benefit from this public methods. If so, can you please add the javaDocs for this and the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,37 @@
package com.microsoft.sqlserver.jdbc.dns;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the licence header files to the new classes? You can find them in other classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

public int getPriority() {
return priority;
}

/**
* Get the weight of DNS record from 0 to 65535.
* @return The weight, hi value means higher probability of selecting the given record for a given priority.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo of high instead of hi maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dnsName = canonicalHostName;
}
catch (UnknownHostException cannotCanonicalize) {
// ignored, but we are in a bad shape
Copy link
Contributor

Choose a reason for hiding this comment

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

can we log this exception if occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Canonicalisation failure is not really an error, it simply means there is no reverse DNS. Most of the time it might not be an error IMHO, but if we are here, it probably means that Kerberos won't work.

It is just an improvement, but not a case of failure IMHO

@@ -0,0 +1,21 @@
package com.microsoft.sqlserver.jdbc.dns;
Copy link
Contributor

Choose a reason for hiding this comment

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

Test files also need licence header :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

@pierresouchay
Copy link
Contributor Author

@v-afrafi no other change? ;)

@AfsanehR-zz AfsanehR-zz merged commit c1f88c5 into microsoft:dev Apr 17, 2017
@AfsanehR-zz
Copy link
Contributor

Thank you @pierresouchay for your great contribution! We really appreciate your work on this feature! And apologies for the delay.

@pierresouchay
Copy link
Contributor Author

Great, than you!

/**
* Find all SRV Record using DNS.
*
* You can then use {@link DNSRecordsSRVCollection#getBestRecord()} to find the best candidate (for instance for Round-Robin calls)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @pierresouchay, thank you very much for your contribution. Just one question and wish you could help us. This line of JavaDoc gives error to us, because Maven cannot find a reference to DNSRecordsSRVCollection#getBestRecord(), could you please let us know how we can fix it? Thank you.

Copy link
Contributor Author

@pierresouchay pierresouchay May 18, 2017

Choose a reason for hiding this comment

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

Done here: #287

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@v-xiangs simply remove the comment or apply #287

Copy link
Contributor

Choose a reason for hiding this comment

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

@pierresouchay thank you for the quick response ! I am going to create a PR to fix another broken JavaDocs, I guess I will apply your suggestion of the fix in that PR. Thank you!

@ulvii
Copy link
Contributor

ulvii commented Aug 31, 2018

Hi @pierresouchay,

Looks like sun.security.krb5.Config is not accessible anymore in JDK10. Reflective access in KerbAuthentication.getRealmValidator() will be denied from the next release of JDK . Would you mind taking a look at this PR again to see if there is an alternative solution?

@pierresouchay
Copy link
Contributor Author

pierresouchay commented Sep 1, 2018

@ulvii I have no idea and I am not working at all on the subject anymore...

At worse, if I did my job correctly it will fallback on DNSKerberosLocator => which will work as well "regular" environments (but not in ones where many things have been overridden in /etc/krb5.conf.

On our side, I enforced people having DNS records to avoid overriding DNS default behaviour, so we are probably not impacted. I apologise, but I won't have time to work on this, but there is probably another hack to find on in order to make it work again. Since you are from Microsoft, you might have more ease contacting Oracle to find an alternative way than me.

The class seems to be still here in OpenJDK 10: http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/src/java.security.jgss/share/classes/sun/security/krb5/Config.java#l1171

So I suspect, reflection is forbidden right?

In that case, the DNS implementation should work anyway while less powerful for administrators, it should work for most shops

Kind Regards

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.

Implement properly Cross Domain Kerberos using JavaKerberos
8 participants