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