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
