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

Valerie (Yu-Ching) Peng valerie.peng at
Fri Jul 30 17:01:26 PDT 2010

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?
=> 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.
=> 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?
=> 2) I am open for a name change since the current naming seems to 
imply an inheritance relationship which you've changed.
=> 1) line 72 "one and only is non-null" may be clearer as "one of them 
must 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.


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

-------------- next part --------------
An HTML attachment was scrubbed...

More information about the security-dev mailing list