Code revire request: 6966259 (was Re: Avoid PrincipalName with realm == null)

[Xuelei Fan]

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