Looks fine to me. Thanks, Xuelei
On 6/15/2012 3:59 PM, Weijun Wang wrote: > Webrev updated: > > http://cr.openjdk.java.net/~weijun/6966259/webrev.01/ > > Changes made: > > test/sun/security/krb5/name/Constructors.java > src/share/classes/sun/security/krb5/RealmException.java > src/share/classes/sun/security/krb5/Realm.java > src/share/classes/sun/security/krb5/PrincipalName.java > src/share/classes/sun/security/krb5/KrbException.java > > 1. New constructors for KrbException and RealmException that takes > Throwable as an argument. > > 2. In PrincipalName.java: More comments in PrincipalName, and > > 3. Consolidate all nameStrings check into a separate method > > static void validateNameStrings(String[] ns) > > and make sure it's called by all basic constructors. Also, remove all > other duplicate checks. > > 4. Small changes in parseName(String), say > > - if (componentStart < i) { > + if (componentStart <= i) { > > and > > - if (componentStart < i) { > > to make sure that names like "a//b" and "a/" are rejected. > > 5. Remove "equals(PrincipalName other)". I guess it was only used to > compare a PrincipalName and a ServiceName. Also, parseName() is now > private. > > 6. In Realm.java: parseRealmAtSeparator(String). A small change to make > sure a name like "a@" is treated as illegal but not silently bypassed. > > 7. Updating the Constructors regression test to check for illegal names. > > Thanks > Max > > > On 06/15/2012 01:01 PM, Xuelei Fan wrote: >> On 6/15/2012 12:19 PM, Weijun Wang wrote: >>> >>> >>> On 06/15/2012 10:28 AM, Xuelei Fan wrote: >>>> Looks fine to me. Just some minor comments. >>>> >>>> 1. PrincipalName.java >>>> need to make it more clear that PrincipalName is not only for the name >>>> of a principal, but also include the realm. >>>> >>>> - 48 * This class encapsulates a Kerberos principal. >>>> + 48 * This class encapsulates a Kerberos principal, >>>> + * including both of the realm and name of a principal. >>>> >>>> Or some other words like that. >>> >>> Sure. >>> >>>> >>>> 2. KrbAppMessage.java >>>> No copyright date in the header. Other files have the header like >>>> "Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights >>>> reserved." >>> >>> Quite some other files (see krb5/internal) look the same. I remember I >>> was told not to touch them. >>> >> OK. >> >>>> >>>> 3. Realm.java >>>> 63 } catch (KrbException ke) { >>>> 64 RealmException re = new RealmException(ke.getMessage()); >>>> 65 re.initCause(ke); >>>> 66 throw re; >>>> 67 } >>>> >>>> I think you make a lot cleanup to exception thrown with just one call, >>>> like the one in KerberosPrincipal.java: >>>> - IOException ioe = new IOException(e.getMessage()); >>>> - ioe.initCause(e); >>>> - throw ioe; >>>> + throw new IOException(e); >>>> >>>> Would you like to use the same style for the update in Realm.java? >>> >>> Unfortunately RealmException does not provide such a constructor. Are >>> you suggesting me to create one? >>> >> Why not? I think a Exception class should always have such constructor. >> >> Xuelei >> >>>> >>>> Otherwise, looks fine to me. >>>> >>>> As there are too many changes, I would suggest you run a thoroughly >>>> testing before integration in case of any missing. >>> >>> Sure. >>> >>> Thanks >>> Max >>> >>>> >>>> Regards, >>>> Xuelei >>>> >>>> >>>> On 6/11/2012 1:29 PM, Weijun Wang wrote: >>>>> Hi Valerie >>>>> >>>>> Here is the webrev: >>>>> >>>>> http://cr.openjdk.java.net/~weijun/6966259/webrev.00/ >>>>> >>>>> The patch is quite long, but most of the real changes are in a few >>>>> classes: >>>>> >>>>> PrincipalName.java: >>>>> . All fields are final and non-null non-empty now >>>>> . All constructors have a realm argument, including those from DER >>>>> . Add a new static method tgsService(r1, r2) to get AS/TGS name >>>>> . Remove ServiceName class >>>>> . New reg test: Constructors.java >>>>> >>>>> Realm.java: >>>>> . Field is now final >>>>> . New getDefault() method >>>>> >>>>> KDCReqBody.java: >>>>> . cname and sname share the same realm field. This is also the only >>>>> message format that realm comes after name. >>>>> >>>>> KrbKdcRep.java: >>>>> . Related to the class above. The check() method now has a new >>>>> argument >>>>> isAsReq to deal with AS-REQ and TGS-REQ differently. The old code does >>>>> not check crealm equality, now both name and realm are checked, but >>>>> only >>>>> for AS-REQ. For TGS-REQ, no more check for cname equality, this >>>>> will be >>>>> useful for S4U2proxy where the cname of KDCRep comes from the >>>>> additional >>>>> ticket in request. The old code checks if req.crealm is rep.srealm, >>>>> since KDCReqBody has only one realm field, this is the same as >>>>> checking >>>>> req.srealm and rep.srealm. >>>>> >>>>> CCacheInputStream.java: >>>>> . readPrincipal returns null when there is no realm field. This has >>>>> the >>>>> benefit that a ccache is readable even if there is no valid krb5 >>>>> setting. A normal ccache entry's principal should have its own realm >>>>> field. but when an entry is used to store non-ticket, returning null >>>>> won't trigger an exception. (See readCred about "X-CACHECONF:" style >>>>> entries) >>>>> >>>>> Other trivial code changes: >>>>> . Methods with the realm/name argument pair now has only name >>>>> . In parsing DER, read realm and then merge the info into name >>>>> . In encoding DER, encode the realm from name.getRealm() >>>>> . No need to check realm == null for name >>>>> . No need to print realm in debug output >>>>> . No need to call setRealm() >>>>> >>>>> Thanks >>>>> Max >>>>> >>>>> >>>>> On 06/09/2012 08:23 AM, Valerie (Yu-Ching) Peng wrote: >>>>>> Max, >>>>>> >>>>>> Yes, I think the current model that you described sounds error >>>>>> prone. I >>>>>> don't know the history of the current design. >>>>>> But I do also prefer the described changes that you have. >>>>>> I'd expect the refactoring would make the code clearer and more >>>>>> robust. >>>>>> >>>>>> Valerie >>>>>> >>>>>> On 06/06/12 17:55, Weijun Wang wrote: >>>>>>> Hi Valerie >>>>>>> >>>>>>> The krb5 PrincipalName class has a realm field and the class says >>>>>>> >>>>>>> If null, means the default realm >>>>>>> >>>>>>> Ideally this means if the realm of a name is null then this field >>>>>>> can >>>>>>> be null. Otherwise, it must be filled when created. >>>>>>> >>>>>>> In fact, inside our codes, the field is often filled (using >>>>>>> setRealm()) after it's created. This leads to several strange coding >>>>>>> styles that make the codes confusing and error-prone. >>>>>>> >>>>>>> 1. a lot of setRealm() calls that's far from the creation of the >>>>>>> principal name only when the field needs to be used >>>>>>> 2. a lot of if (realm == null) checks >>>>>>> 3. a lot of "unresolved" names that never has a realm but is >>>>>>> definitely not in the default realm (just because the realm field is >>>>>>> not used inside JDK) >>>>>>> >>>>>>> I am planning to fix this to make the PrincipalName immutable and >>>>>>> always with a non-null non-empty realm. I also plan to make Realm >>>>>>> immutable and remove the ServiceName class (it's quite useless). >>>>>>> >>>>>>> A brief look into the code and protocol suggests this is quite >>>>>>> feasible. In every krb5 message and serialized data (I mean >>>>>>> ccache and >>>>>>> keytab) defined, there is always a realm beside name. This is also >>>>>>> true for most Java methods. And I don't think a name with an >>>>>>> "unresolved" realm should exists at all. If we have to deal with >>>>>>> something like this, I'd rather invent a new class for it. >>>>>>> >>>>>>> The code change will be mostly refactoring, removing a lot of realm >>>>>>> arguments/fields and merging it into name. One behavior change is >>>>>>> that >>>>>>> there will be no name with "unresolved" realm anymore, but I think >>>>>>> this should never be true in a real production environment. In fact, >>>>>>> the public API KerberosPrincipal has >>>>>>> >>>>>>> * @throws IllegalArgumentException if name is improperly >>>>>>> * formatted, if name is null, or if name does not contain >>>>>>> * the realm to use and the default realm is not specified >>>>>>> * in either a Kerberos configuration file or via the >>>>>>> * java.security.krb5.realm system property. >>>>>>> >>>>>>> What's your suggestion? I've been haunted by this several times, >>>>>>> mostly because a setRealm() is not called. >>>>>>> >>>>>>> Thanks >>>>>>> Max >>>>>> >>>> >>
