RFR 8043071: Expose session key and KRB_CRED through extended GSS-API

[Sean Mullan]

On 07/09/2014 05:29 AM, Wang Weijun wrote:
>
> On Jul 9, 2014, at 3:21, Sean Mullan <sean.mullan at oracle.com> wrote:
>
>> - several methods are now defined to throw IllegalStateExc, but you
>> (perhaps accidentally) removed the code that does that (ex:
>> getKeyType).
>
> That's because KeyImpl.getKeyType() already throws it. I mentioned it
> in that last paragraph of my previous mail. If you think it makes the
> code more difficult to read, I'll add them back.

No, you don't have to add them back, but if you could add a comment to 
each method indicating that the check for IllegalStateExc is in the 
KeyImpl method, it would help.

>> * KerberosTicket
>>
>> 352-3: put braces around the if (destroyed) statement; same comment
>> applies in rest of code.
>
> There are quite some single line block without braces in this file
> (see init()). My habit was if I don't touch the part I will not
> change it. Some people believe whenever a file is touched it's better
> to update all appearances of such codes. If you think this is good
> I'll update all of them.

Missing braces can more easily lead to inadvertant security bugs. 
Because the destroyed block is security related, I think you should at 
least fix those, but if you want to fix all of them to be consistent, 
that is fine too.

>> * KerberosCredMessage
>>
>> - is there a reason why you didn't override hashCode/equals?
>
> I don't think it's likely for someone to put several
> KerberosCredMessage into a set (which is where I think
> hashCode/equals is useful). Maybe it's a good habit to always verride
> hashCode/equals for these "data" classes?

If you think there is a chance it could be useful, I think it is better 
to do it now than later. In particular, it should be very easy to 
implement, so I would probably do it.

>> - I think it would actually be better to name this class
>> EncryptionKey since that is what it is and that way we would be
>> able to reuse this type in the future if necessary for other
>> structures or APIs that reference this type (EncryptionKey)
>
> This makes sense. I'll withdraw CCC and resubmit it.

Great.

Thanks,
Sean