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

[Weijun Wang]

Webrev updated:

    http://cr.openjdk.java.net/~weijun/6966259/webrev.01/

Changes made:

test/sun/security/krb5/name/Constructors.java
src/share/classes/sun/security/krb5/RealmException.java
src/share/classes/sun/security/krb5/Realm.java
src/share/classes/sun/security/krb5/PrincipalName.java
src/share/classes/sun/security/krb5/KrbException.java

1. New constructors for KrbException and RealmException that takes 
Throwable as an argument.

2. In PrincipalName.java: More comments in PrincipalName, and

3. Consolidate all nameStrings check into a separate method

      static void validateNameStrings(String[] ns)

and make sure it's called by all basic constructors. Also, remove all 
other duplicate checks.

4. Small changes in parseName(String), say

-                    if (componentStart < i) {
+                    if (componentStart <= i) {

and

-        if (componentStart < i) {

to make sure that names like "a//b" and "a/" are rejected.

5. Remove "equals(PrincipalName other)". I guess it was only used to 
compare a PrincipalName and a ServiceName. Also, parseName() is now private.

6. In Realm.java: parseRealmAtSeparator(String). A small change to make 
sure a name like "a@" is treated as illegal but not silently bypassed.

7. Updating the Constructors regression test to check for illegal names.

Thanks
Max


On 06/15/2012 01:01 PM, Xuelei Fan wrote:
> On 6/15/2012 12:19 PM, Weijun Wang wrote:
>>
>>
>> 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.
>>
> OK.
>
>>>
>>> 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?
>>
> Why not? I think a Exception class should always have such constructor.
>
> Xuelei
>
>>>
>>> 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
>>>>>
>>>
>