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

[Wang Weijun]

On Jul 9, 2014, at 3:21, Sean Mullan <sean.mullan at oracle.com> wrote:

> Hi Max,
> 
> Here are my comments:
> 
> * KerberosKey
> 
> 37: Did you mean to use @link instead of @see?

Yes.

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

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

> 
> * Krb5Context
> 
> 1446-9: can myName or peerName be null?

No. They won't be null after a context is established.

There exists something called Anonymous Kerberos (we don't support it) but in this case there is no KRB_CRED to send.

> 
> * Context
> 
> 451: I think you should also print the exception

OK.

> 
> * KerberosCredMessage
> 
> 52-4: these fields should be private

Absolutely correct.

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

> 
> * KerberosSessionKey
> 
> - methods don't need to be final since class is

OK.

> - some of the methods don't check if it is destroyed (ex: getKeyType)

See above (KerberosKey).

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

Thanks
Max

> 
> --Sean
> 
> On 07/04/2014 02:12 AM, Wang Weijun wrote:
>> Hi All
>> 
>> Please review the change at
>> 
>>   http://cr.openjdk.java.net/~weijun/8043071/webrev.00/
>> 
>> Two new inquire type KRB5_GET_SESSION_KEY_EX and KRB5_GET_KRB_CRED are added to get the session key (in a new format) and the KRB_CRED message. Two new classes are created as the types of their return values.
>> 
>> Spec for InquireType values are moved into the InquireType class itself.
>> 
>> For the existing KerberosKey class, the "A call to any of its other methods after this will cause an IllegalStateException to be thrown" in the spec of destroy() is not precise since we know isDestroyed() and toString/hashCode/equals() do not throw it. The sentence is removed and a @throws clause is added to those methods that do throw the exception. The new KerberosSessionKey and KerberosCredMessage have the same style.
>> 
>> Since the data class KeyImpl already throws IllegalStateException when it's destroyed, KerberosKey and KerberosSessionKey do not check again. If you think this makes the code difficult to read, I can add them back.
>> 
>> Thanks
>> Max
>>