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

[Weijun Wang]

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.