On 10/25/2012 10:04 PM, Weijun Wang wrote:
>
>
> On 10/25/2012 08:50 PM, Xuelei Fan wrote:
>>>> If you want to get a value from inside krb5.conf, you can call
>>>> getDefault(String). This might be good to get a value from the
>>>> [libdefaults] section. However, the method was designed to be so
>>>> smart that it can recursively search for key/value pairs no
>>>> matter where and how deep it is.
>>
>> If you change the behaviors and want to look for the key-value pair
>> strictly in the exact section. One end is the flexibility, and the
>> other end is strictly levered. Krb5 configuration format is not
>> strictly leveled (for example, the sample section of [appdefaults] in
>> [1]. The format also applies to other sections.). I can see pros and
>> corns on both sides. I think some serious compatibility issues might
>> occur if we miss something, please consider the enhancement carefully.
>
> You are right. I will be very careful if I want to support reading
> [appdefaults]. Obviously, an ordered map will not suffice.
>
>>
>> Based on the current implementation, I only have one comment as follow.
>> Otherwise, looks fine to me.
>>
>> src/share/classes/sun/security/krb5/KdcComm.java
>> -----------------------------------------------
>> - Config.getInstance().getDefault(key, realm);
>> + Config.getInstance().get("realms", realm, key);
>>
>> The method spec say that the value can be defined inside [libdefaults]
>> or [realms]. It seems that the update only able to get value from
>> [realms].
>
> Yes, a value can be defined for all realms, like this,
>
> [libdefaults]
> max_retries = 3
>
> or for a specific realm, like this,
>
> [realms]
> THIS.REALM = {
> max_retries = 3
> }
>
> As for the method spec of getRealmSpecificValue(), I think it's not only
> talking about this method, but for the overall process. The class first
> reads the global value into the static field defaultKdcRetryLimit, and
> then, when the realm is known, use this method to see if there is a
> specific value defined for that realm. The method itself does not
> understand the whole logic and it's only reading the realm-specific
> value. Therefore, I think it's equivalent to the new .get("realms",
> realm, key) call.
>
OK. Thanks for the clarification.
Xuelei
> Thanks
> Max
>
>
>>
>>
>> Xuelei
>>
>> [1]:
>> http://web.mit.edu/kerberos/krb5-1.5/krb5-1.5/doc/krb5-admin/appdefaults.html#appdefaults
>>
>>
>>
>> On 8/15/2012 8:25 PM, Weijun Wang wrote:
>>> Updated at
>>>
>>> http://cr.openjdk.java.net/~weijun/7184246/webrev.01
>>>
>>> Changes:
>>>
>>> 1. String values (even if there is only one) for the same key are stored
>>> in a vector. Two methods are provided:
>>>
>>> get(String... keys) returns the last value
>>> getAll(String... keys) returns all values concatenated
>>>
>>> so if you see
>>>
>>> [realms]
>>> R = {
>>> kdc = k1
>>> kdc = k2
>>> }
>>>
>>> then get("realms","R","kdc") returns k2, getAll("realms","R","kdc")
>>> returns "k1 k2".
>>>
>>> 2. SCDynamicStore is updated to be consistent with above. I also break
>>> the getConfig() method into 2 so that I can write a test.
>>>
>>> Thanks
>>> Max
>>>
>>> On 08/02/2012 10:14 PM, Weijun Wang wrote:
>>>> Hi Valerie
>>>>
>>>> Please take a look at this
>>>>
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7184246
>>>>
>>>> The code changes include:
>>>>
>>>> 1. Config.java:
>>>> a. Retrieve settings using .get(String... keys) now
>>>> b. Some changes to parsing. The sub-section depth can be at any
>>>> level. For compatibility reasons, multiple values for the same key are
>>>> only for [realms] and [capaths] sections.
>>>> c. Still using Hashtable and Vector because I don't want to make
>>>> changes to Mac's SCDynamicStoreConfig.m.
>>>>
>>>> 2. initStatic() methods in several classes that read krb5.conf settings
>>>> to static fields
>>>>
>>>> 3. All other old calls to getDefault() methods.
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>>
>>>>
>>>> -------- Original Message --------
>>>> 7184246: Simplify Config.get() of krb5
>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7184246
>>>>
>>>> === *Description*
>>>> ============================================================
>>>> This is about the internal class sun.security.krb5.Config.
>>>>
>>>> If you want to get a value from inside krb5.conf, you can call
>>>> getDefault(String). This might be good to get a value from the
>>>> [libdefaults] section. However, the method was designed to be so smart
>>>> that it can recursively search for key/value pairs no matter where and
>>>> how deep it is.
>>>>
>>>> For example, given a krb5.conf
>>>>
>>>> [s1]
>>>> a=b
>>>>
>>>> [s2]
>>>> c=d
>>>>
>>>> [s3]
>>>> e = {
>>>> f = g
>>>> }
>>>>
>>>> getDefault("a") = "b", getDefault("c") = "d", and astonishingly,
>>>> getDefault("f") = "g".
>>>>
>>>> I don't think this is a good design, for several reasons:
>>>>
>>>> 1. It depends on the order of sections if there are key/value pairs
>>>> with
>>>> the same key in different sections.
>>>>
>>>> 2. It ignores wrong settings. For example, when doing a cross-realm
>>>> auth, the Realm.getRealmsList(from,to) is used to get a path which
>>>> should be defined in [capaths]. However, the method simply crawls
>>>> recursively into any subsection it found and won't notice the [capaths]
>>>> being mistakenly typed as [capath]
>>>>
>>>> 3. It lacks certain features. Because the function always return a
>>>> String (same with the getDefault(String,String) method),
>>>> getDefault("e")
>>>> can only return a null. Therefore there is no way to find out the
>>>> existence of the subsection e unless we also know it contains a key f.
>>>>
>>>> 4. The current Config class needs to know what subsections contains
>>>> more
>>>> subsections, and it hardcodes names like [capaths] and [realms].
>>>>
>>>> In short, it's just too smart and becomes unsafe to use. I suggest
>>>> removing all this smartness and a user must use the full paths to get a
>>>> value, say,
>>>>
>>>> kdc = config.get("realms", "SUN.COM", "kdc")
>>>>
>>>> My proposed spec is:
>>>>
>>>> 1. The Config class should understand a krb5.conf without knowing any
>>>> specific section names. All it maintains is a Value, which can be
>>>> either of
>>>>
>>>> String
>>>> List<Value>
>>>> TreeMap<String,Value>
>>>>
>>>> Here I use TreeMap to preserve the order (might not be necessary).
>>>>
>>>> 2. The basic retrieval method will be
>>>>
>>>> Value get(String... key)
>>>>
>>>> 3. There are simpler methods if you already know what the type in your
>>>> case is
>>>>
>>>> String getAsString(String... key)
>>>> List<String> getAsStringList(String... key)
>>>>
>>>> The compatibility risk will be low, and if there really comes a
>>>> compatibility issue, most likely it will be because the caller had
>>>> written his krb5.conf wrong.
>>>>
>>>> One of the advantages of the original design is that when a key is
>>>> provided in both [libdefaults] and a given realm, the method can
>>>> find it
>>>> anyway. This will be useful for keys like kdc_timeout, max_retries.
>>>> However, I think this automatic retrieval is confusing and error-prone,
>>>> I'd rather manually call the get() method twice.
>>>>
>>