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

[Xuelei Fan]

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.

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


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?

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.

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