RFR 8168518: rcache interop with krb5-1.15

[Xuelei Fan]

On 10/25/2016 5:06 PM, Wang Weijun wrote:
>
>> On Oct 25, 2016, at 4:53 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>
>>>>> Is the "HASH" algorithm a new name?  I'm not sure why introduce this
>>>>> none-standard name.
>>>>
>>>> HASH is actually MD5, it is the label used by MIT krb5 in its
>>>> previous releases. In 1.15, it's SHA256. I just want it to the
>>>> same with MIT krb5.
>> I see.  As the HASH and SHA256 was converted to MD5 and SHA-256 if using (with realAlg()),  and the names are never expose to external, it may be not necessary to use the intermediate variables and realAlg() method.
>>
>>   if (the system property is true) {
>>      // make a note, it is the same name as HASH in MIT world.
>>      set the alg to MD5
>>   } else {
>>      // make a note, it is the same name as SHA256 in MIT world.
>>      set the alg to SHA-56.
>>   }
>
> I have to do translation between arg in MessageDigest.getInstance(arg) and the label inside the rcache file, and store a field name inside AuthTimeWithHash.
OK, I missed this point.

> Do you mean a boolean is enough? Although the value is not fully customizable now I still want it to a string instead of boolean so we can be free to change later.
>
>>
>> Personally, I'd like to use "useMD5" as the ending part of the system property.
>
> OK.
>
>>
>> Otherwise, the source code looks fine to me.
>>
>> For the release note, it compatibility impact on JDK 9/8 are mentioned. What about JDK 7/6?  I guess not.  Would you mind mention the impact on JDK 7/6 explicitly?
>
> OK, I remember hash was introduced in jdk7, will double check.
>
>>
>> BTW, can the rcache file can be refreshed to use SHA-256 if MD5 was used previously?
>
> You mean inside the same file? Newly added entries will be SHA-256 and the old ones remains. Still interoperable but just ignored.
>
That's fine.

Thanks,
Xuelei

> Thanks
> Max
>
>>
>> Thanks,
>> Xuelei
>>
>> On 10/25/2016 3:36 PM, Wang Weijun wrote:
>>> Hi Xuelei
>>>
>>> I rethink about it. Here is the updated webrev:
>>>
>>>   http://cr.openjdk.java.net/~weijun/8168518/webrev.01/
>>>
>>> Changes from webrev.00:
>>>
>>> 1. Default value is now SHA256. This is also the same default for latest MIT krb5.
>>>
>>> 2. User can only revert it to HASH with a boolean system property. No more free setting.
>>>
>>> 3. Test fixed. I forgot to pass the system property to child process.
>>>
>>> Please also review the release note at https://bugs.openjdk.java.net/browse/JDK-8168635.
>>>
>>> Thanks
>>> Max
>>>
>>>> On Oct 25, 2016, at 1:46 PM, Wang Weijun <weijun.wang at oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/25/2016 1:40 PM, Xuelei Fan wrote:
>>>>> Is the "HASH" algorithm a new name?  I'm not sure why introduce this
>>>>> none-standard name.
>>>>
>>>> HASH is actually MD5, it is the label used by MIT krb5 in its previous releases. In 1.15, it's SHA256. I just want it to the same with MIT krb5.
>>>>
>>>>> 39     // The hash algorithm can be "HASH" or "SHA256".
>>>>> What if the algorithm is not one of the two?  Why not use the standard
>>>>> names?  Do you want to support SHA3?
>>>>
>>>> No.
>>>>
>>>> The system property is not meant to be fully customizable. The only usage is that you can set it to SHA256 if you want full krb5-1.15 interoperability. I mainly created it so I can test on it.
>>>>
>>>>>
>>>>> As the DEFAULT_HASH_ALG system property is customizable, the value may
>>>>> be not valid.  Therefore, the AssertionError may be too strong to use in
>>>>> line 310 of KrbApReq.java, and the message can have more information.
>>>>
>>>> OK, I'll change the message.
>>>>
>>>>
>>>>>
>>>>> Otherwise, looks fine to me.
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 10/25/2016 12:18 PM, Wang Weijun wrote:
>>>>>> http://cr.openjdk.java.net/~weijun/8168518/webrev.00/
>>>>>>
>>>>>> Please read https://bugs.openjdk.java.net/browse/JDK-8168518 for the
>>>>>> reason. This code change includes:
>>>>>>
>>>>>> 1. Add a hashAlg field in AuthTimeWithHash.java.
>>>>>>
>>>>>> 2. Add AuthTimeWithHash.DEFAULT_HASH_ALG so we can change it later.
>>>>>>
>>>>>> 3. The fix of the bug is inside DflCache.java:
>>>>>>
>>>>>> @@ -300,7 +302,7 @@
>>>>>>
>>>>>>  if (time.equals(a)) {
>>>>>>      // Exact match, must be a replay
>>>>>>      throw new KrbApErrException(Krb5.KRB_AP_ERR_REPEAT);
>>>>>>
>>>>>> -  } else if (time.isSameIgnoresHash(a)) {
>>>>>> +  } else if (time.sameTimeDiffHash((AuthTimeWithHash)a)) {
>>>>>>
>>>>>>     // Two different authenticators in the same second.
>>>>>>     // Remember it
>>>>>>     seeNewButNotSame = true;
>>>>>>
>>>>>> When a AuthTimeWithHash is seen with a different hash, we believe it's
>>>>>> a new one. Before this fix, we simply compare the HASH string. Now
>>>>>> that the algorithm can be different, we only treat it a new one if the
>>>>>> algorithm is the same and the hash value is different.
>>>>>>
>>>>>> This code change has not tried to understand what a different hashAlg
>>>>>> means and try to re-calculate with it. It is just treated as unknown.
>>>>>>
>>>>>> Tests updated. ReplayCacheTestProc.java enhanced so it can be called
>>>>>> with some special system properties to test interop between a
>>>>>> non-system-default native library or even between 2 different native
>>>>>> libraries.
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>