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

[Xuelei Fan]

It's really confusing to mix the name of a principal and realm of a
principal in the same PrincipalName class.

For example, when printing a debug log for "sname", from the name of
"sname", I think it a name of the principal, but the output also include
the realm of the principal.

When I read the spec, I think a message should contains both Realm and
PrincipalName. But when read the code, only PrincipalName class appears
in a call of a method. Feels very strange. I have to read the
implementation carefully in order to understand the debug log and logic
of the code.

Can we make PrincipalName a pure name of a principal, without the realm
field in it? How hard to make the changes? I noticed most of the code
have the name and realm parameters at the same time, just as the krb5 v5
specification.

I can accept your current design if you won't like to make more changes.
I may be able to complete the review tomorrow.

Thanks,
Xuelei

On 6/13/2012 9:12 AM, Weijun Wang wrote:
> 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
>>>>
>>