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

[Weijun Wang]

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
>