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

[Weijun Wang]

Webrev updated at

    http://cr.openjdk.java.net/~weijun/7184815/webrev.01/

Add comments before toString and inside toStringIndented.

> With the new impl, the responsibility for doing indention when
> printing out the String 'obj' is the caller's responsibility.

Well, not really. The indentation is removed in the new impl because 
there is no more indentation now, i.e. the String is printed *inline*.

Compare the old output:

    key = {
        value
    }

to new output

    key = value

The old impl looks like everything is a collection. In the new impl, 
String, Vector and Hashtable have completely different format and are 
easily distinguishable.

Thanks
Max

On 08/10/2012 07:18 AM, Valerie (Yu-Ching) Peng wrote:
> 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.
>>>>>>>
>>>>>>
>>>>>
>>>
>