[9] RFR: 8075299: Additional tests for 6857795

[Artem Smotrakov]

Hi Max,

On 08/05/2015 05:40 PM, Weijun Wang wrote:
> Hi Artem
>
> First, you shouldn't need any @modules here, the 
> sun/security/krb5/auto already contains a TEST.properties file 
> covering everything. For the same reason, you should place 
> KinitConfPlusProps.java somewhere inside auto. If you think a subdir 
> is better, put it into auto/tools.
I moved the tests to sun/security/krb5/auto
>
> Back to the tests themselves:
>
> I don't see how KrbTicket.java is related to the bug description. 
> There is no system properties mentioned.
Right, this test doesn't use any krb5-related system properties. It only 
uses krb5 config file, and check if a krb ticket has appropriate state. 
I didn't find any test that checks it (at least, explicitly). The bug 
subject/description is confusing a little bit, I updated it.
>
> KinitConfPlusProps.java does not seems to prove that both conf file 
> and system properties are used. Why should the 1st kinit call fail? 
The test starts a local KDC instance on a random free port. Next, 
hostname and port number are written to krb5 config file. 
"java.security.krb5.kdc" specifies only a host without port. Since the 
property is preferable, kinit tries to connect to a local KDC on default 
port, and connection fails.

By the way, as far as I know, currently it is not possible to specify a 
port number in "java.security.krb5.kdc".

http://hg.openjdk.java.net/jdk9/dev/jdk/file/66e2bdad38a8/src/java.security.jgss/share/classes/sun/security/krb5/Config.java

It may be better to make it possible. I am not sure that ":" symbol can 
be used here because of compatibility risks. Currently it is used to 
separate hostnames:

         String tmp = getProperty("java.security.krb5.kdc");
         if (tmp != null) {
             // The user can specify a list of kdc hosts separated by ":"
             defaultKDC = tmp.replace(':', ' ');
         } else {
             defaultKDC = null;
         }

What do you think?

> Doesn't the principal name also contain the realm here? 
Yes, it does. I think it would be better to use a principal name without 
realm here since krb config contain it. I updated the test.
> The conf file only contains realm and kdc and nothing else. If both 
> conf file and system properties are provided, how do you prove the 
> conf file is also read and not ignored?
The test doesn't check it. I see the following ways to check it:
- Corrupt krb5 conf, and run kinit with it. I suppose it should fail.
- Add some extra parameters to krb5, run kinit, and then try to use 
obtained data, and check that those extra parameters were used (I am not 
sure about details right now, need to do some experiments)

What do you think?

Thanks for review. Please take a look at updated webrev

http://cr.openjdk.java.net/~asmotrak/8075299/webrev.01/

Artem
>
> Thanks
> Max
>
> On 08/05/2015 07:01 PM, Artem Smotrakov wrote:
>> Hello,
>>
>> Please review a couple of new tests which checks if krb5 settings are
>> read correctly from conf file and system properties.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8075299
>> Webrev: http://cr.openjdk.java.net/~asmotrak/8075299/webrev.00/
>>
>> Artem