code review request: 6960894: Better AS-REQ creation and processing

[Weijun Wang]

Webrev updated:
   http://cr.openjdk.java.net/~weijun/6960894/webrev.01/

Changes made:

   KrbAsReqBuilder.java:
     1. state and state checks
     2. clear() -> destroy()

   KrbAsReq.java:
     split decrypt() to decryptUsingKeys() and decryptUsingPassword()

   KrbKdcReq -> KdcComm:
     The internal class KdcCommunication name is retained,

Comments inline.

On 07/31/2010 08:01 AM, Valerie (Yu-Ching) Peng wrote:
> Hi, Max,
>
> KrbAsRep.java
> => 1) the javadoc of its decrypt method only documents the first two
> arguments which seems incomplete. You meant to emphasize that there two
> are user-provided? Maybe you can just enhance the method description. In
> addition, only one of the keys and password arguments is used. Would it
> be clearer to separate this into two methods, one uses keys and the
> other uses password?

Now in two methods.

> => 2) one thing that I find somewhat confusing is the values of "creds".
> The old model sets all of its fields in the constructor. With the new
> model, creds is null until decrypt(...) is called.

I add some comment to the field.

> KrbKdcReq.java
> => 1) for the if-block between line 229 and 232, is it possible for the
> KRB_ERR_RESPONSE_TOO_BIG when 'useTCP' is true? And don't you have to
> check ibuf again after line 231?

The error should only happens when using UDP. RFC 4120 says:

    KRB_ERR_RESPONSE_TOO_BIG              52  Response too big for UDP;
                                                retry with TCP

The only error handled in KrbKdcReq is KRB_ERR_RESPONSE_TOO_BIG error, 
which I treat as a communication-level error. Other errors are handled 
in a higher level.

> => 2) I am open for a name change since the current naming seems to
> imply an inheritance relationship which you've changed.

I thought the name change would make an ugly webrev. It seems not.

> KrbAsReqBuilder.java
> => 1) line 72 "one and only is non-null" may be clearer as "one of them
> must be null".

I mean that one of them MUST be null and the other one MUST NOT be null.

> => 2) keys(..) and pass(..) are initialization methods which must be
> called before getKeys(), right? Can you rename them so it's clearer?
> There is no checking in getKeys() and if called out of sequence, it
> looks to me that it'll error out w/ NPE since eType is still null.

There are also methods like target(), options(), addresses() that all 
must be called before action(). I've added an internal state now.

Thanks
Max

>
> Thanks,
> Valerie
>
> On 07/21/10 12:32, Valerie (Yu-Ching) Peng wrote:
>> On 06/13/10 08:02, Weijun Wang wrote:
>>> Hi Valerie and Andrew
>>>
>>> Please review the following webrev:
>>>
>>>     http://cr.openjdk.java.net/~weijun/6960894/webrev.00
>>>
>>> The major enhancement is KrbAsReqBuilder which generates AS-REQ, sends it, parses any response, and returns a Credentials object. The other big change is KrbKdcReq, it's no longer base class for KrbAsReq and KrbTgsReq, but mainly a vehicle for both kinds of KDC-REQ messages. Maybe it needs a name change?
>>>
>>> Most other changes are about removing duplicate lines.
>>>
>>> Thanks
>>> Max
>>>
>>> Begin forwarded message:
>>>
>>>
>>>> *Change Request ID*: 6960894
>>>> *Synopsis*: Better AS-REQ creation and processing
>>>>
>>>>
>>>
>>>
>>>> === *Description* ============================================================
>>>> The current AS-REQ creation and processing implementation:
>>>>
>>>> 1. spread into multiple source files and have duplicate codes
>>>> 2. cannot deal with PA-DATA in AS-REP
>>>> 3. only use a single salt, and write it into PrincipalName permanently
>>>> 4. generate too many secret keys and have no consistent way to clear them
>>>> 5. does not handle the preferences of PA-ETYPE-INFO2, PA-ETYPE-INFO correctly
>>>>
>>>
>>>
>>
>