Code revire request: 6966259 (was Re: Avoid PrincipalName with realm == null)
Xuelei Fan
xuelei.fan at oracle.com
Fri Jun 15 09:06:24 UTC 2012
Looks fine to me.
Thanks,
Xuelei
On 6/15/2012 3:59 PM, Weijun Wang wrote:
> 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
>>>>>>
>>>>
>>
More information about the security-dev
mailing list