Code review request: 7184815 (was Re: OpenJDK krb5 ignore /etc/krb5.conf?)

[Valerie (Yu-Ching) Peng]

Max,

Please find my comments in line.

On 08/08/12 19:33, Weijun Wang wrote:
>
> On 08/09/2012 10:10 AM, Valerie (Yu-Ching) Peng wrote:
>> Max,
>>
>> The new ordering for reading the config files sounds reasonable.
>> However, I have questions on scenarios where the various config files
>> don't exist and the corresponding handling.
>> The new impl of getJavaFileName()/getNativeFileName() return file names
>> even when the file doesn't exist.
>> This will lead to an IOException when loadConfigFile(String) is called.
>> Is this intended?
>
> Yes. But this IOException will be ignored at the end of the private 
> constructor. This is the also the old behavior. The main purpose is 
> that even there is no (any kind of) krb5.conf there is still a chance 
> to read config from DNS etc.

Ok, after re-read the old impl, I agree that the current behavior seems 
to match the old one.

>>
>> Also, the toStringIndented(...) method removed several calls which
>> append the 'prefix' and made various adjustments.
>> Do you have sample output using the new impl? Reading through the code,
>> some don't look quite right.
>
> There are 2 major changes:
>
> 1. A string value does not starts from a newline
> 2. Vectors are inside square brackets.

Hmm, now I see what your new code is doing. However, it's a bit obscure 
and hard to understand as far as I am concerned.
With the new impl, the responsibility for doing indention when printing 
out the String 'obj' is the caller's responsibility.
When given a String 'obj', the toStringIndented(...) ignores the given 
prefix value, and only append the String 'obj' followed by a '\n'. Same 
goes for the Vector 'obj', the prefix is not used at all.
Can you add additional comments to make this clear? I think it'd be good 
to include the syntax of your new output, so it's clear that why prefix 
is only used for Hashtable 'obj'.

Thanks,
Valerie

>
> For example, for a very typical krb5.conf
>
> [libdefaults]
> default_realm = R
> [realms]
> R = {
>   kdc = k1
>   kdc = k2
> }
>
> the old toString is
>
> libdefaults = {
>     default_realm = {
>         R
>     }
> }
> realms = {
>     R = {
>         kdc = {
>                 k1
>                 k2
>         }
>     }
> }
>
> and the new output is
>
> {
>     libdefaults = {
>         default_realm = R
>     }
>     realms = {
>         R = {
>             kdc = [k1,k2,]
>         }
>     }
> }
>
> Thanks
> Max
>
>
>> Thanks,
>> Valerie
>>
>> On 08/02/12 06:41, Weijun Wang wrote:
>>> Ping again.
>>>
>>> On 07/18/2012 02:29 PM, Weijun Wang wrote:
>>>> 7184815: [macosx] Need to read Kerberos config in files
>>>>
>>>> Please take a review:
>>>>
>>>>     http://cr.openjdk.java.net/~weijun/7184815/webrev.00/
>>>>
>>>> I break the config setting to Java setting and native setting, and
>>>> insert the reading of SCDynamicStoreConfig between the two. This 
>>>> should
>>>> preserve the 6u behavior and add a fallback to legacy config files.
>>>>
>>>> No new regression test, because of SCDynamicStoreConfig and system
>>>> config files, will ask SQE to create a manual test.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>
>>>> On 07/18/2012 08:26 AM, Weijun Wang wrote:
>>>>> I'm not familiar with how Mac does it, but normally there are two
>>>>> ways a
>>>>> Kerberos authentication is performed, through the initial login and
>>>>> through kinit. The former is integrated into the system (a pam 
>>>>> module?)
>>>>> and I guess in this case the config is inside 
>>>>> SCDynamicStoreConfig. For
>>>>> the latter, the Kerberos clients are regarded as standalone tools 
>>>>> and a
>>>>> /etc/krb5.conf is needed.
>>>>>
>>>>> Java works in both ways, if there is already a credentials cache it
>>>>> will
>>>>> happily use it. On the other hand, it also includes the 
>>>>> Krb5LoginModule
>>>>> that does all the login itself. Therefore, it should read both
>>>>> styles of
>>>>> config on a Mac.
>>>>>
>>>>> I've filed a new bug, It will appear soon at
>>>>>
>>>>>     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7184815
>>>>>
>>>>> Thanks
>>>>> Max
>>>>>
>>>>>
>>>>> On 07/17/2012 10:35 PM, Mike Swingler wrote:
>>>>>> On Jul 16, 2012, at 8:32 PM, Weijun Wang <weijun.wang at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Ping again.
>>>>>>>
>>>>>>> On 07/05/2012 04:34 PM, Weijun Wang wrote:
>>>>>>>> Hi Scott
>>>>>>>>
>>>>>>>> On Mac since Lion, sun.security.krb5.Config tries to locate the
>>>>>>>> config
>>>>>>>> info in this order:
>>>>>>>>
>>>>>>>> 1. java.security.krb5.conf system property
>>>>>>>> 2. ${jre}/lib/security/krb5.conf
>>>>>>>> 3. SCDynamicStoreConfig
>>>>>>>>
>>>>>>>> The main difference from other platforms is that it will not try
>>>>>>>> config
>>>>>>>> files, say, /Library/Preferences/edu.mit.Kerberos or 
>>>>>>>> /etc/krb5.conf.
>>>>>>>>
>>>>>>>> On the other hand, even /usr/bin/kinit comes with Lion reads the
>>>>>>>> config
>>>>>>>> file (if there is no SCDynamicStoreConfig setting).
>>>>>>>>
>>>>>>>> Is there a special reason for the current Java behavior? I do 
>>>>>>>> notice
>>>>>>>> that the Apple 6u33 already does this.
>>>>>>
>>>>>> No special reason I can think of, beyond simply swapping the
>>>>>> implementation to read from the SCDynamicStoreConfig. Java SE 6 had
>>>>>> previously had been relying on the system to write out a
>>>>>> /Library/Preferences/edu.mit.Kerberos file, but that went away 
>>>>>> with OS
>>>>>> X 10.7, so we didn't see much point in reading the file, since 
>>>>>> little
>>>>>> else on the system would be paying attention to it either for the
>>>>>> purposes of SSO.
>>>>>>
>>>>>> It seems perfectly reasonable that if there are no
>>>>>> SCDynamicStoreConfig entries, falling back to reading the legacy
>>>>>> config files may be a valid option. I'm actually somewhat surprised
>>>>>> that they are consulted by kinit.
>>>>>>
>>>>>> Regards,
>>>>>> Mike Swingler
>>>>>> Apple Inc.
>>>>>>
>>>>>
>>>>
>>