Code review request: 7184246: Simplify Config.get() of krb5

[Weijun Wang]

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.
>