Request for refactoring: Config.getXXX()
Valerie (Yu-Ching) Peng
valerie.peng at oracle.com
Thu Jul 5 23:03:35 UTC 2012
Yes, I prefer the idea of explicitly specifying the path for finding the
values even if more calls are needed.
Valerie
On 06/18/12 19:07, Weijun Wang wrote:
> 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 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.
>
> 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 full 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 simply 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.
>
> Thanks
> Max
>
More information about the security-dev
mailing list