code review request: 6960894: Better AS-REQ creation and processing
Weijun.Wang at Sun.COM
Sun Aug 8 19:11:36 PDT 2010
1. state and state checks
2. clear() -> destroy()
split decrypt() to decryptUsingKeys() and decryptUsingPassword()
KrbKdcReq -> KdcComm:
The internal class KdcCommunication name is retained,
On 07/31/2010 08:01 AM, Valerie (Yu-Ching) Peng wrote:
> Hi, Max,
> => 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.
> => 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.
> => 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.
> 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:
>>> 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.
>>> 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