code review request: 6960894: Better AS-REQ creation and processing
Weijun Wang
Weijun.Wang at Sun.COM
Mon Aug 9 02:11:36 UTC 2010
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
>>>>
>>>
>>>
>>
>
More information about the security-dev
mailing list