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

[Weijun Wang]

On 06/12/2012 10:23 PM, Xuelei Fan wrote:
> I have not reviewed too much about the fix. But before looking more
> updates, I was wondering whether it is good to have PrincipalName to be
> paired with Realm in the PrincipalName class.
>
> In RFC4120, realm names and principal names are two different name
> constraints for different purpose. The PrincipalName is defined as:
>
>     PrincipalName   ::= SEQUENCE {
>             name-type       [0] Int32,
>             name-string     [1] SEQUENCE OF KerberosString
>     }
>
> There is no realm field in the structure. When I read Java code, I think
> the PrincipalName class is a one-to-one map to above PrincipalName
> specification.  Otherwise, the class name may be confusing to me.

Yes, it's true that the PrincipalName defined in RFC has no realm, and 
therefore not equivalent to the PrincipalName class in Java, but I think 
there are several reasons we keep the current design:

1. In Kerberos, PrincipalName and Realm always appear together. In this 
sense, they should be modeled as a single data entity.

2. In Java, we had this model from the very beginning, i.e. 
PrincipalName includes the realm info inside. (We should document this). 
The current problem is that whether/when the realm field is filled is 
not consistent.

>
> I was wondering, can we just make the PrincipalName independent from
> Realm, and create a new class, such as XXXPrincipal to pair the
> PrincipalName and Realm?
>
>      XXXPrincipal ::= SEQUENCE {
>          principal    PrincipalName,
>          realm        Realm
>      }

Suppose you agree with me on the model design, this is just a name 
change, and after this name change, the plain PrincipalName class will 
be useless. I doubt this makes any real enhancement, but it really 
touches all the source files.

Thanks
Max

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