RFR 8168518: rcache interop with krb5-1.15

[Wang Weijun]

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

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