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

[Weijun Wang]

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.

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

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